Skip to content

[Routing] UrlGenerator::doGenerate uses deprecated functionality #8176

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 Jun 2, 2013 · 3 comments
Closed

[Routing] UrlGenerator::doGenerate uses deprecated functionality #8176

Tobion opened this issue Jun 2, 2013 · 3 comments
Labels
Routing

Comments

@Tobion
Copy link
Contributor

Tobion commented Jun 2, 2013

The UrlGenerator::doGenerate method expects the "_scheme" requirement to be part of $route->getRequirements() which is deprecated. It's in $route->getSchemes() now.
So the signiture of the method enforces people to use a deprecated feature.
I think it should be fixed for 2.3.
@Tobion Tobion mentioned this issue Sep 1, 2013
Closed
@danez
Copy link
Contributor

danez commented Sep 1, 2013

I think it should be something like this. If the Route requires one of multiple schemes just pick the first for generation.

...
if (!empty($routeSchemes) && ($req = strtolower($routeSchemes[0])) && $scheme !== $req) {
    $referenceType = self::ABSOLUTE_URL;
    $scheme = $req;
}
...

And for code readability and to be able to call $route->getSchemes() I would suggest to change the doGenerate() method to:
protected function doGenerate(Route $route, CompiledRoute $compiledRoute, $parameters, $name, $referenceType)
instead of making more and more parameters. 5 parameters is already a lot, I would even consider doing the compile inside and only supplying Route. Not sure if symfony would count this change as backward incompatibility change.

@Tobion
Copy link
Contributor Author

Tobion commented Sep 1, 2013

Your signature doesnt work. Its used by the dumped generator for performance.

@danez
Copy link
Contributor

danez commented Sep 1, 2013

Ah haven't seen that. Tricky one...

This was referenced Nov 23, 2013
Merged
Merged
fabpot added a commit that referenced this issue Dec 16, 2013
@fabpot
bug #9601 [Routing] Remove usage of deprecated _scheme requirement (D…
0af3d19 
…anez)

This PR was merged into the 2.3 branch.

Discussion
----------

[Routing] Remove usage of deprecated _scheme requirement

**This is exact the same commit as it was in #9585, which was not merged due to my fault. Sorry for the noise.**

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #8898, #8176
| License       | MIT
| Doc PR        |

I removed all usages of the deprecated _scheme requirement inside the Routing Component.
Most parts were pretty easy and after multiple refactorings I came up with the solution to have a Route::hasScheme() method and check against this method.

I also checked for performance and after trying in_array, arra_flip+isset and foreach, the last one was clearly the winner.
https://gist.github.com/Danez/7609898#file-test_performance-php

I also adjusted all tests that test '_scheme' to also check the new schemes-requirement.

Commits
-------

557dfaa Remove usage of deprecated _scheme in Routing Component
@fabpot fabpot closed this as completed Dec 16, 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
3 participants
@fabpot @danez @Tobion