Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add routing params to request query params for middleware usage #250

Merged
merged 10 commits into from
May 18, 2020

Conversation

delboy1978uk
Copy link
Contributor

As per issue here #249

@odan
Copy link

odan commented Aug 27, 2019

From my point of view: Routing args and a query string is conceptionally someting different and should not be mixed.

Mixing the query string arguments (?id=1) and the route vars (/users/{id:[0-9]+}) could cause some naming conflicts and could break working code.

@delboy1978uk
Copy link
Contributor Author

A routing var as you put it, is just a pretty URL. So in most folks eyes I think they would agree that this is indeed a query parameter.
The only example (using your example) I can think would break the code would be
/users/5?id=6
But I doubt anyone would do that.

@delboy1978uk
Copy link
Contributor Author

I forgot to tag this issue in the pull request #249. You can't get any routing params in any middleware, so we do need this!

@delboy1978uk
Copy link
Contributor Author

delboy1978uk commented Aug 27, 2019

Just had a discussion in ##php and it has been suggested than the route vars get put into PSR-7 ServerRequestInterface attributes. https://github.com/php-fig/http-message/blob/master/src/ServerRequestInterface.php#L202:L204 The docblock itself says:
The request "attributes" may be used to allow injection of any parameters derived from the request: e.g. the results of path match operations, and that sounds exactly like what we need. I'll work on refactoring the array merge out and injecting route vars as attributes instead. Thanks for your input @odan :-) And thanks to @Bittarman for helping find the correct solution.

@philipobenito
Copy link
Member

sorry for the delay on this, looking at a release the first week in the new year so will look at bringing this in for that

@jan-di
Copy link

jan-di commented Feb 5, 2020

Hello @philipobenito, is there anything how we can help to get this pull request merged? I would like to use route specific vars in a Middleware and i think this is the one big missing part of this router, to make it completly middleware ready 🙂
Thanks

@philipobenito
Copy link
Member

philipobenito commented Feb 5, 2020 via email

@@ -19,7 +19,7 @@ class DispatchIntegrationTest extends TestCase
*
* @return void
*/
public function testDispatchesFoundRoute(): void
/* public function testDispatchesFoundRoute(): void
Copy link
Member

Choose a reason for hiding this comment

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

why is this commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh! let me sort that just now!! apologies

* @param Route $route
* @return ServerRequestInterface
*/
private function requestWithRouteAttributes(ServerRequestInterface $request, Route $route): ServerRequestInterface
Copy link
Member

Choose a reason for hiding this comment

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

protected please

break;
}

return $this->handle($request);
}

/**
* @param ServerRequestInterface $request
Copy link
Member

Choose a reason for hiding this comment

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

Please format doc comments in line with others

@philipobenito
Copy link
Member

I'll look at getting these merged and released at some point today or tomorrow 👍

@philipobenito
Copy link
Member

philipobenito commented May 13, 2020 via email

@philipobenito philipobenito merged commit 1145d38 into thephpleague:master May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants