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

Check if stream is seekable when downloading object to path #193

Closed
wants to merge 1 commit into from

Conversation

gedimin45
Copy link
Contributor

The destination path can also be php://output, then the file is streamed directly to output. However, that is not seekable and an exception is thrown.
Perhaps the method name is not accurate anymore then - something like downloadToPath might be more appropriate.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@googlebot googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Oct 7, 2016
@gedimin45
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Oct 7, 2016
@jdpedrie
Copy link
Contributor

jdpedrie commented Oct 7, 2016

Thanks for the PR! I'll leave the question of the method name to @dwsupplee as he's the lead on our Storage implementation, but if you don't mind, would you please add a test to Google\Cloud\Tests\Storage\StorageObjectTest showing the correct behavior when streaming to php://output?

You can capture and assert against the output using ob_start() and ob_get_clean(). :)

You should be fine copying most of the body of Google\Cloud\Tests\Storage\StorageObject::testDownloadsToFile() and updating the assertion as necessary.

If you're not familiar with PHPUnit or have any questions please let us know. One of us would be happy to assist.

@jdpedrie jdpedrie added the api: storage Issues related to the Cloud Storage API. label Oct 7, 2016
@dwsupplee
Copy link
Contributor

Thanks for the PR - downloadToPath sounds a like solid suggestion to me. @Ged15 would you like to include that change in this PR as well? :)

@gedimin45
Copy link
Contributor Author

@jdpedrie @dwsupplee please take a look at #199. I believe it introduces a more flexible approach in retrieving StorageObject contents.

@dwsupplee
Copy link
Contributor

I really like the direction in #199. Mind if I close this out @Ged15?

@gedimin45
Copy link
Contributor Author

I can do it as well :)

@gedimin45 gedimin45 closed this Oct 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the Cloud Storage API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants