Hi all, Replying to both Marco Pivetta and Joe Watkins here, as some of their critiques overlap. I apologize in advance for the length of this email.
* * * On Jan 6, 2017, at 10:47, Marco Pivetta <ocram...@gmail.com> wrote: > Bundling the [PSR-7] interface is no biggie, so I suggest steering away and > going that way It's a bigger deal than one might think. But again, I have no desire to highlight the negatives of PSR-7 in this discussion. I'd much rather discuss the RFC on its own merits, which Marco does next: > There are several technical issues too: > > * A magic constructor that fetches data from global state: add a factory for > that, leave the constructor open for modifications or new instantiation ways, > or make it private. I get that. John and I tried both variations, and found that the static-factory version was clunkier than we expected. For example, if ServerRequest uses a static factory, then users extending the ServerRequest class will additionally have to write their own factory. Having the constructor just "felt" better when we used it. But, I am willing to revisit that decision if needed. As far as global state: yeah, I hear you. Since ServerRequest is intended to encapsulate the superglobals, it seemed reasonable to have it default to copying the superglobals internally. Note that the `__contruct(array $globals = [])` signature allows for a complete override of whatever you want. (The vagaries of JIT population come into play here as well.) > * Magic read-only properties that go against the language semantics: make > 'em accessors, or do whatever you want, but don't invent new language > semantics just for your own extension. You're right that it's not often seen. However, public read-only properties are not a new invention for this RFC. They already exist, such as in PDOStatement::$queryString <https://secure.php.net/pdostatement>. FWIW, the use of a public read-only property turns out to be very practical in daily work. Instead of variations on ... $request->getPost(); // returns the whole array $request->getPost($key); // returns a value in the array $request->getPost($key)[$subkey]; // returns a sub-value in the array $request->getPost($key, $default); // returns a value, or a default if not present ... you work with the property like any other array: $request->post; $request->post[$key]; $request->post[$key][$sub]; $request->post[$key] ?? $default; It's just that the property is read-only, and you get an exception if you try to change it (same as with PDOStatement::$queryString, if I recall correctly.) > * A series of non-exhaustive properties that give some "helper" API to see > if it's XHR or not, violating basic SRP. Are you abstracting a request or > what you want to do with the request? You can define separate utility > functions for that. Yes, functions. /me nods They are definitely non-exhaustive; the list of properties was put together from several reference projects that have classes similar to ServerRequest (more on that below). When some of those projects had a use for for a particular value, I figured it made sense to include it here. (If you or others want to provide patches to make it more exhaustive, so much the better!) As for being a violation of the single-responsibility principle, I don't know. It seems reasonable to think that if the object contains (e.g.) `$request->server['HTTP_FORWARDED']`, it might do well to contain a parsed version of the same information as an array in `$request->forwarded`. > * As per current API, I'd expect the ctor to throw an exception when used > outside the web SAPI context I will defer to John Boehr on this one. I do wonder how difficult that would make it to test at the command-line, or include in command-line testing environments (e.g. PHPUnit). > * You use the *with* prefix for setting values, while you know exactly that > *with* was used to differentiate from a mutable and a non-mutable API in the > existing request abstractions: this makes things confusing again I'm sorry, I don't understand the criticism here. The `ServerRequest::with*()` methods do in fact provide an immutable API (a pretty strict one at that). That is, they return a new copy of the ServerRequest object with the changed values, leaving the original copy unchanged. Maybe I'm not getting what you're saying ... ? > * There's no interface for these classes, and they are not marked as final, > as far as I can see. This means that users will extend them (unwise choice), > and with that they will have a class with reflection bits in the extension > (extremely bad idea!). An interface and final are needed here. Extensibility is an intended option. Many classes in PHP can be extended, to good effect (e.g., PDO). But I am willing to be hear about why this is a bad call -- can you expand on your reasoning? > I am obviously defensive and biased here Sure; anyone would be. Thanks for being up-front about it though. :-) * * * On Jan 7, 2017, at 06:00, Joe Watkins <pthre...@pthreads.org> wrote: > > Regardless of the details of the implementation, I feel it necessary to point > out that this is a surprising RFC. My apologies for giving you cause to be surprised. I thought I had broadcast my intentions back in September <http://news.php.net/php.internals/96156> but obviously I failed. My bad. > There are extensions that are absolutely a core part of the ecosystem that > remain outside of php-src because that is where they belong. xdebug is one, > apc was another, redis, memcached, and a whole list of others besides. Noted. Honestly I figure at best this extension has only 1 chance in 3 of getting accepted. However, as I think it is central to the operation of PHP in its primary use cases (i.e., reading superglobals and sending back response headers/cookies/content), taking that chance seems worthwhile. > * it is not restricted by what is possible outside of php-src (as phpdbg > was) FWIW, I have not found it possible to provide read-only public properties in userland, especially when extending a parent class that wants to enforce the read-only nature of those properties. This is central to the intent of ServerRequest. Further, as far as I know, it is not possible to detect if a variable is a reference in userland. Doing so is a prerequsite to enforcing immutability, which ServerRequest intends to do. I am of course happy to be corrected on this, as it would solve other problems I have had elsewhere. * * * Both Joe and Marco had a similar observation. Marco said: > if something new is being designed for inclusion in php-src, and it has to > reach millions of developers, it better be *reeeeeealli* good. ... IMO needs > another few design iterations, and a lot more adoption, in order to become > interesting. Orrrrr you provide us with a factory for an implementation of > these concepts that already has adoption, traction and extremely careful > design behind it. Joe said: > > * it doesn't have any user base > > ... > > I think if you really want to push this forward, the best place to do that is > outside the core, where you can pick a release schedule not bound by php-src, > where the API can shift because of the will of consumers, rather than > internals (dis)ability to agree on anything so contrived as how we should > handle HTTP transactions. > > The day may come where an abstraction is so popular that we should consider > merging it into core, dedicating our time to maintaining it, and even > possibly allow it to deprecate and completely replace the current API ... You are both completely correct, of course, that this extension has so little adoption as to be virtually unknown. It is, in itself, a infant. However, its design is born from commonalities across many different projects, from many different authors. They all independently settled on broadly similar solutions to the problem of dealing with PHP superglobals, and PHP global functions for sending response elements, in a more OO-ish way. Links to references, and longer explanation of the "broadly similar solutions" are in my blog post at <http://paul-m-jones.com/archives/6494>. Meanwhile, note that the reference projects include Aura, Cake, Code Igniter, Horde, Joomla, Lithium, MediaWiki, Phalcon, Symfony (and thereby Drupal and Laravel), Yaf, Yii, and Zend Framework. That's a mere dozen that are themselves representative of wide swaths of other parts of PHP userland. So it is true that this extension, per se, is not widely adopted. But it is also true the collection of ideas and solutions it represents is widely vetted and broadly agreed-upon in intent, if not in the names of specific classes, methods, and properties. -- Paul M. Jones pmjone...@gmail.com 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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
