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

Fix for URIs with a query string not matching#3712

Closed
BenjaminNolan wants to merge 2 commits intozendframework:masterfrom
BenjaminNolan:hotfix/ZF-3711
Closed

Fix for URIs with a query string not matching#3712
BenjaminNolan wants to merge 2 commits intozendframework:masterfrom
BenjaminNolan:hotfix/ZF-3711

Conversation

@BenjaminNolan
Copy link
Copy Markdown
Contributor

Fix for #3711 — URIs with query strings no longer match in 2.1, matched fine in 2.0.5.

@weierophinney
Copy link
Copy Markdown
Member

https://travis-ci.org/zendframework/zf2/jobs/4642977/#L632

Your fix introduces a new test failure. It looks like the released behavior is intentional -- but as it introduces a regression, we need to:

  • introduce a test for the original behavior, which you fix here
  • figure out how to resolve the new behavior that was introduced, if possible

I think @SocalNick was involved with the query route functionality; if he wasn't, @DASPRiD can likely remember who. We need one of them to assist with this fix.

@SocalNick
Copy link
Copy Markdown
Contributor

I have inquired about it, but not sure who made it originally. For what it's worth, the opposite change was just added: d4c256a

I noticed that some of our routes weren't behaving after said patch, but we have adjusted accordingly already...

@BenjaminNolan
Copy link
Copy Markdown
Contributor Author

I asked @DASPRiD about it in the IRC channel shortly after I submitted the pull request (and thought I'd also mentioned this here, but apparently not!), and he had no idea who added the Query route functionality (and I believe mentioned that he wouldn't have added it personally).

What is the rationale for adding this behaviour?

Also, I've been trying to guess at the correct way to fix this problem as the reference guide does not seem to have been updated yet, and Zend\Mvc\Router\Http\Query only makes reference to http://guides.rubyonrails.org/routing.html, which is not helpful at all either for explaining why the Query route was added—which isn't Ruby's behaviour, judging by that guide—or for explaining the correct usage of that route, ie. does it replace the previous Segment route when you want to pass through query string parameters, or does it need to be added as a sub-route of each Segment which is intended to accept query string parameter?

And yeesh, is that not the queen of run-on sentences or what?

@weierophinney
Copy link
Copy Markdown
Member

@Syniq I think we should likely fix this -- I've run into the issue myself, but hadn't had a chance to diagnose it. We just need to identify what use case resulted in the change, and see if we can accommodate both the old behavior and the new.

@BenjaminNolan
Copy link
Copy Markdown
Contributor Author

@weierophinney So, the new behaviour is to add in a Query child route to the affected route which has the accepted parameters as defaults. The only way I can think of to check automatically whether the system expects the old behaviour is to scour the router and check for any Query routes. If there aren't any, then you're using the old behaviour. :/ Not particularly graceful, but as I said its the only way I can think of at 10 past 7 on a Friday evening when I'm still at my desk and should have left 2 hours ago. ;)

@cybercandyman
Copy link
Copy Markdown
Contributor

Hi. I do not understand what is wrong with that, i cannot reproduce it.
Can you provide an example please ?

@weierophinney
Copy link
Copy Markdown
Member

Fixed with #3778

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants