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]
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
