8000 non-catchable pattern in requirements · Issue #10548 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

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

Closed
fabpot opened this issue Mar 27, 2014 · 6 comments
Closed

non-catchable pattern in requirements #10548

fabpot opened this issue Mar 27, 2014 · 6 comments

Comments

@fabpot
Copy link
Member
fabpot commented Mar 27, 2014

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

@Tobion
Copy link
Contributor
Tobion commented Mar 27, 2014

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.

@fabpot fabpot closed this as completed Apr 28, 2014
@Tobion
Copy link
Contributor
Tobion commented Apr 28, 2014

I would leave the issue open because I think it makes sense to change this at some point.
Since we only do a full regex when matching, the requirements check on generating should use the same approach to make it consistent. The only downside as I wrote above is, that you cannot pinpoint individual requirements to fail anymore. But I think usually it's quite obvious even when you can only say "regex X does not match path Y".

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

@fabpot fabpot reopened this Apr 28, 2014
@Tobion
Copy link
Contributor
Tobion commented Dec 16, 2014

Related #8139

@fabpot
Copy link
Member Author
fabpot commented Feb 20, 2018

Closing this old issue.

@fabpot fabpot closed this as completed Feb 20, 2018
@schmittjoh
Copy link
Contributor

Just a quick follow up on this. An alternative to rewriting the validation could be to take the strictRequirements setting and allow this to be changed on a per-route basis instead of a switch on the framework level.

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 / hence the lookahead to decide where the branch name ends and the path starts.

@stof
Copy link
Member
stof commented Jun 5, 2018

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)

fabpot added a commit that referenced this issue Feb 21, 2019
… (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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants
0