Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:108540
Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 13.0 \(3608.60.0.2.5\))
In-Reply-To: <CANUQDCiP31aEqULqeGpN66ESHarKxWuXgQt_CqWeCLZc+1L-fA@mail.gmail.com>
Date: Thu, 13 Feb 2020 09:16:49 -0600
Cc: PHP Internals <internals@lists.php.net>
Content-Transfer-Encoding: quoted-printable
Message-ID: <3E805205-A559-44D7-98FA-960E92FE5E6E@pmjones.io>
References: <50BD013E-CF72-414C-BBC0-A7A2E45CBDDB@pmjones.io>
 <CANUQDCiP31aEqULqeGpN66ESHarKxWuXgQt_CqWeCLZc+1L-fA@mail.gmail.com>
To: Niklas Keller <me@kelunik.com>
Subject: Re: [PHP-DEV] RFC: Server-Side Request and Response Objects (v2)
From: pmjones@pmjones.io ("Paul M. Jones")

Hi Niklas,

> On Feb 12, 2020, at 12:20, Niklas Keller <me@kelunik.com> wrote:
>=20
> I think the request / response API is entirely fine being solved in
> userland instead of in php-src. However, since you already made such a
> proposal, I want to give some feedback:

Re: userland, I gave a counterargument in my reply to Mark Randall.

Even so, thanks for providing feedback in spite of that objection; I =
appreciate your time and effort.


> Naming
>=20
> I think we shouldn't take over the naming of the super globals, e.g.
> $_GET really contains the query parameters and has nothing to do with
> GET or POST, so $request->getQueryParameter(...) would be a better
> name.

/me nods

Yeah, naming is one of the hard problems. I considered $query as an =
alternative property name for $get, but in the end, the `$_GET =3D> =
$get` symmetry was too great to ignore. If others here feel that $query =
is a better name for `$_GET` than $get, I will submit to consensus on =
that point.

Using a getQueryParameter() method strikes a little too close to PSR-7, =
and thereby to charges of favoritism. But let's say we do use a method =
instead of a property here, call it getQuery(); then, of the following =
...

    $foo =3D $request->getQuery()['foo'];
    // vs
    $foo =3D $request->query['foo'];

... the latter (using a property) looks and feels more appropriate to =
me. Thus, the RFC specifies properties over methods for ServerRequest.


> Type Safety
>=20
> I think the API should be type safe. Currently $request->get['key']
> can be null | string | string[] | ... Most parameters only appear a
> single time, so a method returning the first parameter value or null
> could be used instead.

This sounds a little like using `$_REQUEST` instead of `$_GET`, =
`$_POST`, and `$_COOKIES`. Using the "first" parameter would then depend =
on the order in which the superglobals get entered into the =
ServerRequest object, and/or we're in the business of reading and =
honoring the `variables_order` INI setting, at which point the logic =
starts sounding rather involved.

So while I do get the desire for type-safety, I'd prefer to avoid all =
that intricacy, and go with something much closer to what PHP itself =
already does. That is, read $get for $_GET, $post for $_POST, etc., and =
just go with what is stored there.


> API Issues
>=20
> I don't see any reason to keep specials such as
> $_SERVER['HTTP_X_REQUESTED_WITH'] vs. $request->requestedWith, which
> is just another HTTP header.

I get that; $requestedWith in 1.x was a boolean $xhr, to let you know if =
`$_SERVER['HTTP_X_REQUESTED_WITH'] =3D=3D=3D 'XmlHttpRequest'`. It =
struck me that there might be different values in the future, and so =
moved to just $requestedWith instead. I am not attached to that =
particular property; if others agree, I am OK with removing it.


> If there's $_SERVER['HTTP_X_HTTP_METHOD_OVERRIDE'] =3D> =
$request->method
> and $_SERVER['REQUEST_METHOD'] =3D> $request->method, how can I get =
the
> original  (actual) method?

The $method property is a calculated value: if there is a method =
override on a POST, $method is the override; otherwise, it is the =
"actual" method. So:

- $request->server['REQUEST_METHOD'] is the original (actual) method,
- $request->server['HTTP_X_METHOD_OVERRIDE'] is the override method, and
- $request->method is the "calculated" value between them.

You can see the code here: =
https://github.com/pmjones/ext-request/blob/2.x/php_request.c#L147-L175


> Given 'echo $content; =3D> $response->setContent($content);', =
shouldn't
> this rather be something like `addContent()`?

That looks like poor describing on my part in the RFC. It is more true =
to say that these are equivalent:

    echo $content;
   =20
    // =3D>
   =20
    $response->setContent($content);
    $responseSender->send($response);

I will try to make that more apparent in the RFC.

> How does streaming responses work? There's ServerResponseSender, but I =
think this should
> be part of the Response API.

Here's an example:

    $fh =3D fopen('/path/to/file.ext', 'rb');
    $response->setContent($fh);
    // ...
    $responseSender->send($response);

When the ServerResponseSender calls $response->getContent() and detects =
a resource, it calls rewind() on that resource, then fpassthru(). That =
should stream through nicely.

For more information, please see the ServerResponseSender methods at =
<https://github.com/pmjones/ext-request#methods-2> under the =
sendContent() bullet:

=E2=80=A2 If the content is a resource, it is sent using rewind() and =
then fpassthru().

=E2=80=A2 If the content is a callable object or closure, it is invoked, =
and then its return value (if any) is echoed as a string; note that =
object returns will be cast to string at this point, invoking the =
__toString() method if present.

=E2=80=A2 Otherwise, the content is echoed as a string; note that =
objects will be cast to string at this point, invoking the __toString() =
method if present.

There are limitations to that approach, but experience has shown that it =
covers the vast majority of common requirements.


> The Request object should be mutable, e.g. you might want to change
> the client IP to be based on a x-forwarded-header instead of the TCP
> source address.

That's a great example.

First, note that ServerRequest avoids setting a $clientIp property. =
Further, *extensions* to the ServerRequest object might well be mutable. =
So, to go with your example, you would be well within bounds to do =
something like this:

    class MyServerRequest extends ServerRequest
    {
        private $clientIp;

        public function __construct(array $globals, ?string $content =3D =
null)
        {
            parent::__construct($globals, $content);
            if ($this->forwarded) {
                $this->clientIp =3D // ...
            }
        }

        public function getClientIp()
        {
            return $this->clientIp;
        }
    }

You could do that for all your custom calculations on a ServerRequest =
object.

> Other Commentary
>=20
>> A: It supports async exactly as much as PHP itself does.
>=20
> Not really. PHP has built-in stream_select / non-blocking streams, so
> supports the tools required for async out of the box.

I will address this separately, per the resolution of our private email =
conversation.


--=20
Paul M. Jones
pmjones@pmjones.io
http://paul-m-jones.com

Modernizing Legacy Applications in PHP
https://leanpub.com/mlaphp

Solving the N+1 Problem in PHP
https://leanpub.com/sn1php