Extend CURLFile to support streams #4034
Conversation
|
This is a follow-up to PR #4032. /cc @weltling, @adoy. PS: see also curl/curl#3675 (comment) |
| if (php_stream_stat(stream, &ssb)) { | ||
| zend_string_release_ex(string_key, 0); | ||
| return FAILURE; | ||
| } |
There was a problem hiding this comment.
It might be worth to support non seekable stream, like URL, which is a completely legit scenario.
Thanks.
There was a problem hiding this comment.
That may not always work (see https://curl.haxx.se/libcurl/c/curl_mime_data_cb.html), but we still could fail in this case.
There was a problem hiding this comment.
Passing -1 for data length should be ok in general and would spare the stat call. Not requiring stream to be seekable and setting the seek function to NULL might be still a fallback. Knowing the stream size ahead is good, but doesn't guarantee we still don't end up with a partial data as it's stream dependent.
Thanks.
There was a problem hiding this comment.
The curl docs are not particularly clear regarding the datasize, but it seems passing -1 works.
Instead of passing NULL as seekfunc I'd suggest to have a callback which tries to seek, and returns CURL_SEEKFUNC_CANTSEEK otherwise. which indicates that "libcurl is free to work around the problem if possible".
There was a problem hiding this comment.
Yep, could be probably an option, too. Thanks.
|
Can you please also add a test where a non-file stream is used? |
|
Non-statable and possibly non-seekable streams support added, as well as a test for data:// |
|
If there are no objections, I'm going to merge this in a few days. |
Do we need to do anything about this (as we expose curl_easy_duphandle)? |
|
@nikic Indeed, thanks! The best idea I came up with so far would be to not register a |
Due to former restrictions of the libcurl API, curl multipart/formdata file uploads supported only proper files. However, as of curl 7.56.0 the new `curl_mime_*()` API is available (and already supported by PHP[1]), which allows us to support arbitrary *seekable* streams, which is generally desirable, and particularly resolves issues with the transparent Unicode and long part support on Windows (see bug #77711). Note that older curl versions are still supported, but CURLFile is still restricted to proper files in this case. [1] <http://git.php.net/?p=php-src.git;a=commit;h=a83b68ba56714bfa06737a61af795460caa4a105>
We pass `-1` as `datasize` to avoid the `stat` call, and let curl handle non-seekable streams.
If the CURL handle is duplicated, the `freefunc` is called multiple times, so we must take care not to close the stream prematurely. Thus, instead of passing a `freefunc` to `curl_mime_data_cb`, we add the streams to a dedicated `zend_llist` which we release when the last duplicated CURL handle is destroyed.
|
I have addressed the duplication of handles issue, and retargeted to PHP-7.4. CI is green. If there are no objections, I'll merge this on Monday. |
|
Applied as c68dc6b. Thanks! |
Due to former restrictions of the libcurl API, curl multipart/formdata
file uploads supported only proper files. However, as of curl 7.56.0
the new
curl_mime_*()API is available (and already supported byPHP[1]), which allows us to support arbitrary seekable streams, which
is generally desirable, and particularly resolves issues with the
transparent Unicode and long part support on Windows (see bug #77711).
Note that older curl versions are still supported, but CURLFile is
still restricted to proper files in this case.
[1] http://git.php.net/?p=php-src.git;a=commit;h=a83b68ba56714bfa06737a61af795460caa4a105