Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Routing] PSR-4 directory loader#47916

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@derrabus
Copy link
Member

@derrabusderrabus commentedOct 19, 2022•
edited
Loading

QA
Branch?6.2
Bug fix?no
New feature?yes
Deprecations?no (but we could…)
TicketsFix#47881
LicenseMIT
Doc PRsymfony/symfony-docs#17373

This PR adds a PSR-4 directory loader to the Routing component.

When we currently want to load routes from a directory with controllers, we configure it like this:

controllers:resource:../src/Controller/type:attribute

What happens now is thatAnnotationDirectoryLoader searches that directory recursively for*.php files and uses PHP's tokenizer extension to discover all controller classes that are defined within that directory. This step feels unnecessarily expensive given that modern projects follow the PSR-4 standard to structure their code. And if we use the DI component's autodiscovery feature to register our controllers as services, our controller directory already has to follow PSR-4.

A client I'm working with adds the additional challange that they deliver their code to their customers in encrypted form (using SourceGuardian). This means that the PHP files contain encrypted bytecode instead of plain PHP and thus cannot be parsed. We currently overcome this limitation by extendingAnnotationDirectoryLoader and overriding the protectedfindClass() method.

This PR proposes to extend the resource typeattribute and allow to suffix it with a PSR-4 namespace root.

controllers:resource:../src/Controller/type:attribute@App\Controller

In order to use PSR-4 to discover controller classes, the directory path is not enough. We always need an additional piece of information which is the namespace root for the given directory. Without changing large parts of the Config component, I did not find a nice way to pass down that information to the PSR-4 route loader. Encoding that information into thetype parameter seemed like the pragmatic approach, but I'm open to discussing alternatives.

This approach should play nicely with projects that already use attributes for routing and PSR-4 autodiscovery to register controllers as services. In fact, most project should be able to swap thetype: annotation ortype: attribute config withtype: attribute@Your\Namespace\Here and the only difference they notice is that router compilation becomes a bit faster.

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Nice one!

derrabus reacted with heart emoji
@derrabusderrabusforce-pushed thefeature/psr4-attribute-routing branch 2 times, most recently from83ee61d to3d0f3cbCompareOctober 19, 2022 14:07
@derrabus
Copy link
MemberAuthor

If we accept this feature, we can consider deprecatingAnnotationDirectoryLoader andAnnotationFileLoader.

Tobion reacted with thumbs up emoji

@derrabusderrabusforce-pushed thefeature/psr4-attribute-routing branch 2 times, most recently from3a37b7f tod5552b8CompareOctober 19, 2022 14:18
@derrabusderrabusforce-pushed thefeature/psr4-attribute-routing branch fromd5552b8 toc3aef36CompareOctober 19, 2022 14:29
@derrabusderrabusforce-pushed thefeature/psr4-attribute-routing branch fromc3aef36 to11a55adCompareOctober 19, 2022 14:33
@derrabusderrabusforce-pushed thefeature/psr4-attribute-routing branch 2 times, most recently from3a02ca9 tobeca77dCompareOctober 19, 2022 14:54
@derrabus
Copy link
MemberAuthor

Recipe:symfony/recipes#1136

@fancyweb
Copy link
Contributor

We always need an additional piece of information which is the namespace root for the given directory.

Could we make the namespace root part (@App\Controller in your example) optional and just try to useApp\ + follow the directory structure? It would work most of the time isn't it?

@nicolas-grekasnicolas-grekas added the ❄️ Feature FreezeImportant Pull Requests to finish before the next Symfony "feature freeze" labelOct 19, 2022
@derrabus
Copy link
MemberAuthor

We always need an additional piece of information which is the namespace root for the given directory.

Could we make the namespace root part (@App\Controller in your example) optional and just try to useApp\ + follow the directory structure? It would work most of the time isn't it?

TheApp namespace is a convention we use in the recipes, but afaik we don't materialize it in the components. Plus, I'm not really keen on implementing fancy guesswork, tbh.

nicolas-grekas reacted with thumbs up emoji

@derrabus
Copy link
MemberAuthor

Docs:symfony/symfony-docs#17373

@nicolas-grekas
Copy link
Member

What about replacingpsr4@App\Controller byattribute@App\Controller? That'd might be more explicit.

@derrabus
Copy link
MemberAuthor

What about replacingpsr4@App\Controller byattribute@App\Controller? That'd might be more explicit.

It would be more consistent with the other loaders, yes. Let's try that.

