Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:108691
To: php internals <internals@lists.php.net>
References: <50BD013E-CF72-414C-BBC0-A7A2E45CBDDB@pmjones.io>
 <CANUQDCiP31aEqULqeGpN66ESHarKxWuXgQt_CqWeCLZc+1L-fA@mail.gmail.com>
 <5904137.fSVIMsojiJ@mcmic-probook>
 <CDBA02D0-07E1-4723-947C-22B403E650C0@pmjones.io>
 <CA+hqhpcOqCqXuOh3bWavJ+pZ5VVhekUO9LkKgd9c+b98VVnZSA@mail.gmail.com>
 <CAG9XoMRm2Nqjg3UbCc5=1_8m2W7qf8QRoSVk6d_qENJCKE1=9A@mail.gmail.com>
 <fcf43463-f9f7-4235-a84b-658074772ed7@www.fastmail.com>
 <3DDBFBA4-8D3A-46C5-9A10-B093A5E2386B@pmjones.io>
 <54493258-B52E-442A-A11D-82E1D4C7DE5E@gmail.com>
 <8264F6CF-AD55-41E2-9F5A-DAE8942E2B79@pmjones.io>
 <f58eea0a-6d71-be57-c0b8-abc4321cdb38@gmail.com>
 <1BF3D29A-04C6-4A82-9216-EAE991A38916@pmjones.io>
Message-ID: <10388b3a-3402-a6dc-3837-b03d735edf46@gmail.com>
Date: Wed, 19 Feb 2020 22:43:40 +0000
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.5.0
MIME-Version: 1.0
In-Reply-To: <1BF3D29A-04C6-4A82-9216-EAE991A38916@pmjones.io>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-GB
Subject: Re: [PHP-DEV] RFC: Server-Side Request and Response Objects (v2)
From: rowan.collins@gmail.com (Rowan Tommins)

On 19/02/2020 20:21, Paul M. Jones wrote:
>> Sell it to me! :)
> I don't do sales; occasionally I can present a good narrative. Maybe the narrative below will help.


Hah! Fair enough. :)


>> - When working out the details, what code should we be picturing using the new classes?
> Not to be flippant, but: request-reading and response-writing code?


The reason I keep trying to pin you down on this is that I am concerned 
about the "jack of all trades" effect - that a feature which lacks focus 
can end up being less useful than if it was refined for one job. From 
the point of view of a reviewer, it's also hard to answer the question 
"do you think this is a good design?" when you don't know what it's the 
design for.

To take a specific example, if an aim is for this class to be used as an 
underpinning for higher-level libraries, we could look at some existing 
examples. We can think about what new functionality they would gain, or 
what old code they could delete, by using this class. We can also look 
at whether the interface as currently proposed would be "comfortable" to 
integrate, or if there are changes that would make it easier.

That doesn't mean you have to declare there to be one single goal, but 
it's easier to get specific about a design fitting a goal than making an 
abstract judgement about it.


>
>> The lack of behaviour also makes it less useful to people writing their own request and response objects
> 1. It "bundles" several related pieces of data; not just $_POST but $_GET, $_COOKIES, etc., so they can be carried around together.
>
> 2. It separates that data from global state.
>
> 4. Its copies of that data are read-only and immutable, so that once set they cannot be modified at a distance when shared around; this keeps them stable throughout the system.


You see, here's an example where specifying the goal matters. In that 
particular comment, I specifically talked about the use case of wrapping 
a larger object around this one. The boilerplate to wrap this object in 
a different interface is probably *larger* than the boilerplate to wrap 
the superglobals directly, so *for that use case* these three features 
are not relevant.



> 3. It parses some of that data into commonly-needed structures (eg. the `$accept*` arrays).


This, however, is the kind of thing I mean by "behaviour", and the kind 
of thing I'd like to see more of. Going back to the goal of "libraries 
could wrap this object and extend it", we can directly see how it would 
make their lives easier because they wouldn't have to reimplement it.

The particular selection of fields feels rather arbitrary, though - for 
instance, I've never heard of "Content-MD5", but would have expected at 
least some URL-related properties, like Host.



> Further, there's no need to optimize for calculation-on-demand, since the calculations are so speedy in the first place. And since it will happen only once in the request lifespan, you might as well build all the properties at construction-time. At that point, getter-type methods are just returning what has already been calculated. And at *that* point, read-only properties do the trick quite nicely.


