Skip to content

[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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 20, 2022
Merged

[Routing] PSR-4 directory loader #47916

merged 1 commit into from
Oct 20, 2022

Conversation

derrabus
Copy link
Member

@derrabus derrabus commented Oct 19, 2022 •
edited
Loading

Q A
Branch? 6.2
Bug fix? no
New feature? yes
Deprecations? no (but we could…)
Tickets Fix #47881
License MIT
Doc PR symfony/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 that AnnotationDirectoryLoader 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 extending AnnotationDirectoryLoader and overriding the protected findClass() method.

This PR proposes to extend the resource type attribute 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 the type 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 the type: annotation or type: attribute config with type: attribute@Your\Namespace\Here and the only difference they notice is that router compilation becomes a bit faster.

@carsonbot carsonbot added this to the 6.2 milestone Oct 19, 2022
nicolas-grekas
nicolas-grekas reviewed Oct 19, 2022
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-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 derrabus force-pushed the feature/psr4-attribute-routing branch 2 times, most recently from 83ee61d to 3d0f3cb Compare October 19, 2022 14:07
@derrabus
Copy link
Member Author

If we accept this feature, we can consider deprecating AnnotationDirectoryLoader and AnnotationFileLoader.

@derrabus derrabus force-pushed the feature/psr4-attribute-routing branch 2 times, most recently from 3a37b7f to d5552b8 Compare October 19, 2022 14:18
@derrabus derrabus force-pushed the feature/psr4-attribute-routing branch from d5552b8 to c3aef36 Compare October 19, 2022 14:29
@derrabus derrabus force-pushed the feature/psr4-attribute-routing branch from c3aef36 to 11a55ad Compare October 19, 2022 14:33
@derrabus derrabus force-pushed the feature/psr4-attribute-routing branch 2 times, most recently from 3a02ca9 to beca77d Compare October 19, 2022 14:54
@derrabus derrabus mentioned this pull request Oct 19, 2022
Merged
@derrabus
Copy link
Member Author

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 use App\ + follow the directory structure? It would work most of the time isn't it?

@nicolas-grekas nicolas-grekas added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Oct 19, 2022
@derrabus
Copy link
Member Author

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 use App\ + follow the directory structure? It would work most of the time isn't it?

The App 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.

@derrabus derrabus mentioned this pull request Oct 19, 2022
Merged
@derrabus
Copy link
Member Author

Docs: symfony/symfony-docs#17373

@nicolas-grekas
Copy link
Member

What about replacing psr4@App\Controller by attribute@App\Controller? That'd might be more explicit.

@derrabus
Copy link
Member Author

What about replacing psr4@App\Controller by attribute@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 old AnnotationDirectoryLoader would kick in and parse all PHP files again. I'm not sure if that's a pro or a con, I must admit. 😓

@derrabus derrabus force-pushed the feature/psr4-attribute-routing branch 2 times, most recently from 046ef87 to a31b27d Compare October 19, 2022 16:39
@derrabus
Copy link
Member Author

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

@derrabus
Copy link
Member Author

Should I rename Psr4DirectoryLoader to AttributePsr4DirectoryLoader?

@nicolas-grekas
Copy link
Member

Nah, not needed IMHO.

@derrabus derrabus force-pushed the feature/psr4-attribute-routing branch from a31b27d to 158e30d Compare October 19, 2022 17:07
@fabpot
Copy link
Member

fabpot commented Oct 20, 2022

Thank you @derrabus.

@fabpot fabpot merged commit 5d69151 into symfony:6.2 Oct 20, 2022
@derrabus derrabus deleted the feature/psr4-attribute-routing branch October 20, 2022 07:13
@derrabus derrabus mentioned this pull request Oct 21, 2022
Merged
fabpot added a commit that referenced this pull request Oct 22, 2022
@fabpot
feature #47943 [Config][Routing] Nicer config syntax for PSR-4 route …
22481f2 
…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.

```YAML
controllers:
    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
@fabpot fabpot mentioned this pull request Oct 24, 2022
Merged
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Oct 26, 2022
@javiereguiluz
minor #17373 Document the PSR-4 route loader (derrabus)
4d1acb5 
This PR was merged into the 6.2 branch.

Discussion
----------

Document the PSR-4 route loader

This PR documents symfony/symfony#47916

Although 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
@derrabus derrabus mentioned this pull request Oct 26, 2022
Merged
fabpot added a commit that referenced this pull request Oct 27, 2022
@fabpot
minor #48010 [Routing] Add tests for loading PSR-4 classes from PHP f…
b87d6f9 
…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/A

Follow-up to #47916, #47943

This 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

Tobion commented Nov 9, 2022

If we accept this feature, we can consider deprecating AnnotationDirectoryLoader and AnnotationFileLoader.

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 derrabus mentioned this pull request Sep 10, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@welcoMattic welcoMattic welcoMattic approved these changes

@maxbeckers maxbeckers maxbeckers approved these changes

Assignees
No one assigned
Labels
Feature Routing ❄️ Feature Freeze Important 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