This way, the namespace would in fact become optional because if you omit it, the oldAnnotationDirectoryLoader would kick in and parse all PHP files again. I'm not sure if that's a pro or a con, I must admit. 😓

fancyweb reacted with laugh emoji

@derrabusderrabusforce-pushed thefeature/psr4-attribute-routing branch 2 times, most recently from046ef87 toa31b27dCompareOctober 19, 2022 16:39
@derrabus
Copy link
MemberAuthor

Done. I like the change. I'll update the recipe and the docs right away.

nicolas-grekas reacted with heart emoji

@derrabus
Copy link
MemberAuthor

Should I renamePsr4DirectoryLoader toAttributePsr4DirectoryLoader?

@nicolas-grekas
Copy link
Member

Nah, not needed IMHO.

derrabus reacted with thumbs up emoji

@derrabusderrabusforce-pushed thefeature/psr4-attribute-routing branch froma31b27d to158e30dCompareOctober 19, 2022 17:07
@fabpot
Copy link
Member

Thank you@derrabus.

derrabus reacted with heart emoji

@fabpotfabpot merged commit5d69151 intosymfony:6.2Oct 20, 2022
@derrabusderrabus deleted the feature/psr4-attribute-routing branchOctober 20, 2022 07:13
fabpot added a commit that referenced this pull requestOct 22, 2022
…loading (derrabus)This PR was merged into the 6.2 branch.Discussion----------[Config][Routing] Nicer config syntax for PSR-4 route loading| Q             | A| ------------- | ---| Branch?       | 6.2| Bug fix?      | no| New feature?  | yes| Deprecations? | no| Tickets       | Follow-up to#47916| License       | MIT| Doc PR        |symfony/symfony-docs#17373 (WIP)This PR implements an alternative syntax for the PSR-4 route loader introduced in#47916.```YAMLcontrollers:    resource:        path: ./Controllers        namespace: App\Controllers    type: attribute``````XML<routes>    <import type="attribute">        <resource path="./Controllers" namespace="App\Psr4Controllers" />    </import></routes>```Commits-------d7df3be Nicer config syntax for PSR-4 route loading
@fabpotfabpot mentioned this pull requestOct 24, 2022
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull requestOct 26, 2022
This PR was merged into the 6.2 branch.Discussion----------Document the PSR-4 route loaderThis PR documentssymfony/symfony#47916Although the PR does not deprecate `AnnotationDirectoryLoader` and `AnnotationFileLoader`, I chose to recommend only the new `Psr4DirectoryLoader` and `AnnotationDirectoryLoader` in `routing.rst`. In `routing/custom_route_loader.rst`, all four loaders are listed for completeness.Using a class name as resource instead of a file path has always worked, but wasn't documented for some reason. I have added examples for that as well.Commits-------ac2f793 Document the PSR-4 route loader
fabpot added a commit that referenced this pull requestOct 27, 2022
…iles (derrabus)This PR was merged into the 6.2 branch.Discussion----------[Routing] Add tests for loading PSR-4 classes from PHP files| Q             | A| ------------- | ---| Branch?       | 6.2| Bug fix?      | no| New feature?  | no| Deprecations? | no| Tickets       | N/A| License       | MIT| Doc PR        | N/AFollow-up to#47916,#47943This PR adds more tests, demonstrating how to trigger the new PSR-4 loader from a PHP config file.Commits-------416639c Add tests for loading PSR-4 classes from PHP files
@Tobion
Copy link
Contributor

If we accept this feature, we can consider deprecatingAnnotationDirectoryLoader andAnnotationFileLoader.

I think it makes sense. The AnnotationFileLoader seems to assume the class is autoloadable anyway (it does not include the file manually). So it basically requires an autoloading config anyway which is very likely psr-4.

Btw, wouldn't it be usefull if composer offered an API to query what namespace is expected in a directory (and the other way round). Then we could re-use the information composer already has.

derrabus reacted with thumbs up emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@stofstofstof left review comments

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@welcoMatticwelcoMatticwelcoMattic approved these changes

+1 more reviewer

@maxbeckersmaxbeckersmaxbeckers approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

FeatureRouting❄️ Feature FreezeImportant Pull Requests to finish before the next Symfony "feature freeze"Status: Reviewed

Projects

None yet

Milestone

6.2

Development

Successfully merging this pull request may close these issues.

[Routing] Load attributes from a directory following PSR-4

9 participants

@derrabus@fancyweb@nicolas-grekas@fabpot@Tobion@stof@welcoMattic@maxbeckers@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp