Add request_parse_body() function

[iluuu1994]

This function allows populating the $_POST and $_FILES globals for non-post requests. This avoids manual parsing of RFC1867 requests.

Fixes #55815

https://wiki.php.net/rfc/rfc1867-non-post

[KapitanOczywisty]

What happens when you call populate_post_data multiple times, or when $_POST/$_FILES are already populated?

Also I could see this as ini setting e.g. PHP_INI_PERDIR populate_post_custom_methods=PUT,PATCH, which would be consistent in when in request lifecycle data are populated between POST and PUT methods.

[iluuu1994]

What happens when you call populate_post_data multiple times, or when $_POST/$_FILES are already populated?

The previous values are overwritten. Uploaded files have another separate storage (SG(rfc1867_uploaded_files)) so that they are freed even if no longer present in $_FILES.

Also I could see this as ini setting e.g. PHP_INI_PERDIR populate_post_custom_methods=PUT,PATCH, which would be consistent in when in request lifecycle data are populated between POST and PUT methods.

Not sure if you've seen, but https://externals.io/message/120641 contains my full analysis.

[KapitanOczywisty]

Yes, I've read that. I'll rephrase: I'd add ini setting to be default way to populate $_POST/$_FILES for custom HTTP methods. If user cannot use this setting (which should be rare with PHP_INI_PERDIR), they can use function to manually populate these variables in later time.

Function can be easily called multiple times, which could be hard to realize. INI option wouldn't have these problems, would work exactly like POST and in most cases could be used without problems - in others there would be a function.

[bukka]

I think INI would be better here as it is more consistent with other setting and flow. I don't really see why function would be better even after reading that analysis.

[iluuu1994]

A framework couldn't make use of this functionality without the user explicitly enabling this in the config. Furthermore, some hosting scenarios don't allow for ini setting. There's also the possibility of old code parsing the content manually and breaking if this setting is enabled. A function can keep old code working, while making it possible to use it for new code.

[bukka]

Ok I see some use case for the function but I still think it would be also good to have an ini for this in addition to the function as in most cases it suffices and it would be consistent with the current setup.

If I read the current code correctly it looks to me, it resets the vars which I don't think is ideal as in most cases there is no point to reset it. That said there might be some use cases for that so I think there should be an optional parameter in the function to select if it should be overwritten and preferably not overwrite it by default.

[iluuu1994]

@bukka Thank you. I will consider this.

If I read the current code correctly it looks to me, it resets the vars which I don't think is ideal as in most cases there is no point to reset it. That said there might be some use cases for that so I think there should be an optional parameter in the function to select if it should be overwritten and preferably not overwrite it by default.

For the "normal" use-case the array has to be re-initialized because it contains zend_empty_array which is a static, immutable value. Do you see a use-case where $_POST would already be populated? I think this is just the case if the request is POST, or if the user mutates the global manually. For the former, calling the function is wrong as the content was already consumed and nothing is repopulated. For the latter, I guess merging the values instead of overwriting could be desired. But I'm not sure if that's common practice.

[dstogov]

I think we should avoid SAPI interface changes.

[iluuu1994]

@dstogov I avoided the SAPI API changes by backing up and restoring the $_POST and $_FILES globals. The existing API is quite convoluted, but I think it's better to improve this separately, after the RFC has been voted on (either with or without these changes, depending on the voting result).

[dstogov]

@dstogov I avoided the SAPI API changes by backing up and restoring the $_POST and $_FILES globals. The existing API is quite convoluted, but I think it's better to improve this separately, after the RFC has been voted on (either with or without these changes, depending on the voting result).

Sorry for the noise. Really that API change wasn't a big problem. I thought, it was a change in _sapi_module_struct. My mistake.

[iluuu1994]

@dstogov It's ok, I think it's better not to change the API too much without a clear strategy on how to improve it in the future. The C changes are now quite straight forward. Since this code doesn't change frequently the need to improve it is not super high.

[bukka]

I just went through this properly and it looks all good to me. Just some few really minor comments but couldn't really find anything else which is good.