Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Routing] Remove usage of deprecated _scheme requirement#9585
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| @@ -145,7 +145,7 @@ public function generate($name, $parameters = array(), $referenceType = self::AB | |||
| * @throws InvalidParameterException When a parameter value for a placeholder is not correct because | |||
| * it does not match the requirement | |||
| */ | |||
| protected function doGenerate($variables, $defaults, $requirements, $tokens, $parameters, $name, $referenceType, $hostTokens) | |||
| protected function doGenerate($variables, $defaults, $requiredSchemes, $tokens, $parameters, $name, $referenceType, $hostTokens) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As the only requirement checked inside doGenerate was '_scheme' I exchanged $requirements with $requiredSchemes. So the semantic of this argument changed. But as this method is protected and not a public api, I thought it may be okay. Do you think this is a BC? Otherwise the only solution would be adding an additional optional parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It is a bc break and cannot be merged as this in 2.3.
The method is called by several other libraries that reuse the routing component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We cannot make such a BC break in 2.3, but not even in master.
Okay I removed the BC on the doGenerate-method and added the schemes-requirement as new optional argument. In this case if this method would be called as before this PR it would still do the same. |
| @@ -141,10 +141,15 @@ public function testScheme() | |||
| { | |||
| $route = new Route('/'); | |||
| $this->assertEquals(array(), $route->getSchemes(), 'schemes is initialized with array()'); | |||
| $this->assertEquals(false, $route->hasScheme('http')); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Please use:assertTrue() &assertFalse().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done
By accident I completely destroyed the commit in my fork, and it seems nothing was merged. I now had to restore the commit from the reflog and gonna open a new PR. |
…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-phpI also adjusted all tests that test '_scheme' to also check the new schemes-requirement.Commits-------557dfaa Remove usage of deprecated _scheme in Routing Component
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.