Skip to content

[Routing] Fix PSR-4 directory loader for abstract classes #48170

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
Nov 9, 2022
Merged

[Routing] Fix PSR-4 directory loader for abstract classes #48170

merged 1 commit into from
Nov 9, 2022

Conversation

derrabus
Copy link
Member

@derrabus derrabus commented Nov 9, 2022

Q A
Branch? 6.2
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #48168, Replaces #48169
License MIT
Doc PR N/A

@carsonbot carsonbot added this to the 6.2 milestone Nov 9, 2022
@derrabus derrabus mentioned this pull request Nov 9, 2022
Closed
MatTheCat added a commit to MatTheCat/symfony that referenced this pull request Nov 9, 2022
@MatTheCat
Copy tests from symfony#48170
3bf53c8
@nicolas-grekas
Copy link
Member

Thank you @derrabus.

@nicolas-grekas nicolas-grekas merged commit 91b2bca into symfony:6.2 Nov 9, 2022
@stof
Copy link
Member

stof commented Nov 9, 2022

shouldn't this also skip enums ?

@derrabus derrabus deleted the bugfix/psr4-routing-abstract branch November 9, 2022 13:21
@derrabus
Copy link
Member Author

derrabus commented Nov 9, 2022

Enums don't cause an issue as far as I can tell. Shall I add one to the fixtures just in case?

@derrabus derrabus mentioned this pull request Nov 9, 2022
Merged
@derrabus
Copy link
Member Author

derrabus commented Nov 9, 2022

#48177

@stof
Copy link
Member

stof commented Nov 9, 2022

The fact that they don't have an issue is probably because the AnnotationClassLoader for the routing does not validate them (and so tries to load routes defined on them but it is unlikely to find some)

@derrabus
Copy link
Member Author

derrabus commented Nov 9, 2022

Yes. Not sure what happens though if someone tried to add Route attributes to a enum method. 🙈

@stof
Copy link
Member

stof commented Nov 10, 2022 •
edited
Loading

Probably a WTF error when trying to instantiate the controller (unless that part takes care of giving a better error message). However, I'm fine with failing for such case, as there is no valid reason for that (while a route in an abstract class might make sense in some cases where you have a child class belong loaded as well)

fabpot added a commit that referenced this pull request Nov 11, 2022
@fabpot
minor #48177 [Routing] Make sure enums don't confuse the PSR-4 loader…
ea9ed6c 
… (derrabus)

This PR was merged into the 6.2 branch.

Discussion
----------

[Routing] Make sure enums don't confuse the PSR-4 loader

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | #48170 (comment)
| License       | MIT
| Doc PR        | N/A

Commits
-------

6ae6af0 [Routing] Make sure enums don't confuse the PSR-4 loader
@fabpot fabpot mentioned this pull request Nov 19, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
Bug Routing Status: Reviewed
Projects
None yet
Milestone
6.2
Development

Successfully merging this pull request may close these issues.

[Routing] Psr4DirectoryLoader issue with abstract classes
5 participants
@derrabus @nicolas-grekas @stof @chalasr @carsonbot