Proper URI rendering for Zend/Http
Opened this issue · 1 comments
This issue has been moved from the zendframework repository as part of the bug migration program as outlined here - http://framework.zend.com/blog/2016-04-11-issue-closures.html
Original Issue: https://api.github.com/repos/zendframework/zendframework/issues/7479
User: @afeder
Created On: 2015-05-03T15:26:42Z
Updated At: 2015-06-03T17:28:40Z
Body
Fixes #7472.
Comment
User: @Martin-P
Created On: 2015-05-03T17:32:43Z
Updated At: 2015-05-03T17:32:43Z
Body
This PR needs unittests.
Comment
User: @afeder
Created On: 2015-05-03T17:36:07Z
Updated At: 2015-05-03T17:39:57Z
Body
Does any guidance exist as to what kinds of unit tests would be needed?
Comment
User: @samsonasik
Created On: 2015-05-03T18:48:36Z
Updated At: 2015-05-03T18:48:36Z
Body
need rebase
Comment
User: @afeder
Created On: 2015-05-03T18:59:30Z
Updated At: 2015-05-03T18:59:30Z
Body
samsonasik: Indeed. Done.
Comment
User: @afeder
Created On: 2015-05-03T19:24:02Z
Updated At: 2015-05-03T19:24:02Z
Body
I added a test for the issue described in #7472.
Comment
User: @samsonasik
Created On: 2015-05-08T23:46:11Z
Updated At: 2015-05-08T23:46:11Z
Body
need rebase again
Comment
User: @afeder
Created On: 2015-05-09T00:53:00Z
Updated At: 2015-05-09T00:53:00Z
Body
Rebase done.
Comment
User: @Martin-P
Created On: 2015-05-09T12:05:19Z
Updated At: 2015-05-09T12:05:19Z
Body
IMO this needs PR needs many more tests.
There are multiple new methods introduced in Zend\Http\Request which are not tested at all:
setOptionssetArgSeparatorgetArgSeparatorrenderQueryStringrenderUri
A big chunk of code in Zend\Http\Client has been removed and new code is added, but no tests have been added for this.
The only test which has been added is a very basic query string example. What happens with more advanced query strings? Does that work too or will it fail?
Comment
User: @afeder
Created On: 2015-05-09T15:52:06Z
Updated At: 2015-05-09T16:03:00Z
Body
If you share with me what tests you are looking for I shall add them right away.
The big chunks of code being removed or added is little more than migrating the URI building logic from the Client class into the Request class.
I've found two existing tests pertaining to this code which I've now copied over to the tests for the Request class. I've also expanded the query test to cover the case of multiple query parameters being set, and duplicated this test for the renderUri() and renderQueryString() functions.
Comment
User: @afeder
Created On: 2015-05-12T02:13:32Z
Updated At: 2015-05-12T18:21:12Z
Body
I fixed the exception in the setOptions() function per sasezaki's comment. As for testing the function, none exist for the original code in Client.php, but I added a basic one anyway.
This repository has been closed and moved to laminas/laminas-http; a new issue has been opened at laminas/laminas-http#15.