My preference for methods over properties is partly just OO purism, but 
I do think they have real benefits. One of them is the ability to evolve 
the implementation over time.

For instance, the current design has two arrays of uploaded files, one 
in $_FILES format, and one in a newer format. What if in future, we want 
to add a third, where each item is an object? With properties, we'd have 
to pre-populate all three arrays on construct; but with methods, we 
could refactor the internals to pull all three from some common 
reference point.

Similarly, methods can have parameters, and that can be a great way of 
introducing opt-in behaviour. For instance, maybe in future someone 
requests that getMethod() should have a $allow_override flag, which set 
to false would ignore the X-HTTP-METHOD-OVERRIDE header. With 
properties, you need a separate property for every possible combination, 
and you have to come up with names for them all.


> I think $uploads is a pretty distinct name, while being clearly related to files.


Perhaps "descriptive" would be a better word than "distinct". I can tell 
$files and $uploads apart at a glance, but the names tell me nothing 
about why both exist, or which I should be using.



>>> Another "hard" problem is carrying those values around in the system once they are decoupled from global state; the objects in this RFC purport to do so.
> I answered this above, but to reiterate: "carrying around" is *one* thing ServerRequest does, but not *all* it does.


I'm not disputing that; I'm disputing that that particular feature is in 
any way a hard problem.


> I admit I considered this. However, it makes more sense to me in terms of symmetry/complementarity, and in terms of "what we need on a daily basis", to provide both the request object and the response object together.


One oddity of the proposal is that the two objects aren't actually very 
symmetrical.  For instance, to copy a particular header from a request 
to a response, I'd read a plain array $request->headers[$name], but 
write via a method $response->setHeader($name, $value).

(It's also not clear whether this would work correctly if the header had 
multiple values in the request.)



> Maybe? I can similarly imagine that if new-and-different superglobals 
> appear, the ServerRequest object can evolve to contain and/or 
> translate between them. 


Although we can't predict the future, there are things we can do to make 
it more likely we could adapt to such a change. We should make a 
conscious decision whether this is a goal, or something we're happy not 
to focus on.



>> Parsing a request body from an arbitrary source into arrays that match the structure of $_POST and $_FILES would be a really useful feature.
> Yes, although one can do at least the $_POST portion with ServerRequest as it is now.
> ...
> Call `$request = (new ServerRequestFactory())->new();` and voila: the equivalent of `$_POST`, populated from JSON content, stored as $request->input.


I was actually thinking of the opposite: given a request body which 
didn't come from global state, but which contains data in 
multipart/form-data format, extract the key-value pairs and attached 
files. This is a non-trivial piece of functionality, which would have 
real-world uses; here it is implemented in ReactPHP: 
https://github.com/reactphp/http/blob/121abe0558465cc7f2cecdb3027dae959f348409/src/Io/MultipartParser.php


Rather than accepting the content body as an optional constructor 
parameter, what if there were two named constructors:

- fromSuperGlobalArrays($server, $cookie, $post, $get, $files)
- fromRawRequestBody($server, $cookie, $body)

In the first case, accessing the raw body on the object could give null, 
because none is known - defaulting to global state seems like a mistake 
if decoupling from global state is an explicit aim.

That said, perhaps a third constructor could be a short-hand for just 
that: fromCurrentRequest, which would be equivalent to 
fromRawRequestBody($_SERVER, $_COOKIE, file_get_contents('php://input'));



> Yes, the internals thread I pointed to earlier makes that clear. And 
> though I understand where you're coming from, especially regarding "a 
> dedicated and reliable field for the requested URL", this RFC does not 
> have that kind of ambition, nor do I expect it to in the future.


I agree that re-designing the way web servers pass in the URL is a much 
harder problem, but if you look at pretty much any existing Request 
wrapper, it will make some attempt to extract a URL from the $_SERVER 
array. That really feels like a missing feature of this one right now. 
(I appreciate it's not simple, but that's why it's so useful to have 
someone else write it!)


I hope my comments aren't coming across as too negative. Maybe our 
visions for what this should be are just too different, but some mixture 
of hope and stubbornness makes me want to keep nudging it somewhere 
"better".


Regards,

-- 
Rowan Tommins (né Collins)
[IMSoP]