Skip to content
This repository has been archived by the owner on Jan 21, 2020. It is now read-only.

Route name in RouteResult is route path not name #3

Merged
merged 1 commit into from
Dec 14, 2015

Conversation

weierophinney
Copy link
Member

Looking at the RouteResult in the psrRequest shows the route path such as /customer/create as the matchedRouteName rather than customer.create which is what is specified in the route configuration as such:

[
    'name' => 'customer.create',
    'path' => '/customer/create',
    'middleware' =>  App\Action\CreateCustomer::class,
    'allowed_methods' => ['POST'],
],

I'm not sure if this is the intention or not. If it isn't a bug like I suspect it is, what is the purpose of the name then?

The specific line is https://github.com/zendframework/zend-expressive-fastroute/blob/master/src/FastRouteRouter.php#L236

@weierophinney
Copy link
Member

I'll see if I can create a reproduce case, and get back to you.

@weierophinney
Copy link
Member

Verified; I'll get a fix in shortly.

@weierophinney weierophinney added this to the 1.0.1 milestone Dec 14, 2015
@weierophinney weierophinney self-assigned this Dec 14, 2015
@nesl247
Copy link
Author

nesl247 commented Dec 14, 2015

Awesome! Glad to see you get to this so quickly. Just wrote some code and came across this and it was holding me back, so this is great.

Updates `marshalMatchedRoute()` to return the matched route from its
internal `array_reduce()` operation, which ensures that we can marshal
`RouteResult` with the route *name* instead of the *path*.
@@ -154,7 +154,7 @@ public function testMatchingRouteShouldReturnSuccessfulRouteResult()
$result = $router->match($request->reveal());
$this->assertInstanceOf(RouteResult::class, $result);
$this->assertTrue($result->isSuccess());
$this->assertEquals('/foo', $result->getMatchedRouteName());
$this->assertEquals('/foo^GET', $result->getMatchedRouteName());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A note to anybody who stumbles across this: by fixing the issue, I exposed a problem with this particular test. The assumption was that the route name was simply the path. However, that's not the case, nor has it been, for most of the lifetime of this implementation; in order to distinguish between middleware using the same path but different HTTP methods, we had to append the method information auto-magically. Updating the code to return the name picked that up, and hence the change to this assertion.

@weierophinney weierophinney merged commit f84c436 into zendframework:master Dec 14, 2015
weierophinney added a commit that referenced this pull request Dec 14, 2015
weierophinney added a commit that referenced this pull request Dec 14, 2015
weierophinney added a commit that referenced this pull request Dec 14, 2015
@weierophinney weierophinney deleted the hotfix/3 branch December 14, 2015 23:15
@weierophinney
Copy link
Member

@nesl247 Fixed, and a new version, 1.0.1, is released with the fix; thanks for the report!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants