-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Add support for upload files from buffer string in curl extenion #1283
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
Conversation
@Irker 👍 |
Hi! |
@@ -29,35 +29,41 @@ | |||
|
|||
PHP_CURL_API zend_class_entry *curl_CURLFile_class; | |||
|
|||
static void curlfile_ctor(INTERNAL_FUNCTION_PARAMETERS) | |||
static void curlfile_ctor(char *fname, size_t fname_len, char *mime, size_t mime_len, char *postname, size_t postname_len, char *buffer, size_t buffer_len, INTERNAL_FUNCTION_PARAMETERS) | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you need INTERNAL_FUNCTION_PARAMETERS? You don't seem to use them except for return_value. So maybe just pass return_value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, will change it.
if (Z_TYPE_P(prop) == IS_STRING && Z_STRLEN_P(prop) > 0) { | ||
filename = Z_STRVAL_P(prop); | ||
prop = zend_read_property(curl_CURLFile_class, current, "name", sizeof("name")-1, 0, &rv); | ||
if (zval_is_true(prop)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I understand why zval_is_true check? If you wanted to check for name being empty/null, then it should be that check. Otherwise you'll get in trouble with "0" etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smalyshev , can it be something like if (!Z_ISNULL_P(prop) && (Z_TYPE_P(prop) != IS_STRING || Z_STRLEN_P(prop) > 0)) {
? Not sure if prop can be ISUNDEF
after zend_read_property. Can you correct me, before I will push this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure it can be UNDEF, but I'd rather specify explicitly which values we do expect - i.e. if we expect name to be NULL when we use buffer.
Also, whatever check you do here you should also do on __wakeup. In fact, I'd make it a macro or inline function to ensure they are always the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we will use NULL as base value for this condition, we must change this code: zend_declare_property_string(curl_CURLFile_class, "name", sizeof("name")-1, "", ZEND_ACC_PUBLIC);
to zend_declare_property_null. This is small but BC break =(, because we change default value for property.
Previous variant of pull-request with separate CURLBufferFile seems now less terrible )))
Not interested for the community |
@smalyshev is any chance to implement sending files from buffer string?
|
@Irker I'll look into it. |
@smalyshev I do not want to disturb, but I think it's time to remind. |
Add possible to upload files from string, not from disc.
New function curl_buffer_file_create, similar for curl_file_create, but accept other arguments:
$buffer - file content, $post_filename and $mime_type.
Also CURLFile constructor now haven't required arguments. So we can fill file object later:
For BC I save unserialization restriction: if $filename contain string - we cannot unserialize object as before this pull-request. If we have object with buffer unserialization accepted.
If object have $filename we use it and ignore buffer.
$buffer can be empty.