-
-
Notifications
You must be signed in to change notification settings - Fork 129
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deactivate chunked body in S3 #911
Conversation
b9b44cb
to
5caa3ad
Compare
if ($contentLength < self::CHUNK_SIZE || !$this->sendChunkedBody) { | ||
if ($body instanceof ReadOnceResultStream) { | ||
$request->setBody(RewindableStream::create($body)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the body is not an instance of ReadOnceResultStream?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$body
is initialized from $body = $request->getBody()
.
If it's not an instance of ReadOnceResultStream
, then we are good, we can rewind the stream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fail to understand why though.. Before, we always converted to a StringStream
if the body was small, but now we dont anymore. Was it a misstake before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I believe the conversation were not needed.
If I remember well, when I implemented the conversion to string, the ReadOnceResultStream
did not exists and stream have to be converted into string at some point.
Now, thanks to the RewindableStream, the signature is able to works with all cases, but I forgot to revert upgrade this statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry if Im extra slow on this PR.
if ($contentLength < self::CHUNK_SIZE || !$this->sendChunkedBody) { | ||
if ($body instanceof ReadOnceResultStream) { | ||
$request->setBody(RewindableStream::create($body)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fail to understand why though.. Before, we always converted to a StringStream
if the body was small, but now we dont anymore. Was it a misstake before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you see why the tests are failing?
last test fails because of doctrine/annotations#398 |
Thank you for the review |
Awesome. Thank you! |
Fixes #874