[Routing] optimize static requirement on compile #6838

Closed
Tobion opened this Issue Jan 22, 2013 · 9 comments

Projects

None yet

5 participants

@Tobion
Member
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
Member
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

@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
jfsimon commented Jul 16, 2013

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

@stof
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
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
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
Member
stof commented Jul 16, 2013

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

@jfsimon
jfsimon commented Jul 16, 2013

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

@fabpot
Member
fabpot commented Sep 13, 2013

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

@fabpot fabpot closed this Sep 13, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment