search

zendframework / zend-http

Http component from Zend Framework
BSD 3-Clause "New" or "Revised" License
134 stars 85 forks source link

Proper URI rendering for Zend/Http #68

Open GeeH opened 8 years ago

GeeH commented 8 years ago

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:

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.

weierophinney commented 4 years ago

This repository has been closed and moved to laminas/laminas-http; a new issue has been opened at https://github.com/laminas/laminas-http/issues/15.