8000 [Routing] optimize static requirement on compile · Issue #6838 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Routing] optimize static requirement on compile #6838

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
Tobion opened this issue Jan 22, 2013 · 9 comments
Closed

[Routing] optimize static requirement on compile #6838

Tobion opened this issue Jan 22, 2013 · 9 comments
Labels
Routing

Comments

@Tobion
Copy link
Contributor
Tobion commented Jan 22, 2013

I sometimes see route definitions like the following where a requirement for a variable is no real regex but static text:

export:
    path: /export.{_format}
    defaults: { _controller: "Acme:Action:export" }
    requirements:
        _format: csv

This is equivalent to

export:
    path: /export.csv
    defaults: { _controller: "Acme:Action:export", _format: csv }

It can be optimized at compile time and would safe subpattern or even a complete regex match as in this case because the path is static now.
I already know how to implement this. This is just a reminder for me. ^^
Use if (preg_quote($requirement) === $requirement) for detection.
@Tobion
Copy link
Contributor Author
Tobion commented Jan 22, 2013

Oh damnit, it is equivalent for matching but unfortunately not for generating a URL as a _format parameter would become a query param then. Hm, let's see.

If it's not possible to do on compile securely, it can be done at least at dumping the matcher.

@dlsniper
Copy link
Contributor

@Tobion If I were to pickup on this would you guide me thru it? I know you said you know how to do it but I'd rather implement it so I can better understand the new routing component better.

@jfsimon
Copy link
Contributor
jfsimon commented Jul 16, 2013

@Tobion this optimization could be done exclusively by the MatcherDumper.

@jfsimon jfsimon mentioned this issue Jul 16, 2013
Closed
@stof
Copy link
Member
stof commented Jul 16, 2013

Actually, I think this optimization can be done by the RouteCompiler: the generator does not use the compiled regex pattern. Keeping the parameter in the compiled route variables will keep it away from the query string thanks to https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Routing/Generator/UrlGenerator.php#L264

@stof
Copy link
Member
stof commented Jul 16, 2013

hmm, actually, I see 1 issue with this: the param would still be required because of the comparison with route variable but would be omitted because it matches the default added in the route, thus generating a different url.
It would work after fixing #5180

@jfsimon
Copy link
Contributor
jfsimon commented Jul 16, 2013

@stof indeed, it would be better to let the RouteCompiler perform this optimization. I was not aware of #5180, I'll try to ptopose a PR for it, and close #8497 in favaor of a RouteCompiler patch.

@stof
Copy link
Member
stof commented Jul 16, 2013

@jfsimon there is already an old PR for it, but the issue is BC.

@jfsimon
Copy link
Contributor
jfsimon commented Jul 16, 2013

@stof the more I read comments from #5410 the more I doubt it's fixeable :(

@fabpot
Copy link
Member
fabpot commented Sep 13, 2013

Looks like a lot of additional code for a very slight benefit. Closing.

@fabpot fabpot closed this as completed Sep 13, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Routing
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@fabpot @jfsimon @stof @dlsniper @Tobion
0