[RFC][Under discussion] RFC1867 for non-POST HTTP verbs

Hi Tim

php://temp would solve auto-cleanup of files nicely. However, whether they are easier to work with will depend on what you're doing with the file. The most common action after a file uploads is arguably to move it to a permanent location using move_uploaded_file(). With a stream the obvious way to achieve the same is stream_copy_to_stream(). However, as the stream already has a file backing (if big enough, at least) this copy is unnecessary. Please correct me if there's something I have missed. I also would really like to avoid subtle differences between the automatically and manually invoked files. Given that the overwhelming majority will not use PHP with something like RoadRunner, I think it makes more sense to add the special casing (i.e. deleting the files manually) in the uncommon case, than for everybody else to adapt their code for the uncommon case. Ilija

On Fri, 6 Oct 2023, Tim Düsterhus wrote:

I don't think that reading potentially large files into memory is a great idea. It is precisely because of the memory issue that PHP's existing POST handler writes files to disk, instead of leaving their content available through an entry in $_FILES. cheers, Derick

On 10/6/23 11:18, Jakub Zelenka wrote:

Maybe it should go in main/rfc1867.c? That's where the php_rfc1867_callback lives, and this seems somewhat related to that. Cheers, Ben

Hi Jakub

Indeed, I will mention that. Thank you.

TBH I have no idea how it landed there. When I created the PoC I just threw it in somewhere, but it's indeed an odd place. I'll try to find something better. I don't think stream is a great place, as the common case of not providing an input does not operate on streams, but on sapi_module.read_post() directly. I also don't think rfc1867.c is a great place as Ben suggested, because the invoked function is actually agnostic to the exact format. Instead, we use the existing functionality that chooses the parser based on the content type, which includes application/x-www-form-urlencoded. Ilija

Hi On 10/6/23 18:18, Jakub Zelenka wrote:

That reminds me of this thread: https://externals.io/message/118614 I'd love to see some functionality to handle those "input limits" in a well-defined way (e.g. to be able to render a nice error page within my application), but this is currently not (easily) possible, because the error is emitted before my script boots. I'm currently using the error_get_last() workaround mentioned in that thread, but that is everything but great. Best regards Tim Düsterhus

Hi Ilija, pt., 13 paź 2023, 13:15 użytkownik Ilija Tovilo <tovilo.ilija@gmail.com> napisał:

Considering the function supports two formats multipart/form-data or the application/x-www-form-urlencodedand the fact that the return type and required arguments differ in regards to format: content-type needed for multipart which returns two arrays packed into return array aka $_POST and $_FILES and the boundary that is part of content-type header and the other format that doesn't require content-type header and returns effectively only one array aka $_POST why not having two separate functions? The proposal is to include two separate functions that have clear semantics and prevent from invoking multipart without boundary or only form with boundary: * request_parse_body($input_stream): array - returning just $_POST and maybe * request_parse_multupart_body($input_stream, string $boundary): array returning as proposed now two arrays packed $_POST and $_FILES Why the $inpit_stream accepts null - does it make sense to invoke the function then?

On Fri, 13 Oct 2023, Ilija Tovilo wrote:

…

The only comment I would have is that I probably would be in favour of not leaving the "config" argument (to over ride per call the post_max_size settings etc) to a future scope. Having to do ini_get/ini_set/ini_set(old) to override these settings seems clunky. cheers, Derick

On Fri, Oct 13, 2023, at 11:15 AM, Ilija Tovilo wrote:

The functionality all seems reasonable to me. I have a few smaller concerns: 1. Like Derick, I think I'd favor including the config overrides now. 2. Lots of request bodies are not forms these days; they're frequently JSON or GraphQL. This function would be useless in those cases; that's fine, but should the name then suggest that it's for form data only? request_parse_form() or similar? I'm just concerned about misleading people into thinking it can parse their JSON bodies, when that's not a thing. (Unless we wanted to provide some kind of callback mechanism, which is probably overkill here.) 3. For an unsupported mime type, I'd recommend a more specific exception than InvalidArgumentException. Give that a custom sub-class that tracks what the actual mime type was that the request had and it rejected. 4. I don't quite grok the "input" section. So if I don't disable the automatic parsing, does that mean request_parse_body() will always fail? Or will it still work, but just be more memory-wasteful? That's not clear to me; I'd prefer if it works but is just memory-wasteful, personally, as that would be more portable for projects that want to use it. --Larry Garfield

Hey, I'm not sure if this is bikeshedding, but the concept of parsing bodies for non-POST requests lands really close to a proposal for adding a QUERY method to the HTTP standard. Draft: https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-safe-method-w-body Discussion: https://github.com/httpwg/http-extensions/labels/safe-method-w-body It's meant to address the recent need for complex querying (GraphQL / Elastic Search) that necessitates using POST but loses the default caching of GET. I think this RFC could serve as the groundwork for supporting QUERY if it's extended to other MIME types in the future as Larry suggested. But QUERY probably still has years to go before there is a consensus on it (I think it's been talked about for 6+ years now) On Tue, Dec 5, 2023 at 2:29 PM Ilija Tovilo <tovilo.ilija@gmail.com> wrote: