Re: Re: [RFC][Under discussion] RFC1867 for non-POST HTTP verbs
| From: | Larry Garfield | Date: | Thu, 19 Oct 2023 14:52:27 +0000 |
| Subject: | Re: Re: [RFC][Under discussion] RFC1867 for non-POST HTTP verbs | ||
| References: | 1 2 3 4 | Groups: | php.internals |
| Request: | Send a blank email to internals+get-121434@lists.php.net to get a copy of this message | ||
On Thu, Oct 19, 2023, at 11:26 AM, Ilija Tovilo wrote:
>> 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.)
>
> 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.
Is extending it to other mime types in the future the intent? That's not mentioned in future
scope, IIRC. That would be an interesting conversation to have, though probably a distraction from
the current scope.
>> 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.
>
> A custom exception class sounds reasonable. The mime-type is contained
> in the exception message.
This is a pet peeve of mine in exception design. With a common exception, the catching code can do
*nothing* other than log the message wholesale. It doesn't have any context beyond the
exception type, which is often insufficient. I've had cases where I needed to sscanf() a PHP
exception message in order to get the data out of it that i needed in order to handle it correctly.
That's... just kinda gross.
Now that we have readonly properties, any variable part of the message really should be exposed as
either a readonly property of the exception, or via a method. One or the other, but it's
inadequate to only provide that information in a pre-formatted string that requires fugly parsing.
>> 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.
>
> 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.
Please clarify that explicitly in the RFC, so hopefully we remember to document that very very
clearly when the docs are updated later.
--Larry Garfield