Add check for empty string to stream factories #70
Conversation
| @@ -24,7 +24,7 @@ public function createStream($body = null) | |||
| } else { | |||
| $stream = new Stream('php://memory', 'rw'); | |||
|
|
|||
| if (null !== $body) { | |||
| if (null !== $body || '' !== $body) { | |||
There was a problem hiding this comment.
Why logical or? We should write to the stream if the body has content.
if (null !== $body && '' !== $body) {
There was a problem hiding this comment.
we could also do if (null === $body || '' === $body) {return new Stream('php://memory', 'rw');} as the rest of the code is irrelevant if the body is empty
There was a problem hiding this comment.
Also this doesn't work if body is an object and casting the object to string return an empty value. We should do the following :
$content = (string) $body;
if (strlen($content) > 0) {
$stream->write($content);
}
return $stream
There was a problem hiding this comment.
@dbu indeed, an empty stream does not have to be rewinded, or does it?
@joelwurtz According to the interface the allowed types are string|resource|StreamInterface|null, so an object would not be valid in the first place, you would have to cast it to string first.
| @@ -28,7 +28,7 @@ public function createStream($body = null) | |||
| $resource = fopen('php://memory', 'r+'); | |||
| $stream = new Stream($resource); | |||
|
|
|||
| if (null !== $body) { | |||
| if (null !== $body || '' !== $body) { | |||
yes, but there is no type check to avoid objects. what exactly do we fix with this? is writing '' to a stream a bug? or is it just to be strict? |
|
Well, I guess you can technically pass objects there. There is a string casting before writting.
I don't think it's a bug. But we spare an unnecessary write (and now a rewind) operation. Not sure if adding an extra cast just to check if the string value of something is empty worth it. |
|
if objects are not supposed to be passed, i think we are good here. if you incorrectly pass an object, the behaviour does not change compared to previous, and should still not fail. |
What's in this PR?
Stream factories now check empty strings as well, not just null values.