Repository navigation
Conversation
022fc17 to
c8f8de7
Compare
Make it possible to post multiple files with the same name using cURL. It was already possible to post files, and it was possible to post multiple strings with the same name. This moves things around to combine these two features. Closes php#15859
c8f8de7 to
aece997
Compare
Sjord
changed the title
|
@cmb69 could you perhaps review this please? |
Girgias
left a comment
Girgias
left a comment
There was a problem hiding this comment.
Mixing part of the refactoring with the behavioural changes makes it hard to follow along.
Could you maybe first split build_mime_structure_from_hash into multiple functions dealing with the different types in a different PR, and then rebase this PR when that one landed?
|
I agree that the changes view of this PR is a mess. I moved the inner loop from build_mime_structure_from_hash to its own function. There are hardly any other changes. Splitting this up into multilple pull requests is not going to make things clearer, I'm afraid. Could you take a look at this PR with white-space changes ignored? I think that makes it easier to see what's going on. |
|
@Girgias could you take another look at this? |
|
@arnaud-lb Could you take a look at this? |
|
|
||
| if (error != CURLE_OK) { | ||
| zend_string_release_ex(string_key, 0); | ||
| goto out_mime; |
There was a problem hiding this comment.
Before, error was eventually passed to SAVE_CURL_ERROR(ch, error), but now we skip over it.
| } | ||
| } ZEND_HASH_FOREACH_END(); | ||
|
|
||
| SAVE_CURL_ERROR(ch, error); |
There was a problem hiding this comment.
This is now unreachable (except for error = CURLE_OK)
| } | ||
|
|
||
| postval = Z_STR_P(prop); | ||
| return add_simple_field(mime, string_key, current); |
There was a problem hiding this comment.
Nit: Inconsistent indentation on this line (spaces vs tabs)
| static inline zend_result build_mime_structure_from_hash(php_curl *ch, zval *zpostfields) /* {{{ */ | ||
| { | ||
| HashTable *postfields = Z_ARRVAL_P(zpostfields); | ||
| static inline CURLcode add_form_field(php_curl *ch, curl_mime *mime, zend_string *string_key, zval *current, zval *zpostfields) { |
There was a problem hiding this comment.
Nit: please add a newline before {
| CURLOPT_POSTFIELDS with nested arrays | ||
| --EXTENSIONS-- | ||
| curl | ||
| sockets |
There was a problem hiding this comment.
sockets is not used in this test
| echo "TypeError: {$e->getMessage()}\n"; | ||
| } | ||
| ?> | ||
| --EXPECTF-- |
There was a problem hiding this comment.
Nit: can be --EXPECT-- (without F)
| sockets | ||
| --FILE-- | ||
| <?php | ||
| $socket = stream_socket_server("tcp://0.0.0.0:29999", $errno, $errstr); |
There was a problem hiding this comment.
Nit: Please use port 0 + stream_socket_get_name() as in
f01e914 to
6ffa446
Compare
3 participants
#15859