[Routing] Use correct exception message in ChainRouter::match() #110
Conversation
…a RequestMatcherInterface router is used.
| $this->router->add($high, 20); | ||
|
|
||
| $result = $this->router->match($url); | ||
| $this->assertEquals(array('test'), $result); |
There was a problem hiding this comment.
OT: Did symfony-CMF considered to require 5.4 and some point?
There was a problem hiding this comment.
does it make sense to still call assertEquals? This line won't be reached
There was a problem hiding this comment.
That was a copy/paste oversight. Though I tend to leave those in, in case someone removes the @ExpectedException bit after refactoring.
There was a problem hiding this comment.
The additional assertion conflicts with the test's expectation, which is to assert an @expectedException. The test's actual expectation is no longer clear (was the intention to expect an exception or the positive assertion?), so it should be removed.
| @@ -170,10 +170,13 @@ private function doMatch($url, Request $request = null) | |||
| // matching requests is more powerful than matching URLs only, so try that first | |||
| if ($router instanceof RequestMatcherInterface) { | |||
| if (null === $request) { | |||
| $request = Request::create($url); | |||
| $request_for_matching = Request::create($url); | |||
There was a problem hiding this comment.
shouldn't we then move this code to before the foreach loop, so that we only need to do it once?
There was a problem hiding this comment.
Well, it depends. Are we more likely to have 0 routers that implement RequestMatcherInterface, or more likely to have >1 routers that implement RequestMatcherInterface?
Because right now, we optimize for 0. If we move that code outside the foreach and no routers need the created request when called from match(), that'd be an extra Request::create() call.
| @@ -170,10 +170,12 @@ private function doMatch($url, Request $request = null) | |||
| // matching requests is more powerful than matching URLs only, so try that first | |||
| if ($router instanceof RequestMatcherInterface) { | |||
| if (null === $request) { | |||
| $request = Request::create($url); | |||
There was a problem hiding this comment.
how about then:
move $requestForMatching = $request; to before the loop and then inside the loop do
if (empty(requestForMatching)) {
$requestForMatching = Request::create($url);
}
[Routing] Use correct exception message in ChainRouter::match()
|
thx! fix will be in the 1.3.0-RC2 release expected sometime this week |
Allow ChainRouter::match() to use the correct exception message when a RequestMatcherInterface router is used.
Currently, even if doMatch() is called via match(), as long as the router cares for a request, it will create one and the exception handling will not use the path-specific message, it will use the request-based message.