Skip to content
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

Better handling of router-level redirections #28806

Closed
stof opened this issue Oct 10, 2018 · 9 comments
Closed

Better handling of router-level redirections #28806

stof opened this issue Oct 10, 2018 · 9 comments
Labels
Help wanted Issues and PRs which are looking for volunteers to complete them. HttpKernel Routing Stalled

Comments

@stof
Copy link
Member

stof commented Oct 10, 2018

Description
Currently, router-level redirections require to use a special controller performing the redirection. But this has several drawbacks:

  • the component cannot provide a RedirectableUrlMatcher, as it cannot have such controller (it does not know about the concept of controller), forcing to keep this child class in FrameworkBundle
  • the redirection is not performed at the time the router runs. Instead, a route is almost matched, and things continue to run in the normal request handling (which means running the security layer for instance, and other request listeners) even though the route was not actually matched. This was reported as a bug in the past (I could not find it again right now). This is especially painful as _route is returned as the route which was not actually matched (but redirected to, to match), which means that the contract for this attribute is not actually that it is the name of the matched route.

A better solution would be to specify a dedicated return format for the matcher, which would tell the RouterListener that it must redirect (which it can do by setting a response, which will stop the request handling early).

We might still need to return the existing (lying) properties alongside the redirection marker to preserve BC for places calling the UrlMatcher directly outside the RouterListener.

@javiereguiluz
Copy link
Member

Can you please outline the changes needed for end users who make redirections in YAML/XML routing files and in controllers? Thanks!

@stof
Copy link
Member Author

stof commented Oct 11, 2018

this is not about redirections defined by users using the RedirectController. This is about redirections performed by the router itself (to add or remove trailing slashes)

@stof
Copy link
Member Author

stof commented Oct 11, 2018

For routes defined using the RedirectController, this redirection is performed using a normal match at the router level, so there is no issue, and no lie saying that it was the matched route (it was).
The issue is that router-initiated redirections currently rely on the same RedirectController internally (and so have to fake a match for the route, even though it was not matched)

@linaori
Copy link
Contributor

linaori commented Oct 11, 2018

@stof
Copy link
Member Author

stof commented Oct 11, 2018

@iltar yes. thanks for finding it again.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jun 13, 2020

I think #30501 provided a workable approach and merging it again in 5.2 would essentially allow us to close this issue.

@carsonbot
Copy link

Thank you for this issue.
There has not been a lot of activity here for a while. Has this been resolved?

@carsonbot
Copy link

Could I get an answer? If I do not hear anything I will assume this issue is resolved or abandoned. Please get back to me <3

@carsonbot
Copy link

Hey,

I didn't hear anything so I'm going to close it. Feel free to comment if this is still relevant, I can always reopen!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Help wanted Issues and PRs which are looking for volunteers to complete them. HttpKernel Routing Stalled
Projects
None yet
Development

No branches or pull requests

5 participants