Skip to content

Conversation

blizzz
Copy link
Contributor

@blizzz blizzz commented Mar 30, 2017

…with file download example.

I was looking for a way to download files as stream (e.g. to avoid having the contents of a large file in memory). Tampering with OpenBinaryStream did not work, somehow, I always got the file content.

Instead, it works when specifying the CURLOPT_FILE options (and the usual $value endpoint). This PR provides a method to insert curl options to RequestOptions.

Another thing that is a bit related but I left untouched for now are the insecure default curl options. A more central place to change them for any request would be pretty nice.

…with file download example
@oparoz
Copy link

oparoz commented Apr 3, 2017

👍

@blizzz
Copy link
Contributor Author

blizzz commented Apr 11, 2017

@vgrem could you have a look, please?

Copy link
Owner

@vgrem vgrem left a comment

Choose a reason for hiding this comment

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

Greetings!

Thank you for the idea and PR!

Just one question, do we really need to expose curl properties via addCurlOption function.

If it just a matter of setting stream via CURLOPT_FILE option, may be we could introduce a method lets say:

public function setStream($handle)
{
    $this->streamHandle = $handle;
}

@blizzz
Copy link
Contributor Author

blizzz commented Apr 12, 2017

@vgrem heyo, thanks for getting back to me :)

In my specific case CURLOPT_FILE is what I need, yes. My concern was that limiting a method to this is too narrow, and the next use case for another option might be just around the corner. OTOH, I just realize, following your suggestion would fit better into the style of RequestOptions. Furthermore, just a public property $StreamHandle would be already sufficient. I can adjust the PR accordingly.

@vgrem
Copy link
Owner

vgrem commented Apr 12, 2017

@blizzz, that would be great, appreciate it! Once it is ready, i will commit PR.

Regarding
My concern was that limiting a method to this is too narrow, and the next use case for another option might be just around the corner
you might be right, i was thinking that for exposing curl properties, RequestOptions class might be not the best candidate for that matter.

P.S. Sorry it took me so long to get back to you :)

@blizzz
Copy link
Contributor Author

blizzz commented Apr 12, 2017

@vgrem changes committed!

you might be right, i was thinking that for exposing curl properties, RequestOptionsclass might be not the best candidate for that matter.

Valid point!

P.S. Sorry it took me so long to get back to you :)

No prob :)

@vgrem vgrem merged commit 9d6c58a into vgrem:master Apr 12, 2017
@blizzz blizzz deleted the curl-options branch April 12, 2017 12:58
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.

3 participants