-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
non-catchable pattern in requirements #10548
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
Comments
Yes running the whole regex against the entire URL, instead of validating each requirement would solve the problem. The question is, whether we want to support such lookaround requirements. This would make it impossible to tell which requirement actually failed. |
I would leave the issue open because I think it makes sense to change this at some point. But this also requires some rewrite as the doGenerate https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Routing/Generator/UrlGenerator.php#L148 method does not have the full regex available |
Related #8139 |
Closing this old issue. |
Just a quick follow up on this. An alternative to rewriting the validation could be to take the Typically, there are only a few routes where you need these lookaheads. The use-case here was branch names and paths which are part of the URL on some routes on Scrutinizer. Both, the branch name and the path can contain |
or even detecting automatically the usage of a lookahead in the requirement, to disable the strict check automatically in such case (avoiding the need for extra per-route configuration) |
… (nasimnabavi) This PR was merged into the 3.4 branch. Discussion ---------- [Routing] fix URL generation with look-around requirements | Q | A | ------------- | --- | Branch? | 2.8 up to 4.1 for bug fixes | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #10548 | License | MIT | Doc PR | If you have a non-catchable pattern in requirements like f.e. a positive lookahead (.+(?=/foo/)), the generator will not accept the parameter as the parameter itself cannot fulfil the requirement, but only matches in the context of the entire path. This fix looks for lookAround in the path and ignores checking the requirements if any lookAround exists. Commits ------- c474451 [Routing] fix URL generation with look-around requirements
From #10350 (which contains a unit test to demonstrate the issue)
If you have a non-catchable pattern in requirements like f.e. a positive lookahead (.+(?=/foo/)), the generator will not accept the parameter as the parameter itself cannot fulfil the requirement, but only matches in the context of the entire path.
I'm not sure how/if you would like to fix this, this only adds a failing test case. One way might be to generate the URL and run the matcher against the entire URL.
/cc @Tobion
The text was updated successfully, but these errors were encountered: