zendframework / zend-diactoros

PSR-7 HTTP Message implementation
BSD 3-Clause "New" or "Revised" License
1.55k stars 152 forks source link

UriInterface string / NULL handling #80

Closed kaiwa closed 9 years ago

kaiwa commented 9 years ago

After a discussion (https://github.com/asika32764/http/issues/1) how to handle NULL values given to UriInterface::withPath(), UriInterface::withQuery() and UriInterface::withFragment(), @asika32764 noticed that it is not consistently implemented in zend-diactoros (ex phly/http).

The psr interface (https://github.com/php-fig/http-message/blob/master/src/UriInterface.php) docblocks define

@param string $query The query string to use with the new instance.
@param string $path The path to use with the new instance.
@param string $fragment The fragment to use with the new instance.

In zend-diactoros Uri::withQuery() (https://github.com/phly/http/blob/master/src/Uri.php#L338) and Uri::withPath() (https://github.com/zendframework/zend-diactoros/blob/master/src/Uri.php#L310) require string arguments and throw exceptions when NULL is passed as an argument, while Uri::withFragment() converts NULL to string explicitly (https://github.com/phly/http/blob/master/src/Uri.php#L565).

So I wonder what is correct, to require strings only or to allow also NULL?

It seems a bit meticulous but that actually broke the interoperability of two psr7 implementations (https://github.com/asika32764/http and http://github.com/guzzle/psr7, the latter is using phly/http's Uri class).

asika32764 commented 9 years ago

Yes, I'm wonder it too. If there are any conclusions, I'll modify my package to fit it.

asika32764 commented 9 years ago

The withHost() is also allow NULL but the docblock only use string as argument type.

weierophinney commented 9 years ago

The appropriate approach is to throw an exception for any non-string arguments. I'll work up a PR shortly.

Once complete, I would suggest opening a similar issue on guzzle/psr7 requesting a similar change, and referencing this issue.

weierophinney commented 9 years ago

I've opened #85 to address this issue.