[Routing] Remove usage of deprecated _scheme requirement #9585
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.
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.
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.
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.
Please use: assertTrue() & assertFalse().
|
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-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
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.