Skip to content
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

Bugfix - Make sure readStream actually reads a stream #211

Merged

Conversation

tstrijdhorst
Copy link

Problem

By default readStream does not stream data from S3 itself but it will download the whole file into memory and pass a stream resource to that data in local memory. If you want to stream data directly from S3 you will need to pass (a strangely undocumented) option in the constructor as per suggestion here: https://github.com/thephpleague/flysystem-aws-s3-v3/issues/124#issuecomment-330589497

However if this option is passed via the constructor then writeStream will fail with an exception as described here: https://github.com/thephpleague/flysystem-aws-s3-v3/issues/209

Proposed Solution

By default always assume that readStream means that the client wants to stream directly from S3 and always pass the aforementioned option when calling the S3 sdk. The order is done in such a way that setting this option via the constructor will have precedence over the default.

Tim Strijdhorst added 3 commits August 17, 2020 13:41
Leave the option overwritable by the options array passed in the
constructor.
src/AwsS3Adapter.php Outdated Show resolved Hide resolved
Tim Strijdhorst added 2 commits August 17, 2020 14:49
  prevent BC breaking change since protected methods are part of the
  (semi) public interface of this class
@frankdejonge
Copy link
Member

@tstrijdhorst could you have a look at the failing tests? If not, I can look at it tonight.

@tstrijdhorst
Copy link
Author

@tstrijdhorst could you have a look at the failing tests? If not, I can look at it tonight.

I spend an hour stepping through the tests but I cannot find where the problem is. The code itself works when I use it in my project but the tests fail locally as well.

I'm not exactly sure what it is but I notice that if i take out the ['@http' => ['stream' => true]] from the constructor in this test, the same error will be thrown:
https://github.com/Respondens/flysystem-aws-s3-v3/blob/48d71b45b2280dcc0e7ff9a817f15d5df2606ce0/spec/AwsS3AdapterSpec.php#L581-L606

The problem seems to be with the tests / mocks itself but I have no experience with phpspec so that makes debugging it a bit hard. For instance this $command that is injected, where does it come from?

Also the difference between these two tests is not entirely clear to me:
https://github.com/Respondens/flysystem-aws-s3-v3/blob/48d71b45b2280dcc0e7ff9a817f15d5df2606ce0/spec/AwsS3AdapterSpec.php#L111-L119

https://github.com/Respondens/flysystem-aws-s3-v3/blob/48d71b45b2280dcc0e7ff9a817f15d5df2606ce0/spec/AwsS3AdapterSpec.php#L581-L606

@tstrijdhorst
Copy link
Author

Had an epiphany in bed yesterday, thx Rich Hickey for hammock driven design 🏖️ 😛

Tests are all fixed now.

@frankdejonge
Copy link
Member

Awesome, thank you @tstrijdhorst! I'll tag a release tonight.

@frankdejonge frankdejonge merged commit b01017c into thephpleague:master Aug 18, 2020
@frankdejonge
Copy link
Member

@tstrijdhorst Found some time during my lunch break, version 1.0..26 is all yours! 👍

@tstrijdhorst
Copy link
Author

@frankdejonge thanks for the swift and straight forward communication 🙏

@tstrijdhorst tstrijdhorst deleted the feature/actual-stream-reading branch August 18, 2020 11:04
@boboldehampsink
Copy link

This is a breaking change! fseek stopped working because of this change fseek(): stream does not support seeking and I use this everywhere I use this library

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants