Skip to content

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

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
Closed
wants to merge 1 commit into from
Closed

[Routing] Mark getRouteCollection() as internal #19274

wants to merge 1 commit into from

Conversation

wouterj
Copy link
Member

@wouterj wouterj commented Jul 2, 2016 •
edited
Loading

Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets #19270 (comment)
License MIT
Doc PR -

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

@javiereguiluz javiereguiluz changed the title Mark getRouteCollection() as internal [Routing] Mark getRouteCollection() as internal Jul 2, 2016
@linaori
Copy link
Contributor

linaori commented Jul 2, 2016

What would be the alternative recommended approach to do something based on the routes registered?

@wouterj
Copy link
Member Author

wouterj commented Jul 2, 2016

@iltar try...catch

@linaori
Copy link
Contributor

linaori commented Jul 2, 2016

Ah I got confused, the getRouteCollection() shouldn't be used "runtime". If I call it in a cache warmer, nothing will change.

@dunglas
Copy link
Member

dunglas commented Jul 3, 2016

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?

@linaori
Copy link
Contributor

linaori commented Jul 3, 2016 •
edited
Loading

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 RequestMatcherInterface::matchRequest? That way we can still solve these issues with composition of interfaces while optimizing the public API for the router itself.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jul 3, 2016 •
edited
Loading

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).
👎 for this reason. The alternative is to create a new interface (new name), and deprecate the full interface in favor of the new one.
Really worth it?

@sstok
Copy link
Contributor

sstok commented Jul 5, 2016

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 👍

@apfelbox apfelbox mentioned this pull request Jul 6, 2016
Closed
@apfelbox
Copy link
Contributor

apfelbox commented Jul 6, 2016

I actually thought, that the result of getRouteCollection() is already cached. What is the most performant way to get a list of all routes, if not through this method?

@stof
Copy link
Member

stof commented Jul 6, 2016

@apfelbox none. This is not a use case supported by the component currently

@voronkovich voronkovich mentioned this pull request Jul 6, 2016
Closed
@nicolas-grekas
Copy link
Member

Status: needs work (or be closed :) )

@wouterj
Copy link
Member Author

wouterj commented Aug 24, 2016

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:

  • Can we rewrite component internal logic? (just as recently done with Container::get() usage?)
  • If @internal is kept, will this be sufficient?
  • ... are there other suggestions?

@fabpot
Copy link
Member

fabpot commented Aug 24, 2016

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)?

@nicolas-grekas nicolas-grekas added this to the 3.x milestone Dec 6, 2016
@wouterj wouterj closed this Feb 11, 2017
@wouterj wouterj deleted the deprecate-getRouteCollection branch February 11, 2017 09:05
@nicolas-grekas nicolas-grekas modified the milestones: 3.x, 3.3 Mar 24, 2017
@fabpot fabpot mentioned this pull request Aug 4, 2019
Merged
fabpot added a commit that referenced this pull request Aug 4, 2019
@fabpot
minor #32915 [Routing] Add a warning about the getRouteCollection() m…
5d60221 
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Deprecation Routing Status: Needs Work
Projects
None yet
Milestone
3.3
Development

Successfully merging this pull request may close these issues.
10 participants
@wouterj @linaori @dunglas @nicolas-grekas @sstok @apfelbox @stof @fabpot @javiereguiluz @carsonbot