Skip to content

Implemented FR #51634: Can't post multiple fields with the same name#8292

Closed
derickr wants to merge 1 commit into
masterfrom
bug51634-curl-postfields-multi-value
Closed

Implemented FR #51634: Can't post multiple fields with the same name#8292
derickr wants to merge 1 commit into
masterfrom
bug51634-curl-postfields-multi-value

Conversation

@derickr

@derickr derickr commented Apr 1, 2022

Copy link
Copy Markdown
Member

This PR allows for multiple values with the same field name to be sent to an
HTTP server. This server can't be PHP, because PHP's POST processing does not
allow for two the same field names. But sending multiple fields with the same
name is required for some operations by RFC 7578.

This PR allows you to attach an array of values to a field name, and each of
these will then be sent as its own distinct multipart/form-data field.

This PR allows for multiple values with the same field name to be sent to an
HTTP server. This server can't be PHP, because PHP's POST processing does not
allow for two the same field names. But sending multiple fields with the same
name is required for some operations by RFC 7578.

This PR allows you to attach an array of values to a field name, and each of
these will then be sent as its own distinct multipart/form-data field.
Comment thread ext/curl/tests/curl_postfields_array.phpt
]
];

$options = array(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any reason not to use the short array syntax [] here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not really, I probably copied it from the original example. Will fix.

Comment thread ext/curl/interface.c
}
#endif
zend_tmp_string_release(tmp_postval);
add_simple_field(mime, string_key, current);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: An else-case and removing the continue could remove the duplicate zend_string_release_ex.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, but IMO the current code is more readable, as each special block now looks the same. If we'd to add one in the future, then it's less likely the wrong thing gets copied. (I'm also allergic to else).

@derickr derickr closed this Apr 7, 2022
@derickr
derickr deleted the bug51634-curl-postfields-multi-value branch April 7, 2022 09:00
@derickr

derickr commented Apr 7, 2022

Copy link
Copy Markdown
Member Author

(I merged this manually)

@sebastianbergmann

Copy link
Copy Markdown
Contributor

Will this fix be backported to all currently supported branches or will it only be in PHP >= 8.2?

@cmb69

cmb69 commented Apr 7, 2022

Copy link
Copy Markdown
Member

This only targets the master branch (PHP 8.2). A backport might be considered, but feature requests usually don't get backported to stable versions.

@sebastianbergmann

Copy link
Copy Markdown
Contributor

feature requests usually don't get backported to stable versions

Of course. But as far as I understand the issue, this (c|sh)ould be considered as bug fix and not as a new feature.

What do the release managers of PHP 8.0 and PHP 8.1 think about this?

CC @sgolemon @carusogabriel @ramsey @patrickallaert

@tremby

tremby commented Apr 12, 2022

Copy link
Copy Markdown

Wow 12 years almost to the day since I opened that ticket!

@SjonHortensius

Copy link
Copy Markdown
Contributor

I guess you're not seeing notifications of comments on commits so I'll repost this here:


This commit introduces a dependency on curl 7.56.0 @derickr - curl_mime isn't available

ext/curl/interface.c:2064:41: error: unknown type name 'curl_mime'
 static inline CURLcode add_simple_field(curl_mime *mime, zend_string *string_key, zval *current)

@cmb69

cmb69 commented Apr 29, 2022

Copy link
Copy Markdown
Member

This commit introduces a dependency on curl 7.56.0 @derickr - curl_mime isn't available

Fortunately, this has only been merged into the "master" branch; but still this needs to fixed. Currently, "master" requires libcurl ≥ 7.29.0; lifting a bit might be okay, but 7.56.0 (released 2017-10-04) is likely too much

@derickr

derickr commented Apr 29, 2022

Copy link
Copy Markdown
Member Author

I don't accept that software released for five years by the time PHP 8.2 comes out is "too new". If distributions bundle that new a version of PHP, then they should also have updated their libcurl.

But I should bump the version requirement so that it prevents you from getting a compile error.

@cmb69

cmb69 commented Apr 29, 2022

Copy link
Copy Markdown
Member

But I should bump the version requirement so that it prevents you from getting a compile error.

Well, so far our stance was to be rather conservative regarding version requirements of libcurl and other libraries. I think bumping to 7.56.0 should at least be discussed on the internals mailing list. And if we bump, we likely can simplify other parts of ext/curl.

@SjonHortensius

Copy link
Copy Markdown
Contributor

personally I'm fine with whatever requirement you'd want but I'd noticed the careful coding using LIBCURL_VERSION_NUM which is useless in its current form

@sebastianbergmann

Copy link
Copy Markdown
Contributor

I don't accept that software released for five years by the time PHP 8.2 comes out is "too new".

I second that emotion.

@derickr

derickr commented Apr 29, 2022

Copy link
Copy Markdown
Member Author

@SjonHortensius Yes, but it is nearly impossible to still find a place that has that old of a curl version ;-) In any case: #8460 should fix it.

@cmb69

cmb69 commented Apr 29, 2022

Copy link
Copy Markdown
Member

[…] it is nearly impossible to still find a place that has that old of a curl version

https://curl.se/download/ :)


SingleValue
--------------------------%s
Content-Disposition: form-data; name="multi"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@derickr Hi! Sorry to bother you. I am trying to do the same request with PHP 8.2.9, which doesn't work as expected. What I get is that names are overwritten. So, from this particular test, the receiver will only get Multi2 value. Shouldn't the name be name="multi[]" ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No, multi is correct; but like the feature description states:

This server can't be PHP, because PHP's POST processing does not allow for two[sic] the same field names.

@ghnp5

ghnp5 commented Sep 10, 2024

Copy link
Copy Markdown

@derickr - Hey! Does this allow us to send multiple files using the same key too, like "file[]" ?

I'm using PHP 8.3, but I'm not succeeding in doing this:

$headers = [
	'Content-Type: multipart/form-data'
];

curl_setopt($ch, CURLOPT_HTTPHEADER, $headers);
curl_setopt($ch, CURLOPT_POSTFIELDS, [
	'file' => [
		new CURLFile($file1, posted_filename: 'File1.jpg'),
		new CURLFile($file2, posted_filename: 'File2.jpg')
	]
]);

PHP Fatal error: Uncaught Error: Object of class CURLFile could not be converted to string

Thanks!

@cmb69

cmb69 commented Sep 12, 2024

Copy link
Copy Markdown
Member

Does this allow us to send multiple files using the same key too, like "file[]" ?

No, this is not (yet) supported. Feel free to file a feature request at https://github.com/php/php-src/issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants