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

[6.x] Allow Storage::put to accept a Psr StreamInterface #30179

Merged
merged 3 commits into from
Oct 8, 2019

Conversation

Gman98ish
Copy link
Contributor

Addresses: laravel/ideas#1252

This PR will allow the Storage::put method to accept a Psr StreamInterface. This will mostly be useful when dealing with guzzle, as you will no longer have to manually call detach and pass a resource to Storage::put.

This will not break existing features as it's broadening what the method accepts.

@Gman98ish Gman98ish force-pushed the 6.x branch 2 times, most recently from 19bf8ae to 5ff8b6d Compare October 4, 2019 08:19
@Gman98ish Gman98ish changed the title Allow Storage::put to accept a Psr StreamInterface [6.x] Allow Storage::put to accept a Psr StreamInterface Oct 4, 2019
composer.json Outdated Show resolved Hide resolved
Copy link
Member

@GrahamCampbell GrahamCampbell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please order the suggested dependencies alphabetically.

@netpok
Copy link
Contributor

netpok commented Oct 7, 2019

I am not sure about this, because it calls detach behind the scenes, I wouldn't anticipate that a filesystem write command leaves my stream in an unusable state.

Or it should be very well documented.

@taylorotwell
Copy link
Member

@Gman98ish any comments on @netpok's comment?

@Gman98ish
Copy link
Contributor Author

@netpok That's a good point, it could be pretty confusing/irritating. I guess the alternative would be to avoid using detach and instead read the contents of the stream into memory, then use the regular put method?

Obvious problem with that is that you have the whole contents of the stream in memory, which sorta defeats the point of using a stream. I also looked into chunking it without the use of putStream, but it doesn't look like you can consistently append to a file without relying on the config of the flysystem adapter.

I'm happy to drop this if the overall behaviour is too confusing. Or at the very least, come back to it when I have a better solution

@taylorotwell taylorotwell merged commit cfe6b42 into laravel:6.x Oct 8, 2019
@taylorotwell
Copy link
Member

I think I will merge it for now. We were already calling putStream for other types of streams.

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

Successfully merging this pull request may close these issues.

4 participants