-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Routing] Mark getRouteCollection() as internal #19274
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
What would be the alternative recommended approach to do something based on the routes registered? |
@iltar try...catch |
Ah I got confused, the |
We use it (at compile time) in API Platform: https://github.com/api-platform/core/blob/a7441dfcf2349eb94cd590541409b5095efb51fd/src/Bridge/Symfony/Routing/OperationMethodResolver.php#L104 How can we do something similar if it goes deprecated? |
If I decorate the router, things will break on that method as well: https://github.com/iltar/http-bundle/blob/master/src/Router/ParameterResolvingRouter.php#L55 May I suggest to move this method to another interface like the |
We can't deprecate things in an interface because there would be no continuous upgrade path for a 3 to 4 migration (no way to make an app be both 3&4 ready). |
Why not create a Compiled/generated RouteCollection which contains all the routes and there information (as it normally would) in a dumped format. It would speed-up the API platform and bundle Iltar uses 👍 |
I actually thought, that the result of |
@apfelbox none. This is not a use case supported by the component currently |
Status: needs work (or be closed :) ) |
I think this discussion proofed that deprecating it isn't an option. However, we still have to find a way to make it explicit to everyone that they shouldn't use this method:
|
What about adding a comment explaining when it is safe to use and when it's not (with explanations about the consequences of using if at runtime)? |
…ethod (fabpot) This PR was merged into the 3.4 branch. Discussion ---------- [Routing] Add a warning about the getRouteCollection() method | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tests pass? | yes <!-- please add some, will be required by reviewers --> | Fixed tickets | n/a | License | MIT | Doc PR | n/a In #19274, we tried to deprecate `RouterInterface::getRouteCollection()`, but failed at doing so. I propose to add a warning about why one should never use it at runtime as a first step. Commits ------- 8863f06 [Routing] added a warning about the getRouteCollection() method
The
getRouteCollection()
method should not be used in runtime as it'll rebuild the routing cache. It should be marked as internal and removed from the interface in 4.0.I'm not sure how to mark it as being removed from the interface, ended up deprecating it (but
Router::getRouteCollection()
isn't deprecated and shouldn't).