Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:121430
MIME-Version: 1.0
References: <CAPyj-LD0EDYUZSOM=P9doSZRJ8LipSKKUSBHxV-bRbCV1QsYtQ@mail.gmail.com>
 <CAPyj-LD5=SDXmP5EhR-hfL-FreXXey-CGGoHcqEYq1z44YKRbA@mail.gmail.com> <4daf6b2a-94dc-4d37-bb4d-512bba5f726b@app.fastmail.com>
In-Reply-To: <4daf6b2a-94dc-4d37-bb4d-512bba5f726b@app.fastmail.com>
Date: Thu, 19 Oct 2023 13:26:02 +0200
Message-ID: <CAPyj-LBvaJi3AMvLfvs4Occ6y5qwyQOq7x+Wdd3y42FgEqdQUQ@mail.gmail.com>
To: Larry Garfield <larry@garfieldtech.com>
Cc: php internals <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] Re: [RFC][Under discussion] RFC1867 for non-POST HTTP verbs
From: tovilo.ilija@gmail.com (Ilija Tovilo)

Hi Larry

On Wed, Oct 18, 2023 at 7:26=E2=80=AFPM Larry Garfield <larry@garfieldtech.=
com> wrote:
> > On Fri, Oct 6, 2023 at 3:44=E2=80=AFPM Ilija Tovilo <tovilo.ilija@gmail=
.com> wrote:
> >> https://wiki.php.net/rfc/rfc1867-non-post
>
> The functionality all seems reasonable to me.  I have a few smaller conce=
rns:
>
> 1. Like Derick, I think I'd favor including the config overrides now.

I will check if this can be implemented without too many changes.

> 2. Lots of request bodies are not forms these days; they're frequently JS=
ON 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_pa=
rse_form() or similar?  I'm just concerned about misleading people into thi=
nking 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 overki=
ll here.)

request_parse_body() is indeed not aimed at other content types as-is.
A generic name would allow extending the function to support other
content types in the future, although it's currently unclear whether
that's desirable. E.g. for JSON, people might be confused why there's
a file index in the returned array that is always empty.

> 3. For an unsupported mime type, I'd recommend a more specific exception =
than InvalidArgumentException.  Give that a custom sub-class that tracks wh=
at the actual mime type was that the request had and it rejected.

A custom exception class sounds reasonable. The mime-type is contained
in the exception message.

> 4. I don't quite grok the "input" section.  So if I don't disable the aut=
omatic parsing, does that mean request_parse_body() will always fail?  Or w=
ill it still work, but just be more memory-wasteful?  That's not clear to m=
e; 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.

Whether request_parse_body() can work repeatedly depends on whether
the input has been buffered. application/x-www-form-urlencoded is
buffered and as such works multiple times. multipart/form-data *can*
be buffered if done manually by opening php://input and reading the
whole input before calling request_parse_body(). Yes, for post this
means one needs to disable enable_post_data_reading.

Since files are stored on disk, buffering files means doubling disk
load. Uploading a 2GB file would require at least 4GB of disk space. I
don't think that's a good trade-off as there shouldn't be a reason to
call this function twice.

Ilija