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

Add failing test for encoded routes with a query #6568

Closed
wants to merge 3 commits into from
Closed

Add failing test for encoded routes with a query #6568

wants to merge 3 commits into from

Conversation

PhilGrayson
Copy link

A route like /this%2FAthat with a query string is being assembled
wrongly to /this/that?foo=bar

weierophinney and others added 2 commits August 12, 2014 11:51
A route like /this%2FAthat with a query string is being assembled
wrongly to /this/that?foo=bar
@Ocramius
Copy link
Member

Related: #6566

@rhysr
Copy link

rhysr commented Aug 16, 2014

testAssembleWithEncodedPath is working by accident.
This elseif statement will never execute for urls without query params because $uri->isValidateRelative() uses the path as part of it's checking, which isn't set yet. If it can't find a path, which it doesn't have, it checks for query params, which in the case of testAssembleWithEncodedPath it doesn't have, making the code just return the raw path

testAssembleWithEncodedPathAndQueryParams has query params, causing $uri->isValidateRelative() to return true and execute the normalize method on the $path.

So, it looks like a fix is needed to add the path to the $uri object before the calls to $uri->isValidRelative(), but that will also make both tests fail which is at least consistent, but annoying.

@PhilGrayson
Copy link
Author

I've added a backwards compatible fix for this issue. Setting normalize_path option to false bypasses the normalize call on the URI object.

A more consistent fix would be only run normalize if normalize_path option is true.

@weierophinney
Copy link
Member

Noting for others reviewing: this is not just the failing tests, but a patch, and, despite what Travis says, the specific tests pass.

@weierophinney weierophinney added this to the 2.4.0 milestone Feb 19, 2015
weierophinney added a commit that referenced this pull request Feb 19, 2015
Add failing test for encoded routes with a query
weierophinney added a commit that referenced this pull request Feb 19, 2015
@weierophinney
Copy link
Member

Merged to develop for release with 2.4.0; @PhilGrayson , please document the new option in the docs: https://github.com/zendframework/zf2-documentation

Thanks!

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

Successfully merging this pull request may close these issues.

5 participants