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

ChainRouter: propagate match() call instead of mocking Request and calling matchRequest() #152

Closed

Conversation

sustmi
Copy link

@sustmi sustmi commented Sep 30, 2015

Deprecates PR #147

@dbu
Copy link
Member

dbu commented Sep 30, 2015

thanks for this pull request. i am however afraid that this could lead to unexpected behaviour changes. see for example #151.

@sustmi @ pk16011990 is the problem you encounter not solved when you use the matchRequest method with a request that you create correctly?

@sustmi
Copy link
Author

sustmi commented Sep 30, 2015

@dbu , OK lets forget about fixing propagating match().
Still, I insist that the bug is in ChainRouter and not Symfony.

UrlMatcherInterface::match() takes pathinfo as parameter and not URL.
You should not pass pathinfo into Request::create(), because it expects URI.
According to RFC-3986 URI //g means g is hostname ie. pathinfo is /.

Request::create() is correct, but ChainRouter is not using it right.

These commits try to fix the behaviour: https://github.com/sustmi/Routing/compare/master...fix-matching-pathinfo?expand=1

@lsmith77
Copy link
Member

lsmith77 commented Oct 8, 2015

maybe a safer approach would be to set a "dummy host" in this edge case to prevent the // being misinterpreted?

@dbu
Copy link
Member

dbu commented Nov 4, 2015

@sustmi okay, sorry for letting this sit around so long. i propose we add handling for the specific case of path starting with // and in that case add //host from $_REQUEST if set, //localhost otherwise. that should avoid the edge case, right? can you do a pull request @sustmi ?

@sustmi
Copy link
Author

sustmi commented Nov 4, 2015

I don't understand. If you have Request instance, you don't need to fake it. Or do you really mean $_REQUEST superglobal?

@dbu
Copy link
Member

dbu commented Nov 4, 2015 via email

@dbu
Copy link
Member

dbu commented Jan 2, 2016

superseeded by #163

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

3 participants