Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:121434
Feedback-ID: i8414410d:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-1019-ged83ad8595-fm-20231002.001-ged83ad85
MIME-Version: 1.0
Message-ID: <830f0ff5-f88c-42cd-9224-83b4f9dfdb25@app.fastmail.com>
In-Reply-To: 
 <CAPyj-LBvaJi3AMvLfvs4Occ6y5qwyQOq7x+Wdd3y42FgEqdQUQ@mail.gmail.com>
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>
 <CAPyj-LBvaJi3AMvLfvs4Occ6y5qwyQOq7x+Wdd3y42FgEqdQUQ@mail.gmail.com>
Date: Thu, 19 Oct 2023 14:52:27 +0000
To: "php internals" <internals@lists.php.net>
Content-Type: text/plain
Subject: Re: [PHP-DEV] Re: [RFC][Under discussion] RFC1867 for non-POST HTTP verbs
From: larry@garfieldtech.com ("Larry Garfield")

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