Hi Peter, Thanks for the detailed critique. I'm not sure I can remedy everything you've pointed out, but even so ...
> On Mar 5, 2020, at 03:00, Peter Bowyer <phpmailingli...@gmail.com> wrote: > > I expect to vote "no". My thoughts are: > > 1. The chosen API. We have an OO approach, but headers and query parameters > are accessed through an array-style mechanism. > > ... > > 2. The OO-ish approach. For a core feature which will be around for 20+ > years I would like a pure(r) OO approach taken, and time to be taken to get > that right. What you have is pragmatic, but as I say in (1) not something > I'm a fan of. Rowan Tommins on 18 Feb 2020 at 20:21 expressed this well > (link: https://externals.io/message/108436#108661). And not just headers and query parameters, but the $_POST-derived parameters, the files, the accept-related values, and frankly everything else in ServerRequest. I really do get the objection; methods, if nothing else, are just what we're used to. The only thing I have to say in return is that if the only methods are getters on properties, that are not themselves mutable, it makes sense to just expose the properties directly. Especially so when they are arrays; then a wide range of array functions can be brought to bear on them, instead of adding more methods to duplicate array-related functionality. Having said that, what changes would you like to see on the API, that you feel might be more worthy of your vote to accept the proposal? Getter methods on ServerRequest? Something else? (I'm not making promises, but perhaps knowing what you'd prefer will give me some ideas.) > I'm not a fan of the way everything maps onto ServerRequest, with certain > $_SERVER vars promoted to be part of it (but not all AFAICT). You're correct: not every element of $_SERVER gets promoted to a property. The `HTTP_*` elements go into $headers, and some of *those* (the ones that bear restructuring into arrays) get promoted to properties of their own. Then, once some elements get promoted to properties, it makes sense to promote related elements (e.g., once you have $contentType and $contentCharset, might as well have $contentLength et al.). What *might* you be a fan of, in terms of what gets promoted to a ServerRequest property, and what does not? > 3. WRT the RFC: > a) there is no section on where this came from. With your involvement in > PSR-7 it looks to be an area you've given much thought to. Why did you > develop ext/request? What problem(s) did you see that this is the answer > to? I skimmed the thread but didn't pick out a clear answer to this. This is one of those places where it's tough to "thread the needle." I want to reduce the chances of the RFC being read as a purely negative criticism of existing projects, but at the same time some level of compare-and-contrast is necessary. One set of readers wants to know what's so terrible about the existing projects, and another set of readers demands positive reasoning only within the proposal. It's a "damned if you do, damned if you don't" kind of situation. My earlier attempt at answering "what problem(s) did you see?" is blogged here: http://paul-m-jones.com/post/2017/01/05/psr-7-vs-the-serverrequestresponse-rfc/ Quoting and summarizing from there: - PSR-7 was born to answer the question, “How can we model HTTP messages in PHP for sending a request, and getting back a response?” That is, how can we standardize the model of an HTTP request message for sending, and the model of the returned the HTTP response, when using PHP as an HTTP client? ... PSR-7 [then] expanded to answer a second question: “How can we model HTTP messages for receiving a request, and sending back a response?” This is in addition to the original goal, but idea is the same: building a standard model of HTTP messages. - The proposed RFC starts out by asking a different question. It is not concerned with modeling HTTP messages, whether when sending or receiving them. Instead, it asks: “How can we take the request-related superglobals in PHP, and the various response-related global functions in PHP, and encapsulate them in objects, to make them at least a little more object-oriented?” Because the RFC begins with a different question, it leads to a different answer. > b) Did you look at how other languages and/or frameworks deal with requests > and responses, and was there inspiration and lessons you took from their > successes (or failures)? [For me this is a key omission in most PHP RFCs] I did look at how other frameworks in PHP handle this. Rowan very rightly pointed out that a summary of my research would be helpful, and based on his suggestion I have published this comparison document: https://docs.google.com/spreadsheets/d/e/2PACX-1vQzJP00bOAMYGSVQ8QIIJkXVdAg-OMEfkgna7-b2IsuoWN8x_TazxEYn-yVDF2XQIqnzmHqdDO3KEKx/pubhtml Here are some notes about it: https://externals.io/message/108436#108889 As far as other languages, I did the barest bit of a survey, but since the goal of this RFC is very directly tied to PHP itself, researching other frameworks in other languages was not especially fruitful. > Thinking about porting existing codebases across, ServerRequest holding > copies rather than references would make it hard to interop with existing > code using the superglobals. /me nods along Maybe? Certainly that's true if the existing code depends on superglobal mutability at some point past bootstrapping the application. But speaking from personal experience only, that kind of mutability dependence seems relatively rare. That is, I myself do not recall seeing applications that co-opt $_GET, $_POST, $_COOKIE, etc. to continuously modify them throughout the application. *Sometimes* I see that in $_ENV and $_SERVER, but even then it is a limited sort of thing that's easily extracted to an application-specific object. However, if an application *does* depend on that kind of thing, you're right: ServerRequest would be disconnected from the superglobals, meaning that application would not be a candidate for migration to ext/request. Do note, though, that more than half of the researched projects are nominally readonly in public scope, much like ServerRequest. As such, I am encouraged to think that the interop problem you describe is not often encountered. > I've had this rewriting legacy apps that use > $_SESSION and the new framework uses an OO session handler; it's not fun > but with references can usually be made to work. In this case, what would > the migration path look like? Johannes Schlüter commented on this on 24 Feb > but I didn't understand his comment (link: > https://externals.io/message/108436#108737). $_SESSION and the session-related functions are a whole different beast, one that this RFC intentionally avoids. Much as I like ext/session, it combines too many different concerns. As you say, you cannot simply make a copy of $_SESSION and have everything else keep working; it and the session-related functions are intimately intertwined. But leaving aside $_SESSION, I opine that the migration path in a system that already treats the non-session superglobals in a read-only fashion might be tedious but not difficult. For example: 1. In your setup code, after you have made any changes to the superglobals that your application requires, instantiate ServerRequest via whatever object-sharing system you have in place (whether by DI container or by any other approach). 2. In one script (or function, or class) inject or otherwise acquire the ServerRequest instance; then, line by line, replace `$_GET` with `$request->query`, etc. Repeat for the other superglobals in that script/function/class. It is *almost* at the level of automated search-and-replace. (And along the way, you might do things like replacing `isset($_GET['foo']) ? $_GET['foo'] : 'default')` with `$request->query['foo'] ?? 'default'`.) 3. Run your regression testing (or smoke-test or spot-check) and correct any errors. 4. Commit, push, and notify QA. 5. Repeat 2 & 3 using the next script/function/class. The benefit of this approach, at least if you are treating the superglobals as read-only, is that `$_GET` continues to work alongside `$request->query`, so you need not change the entire application at once. Baby steps and iterative improvement are possible, even preferred. To be clear, I have not tried this approach on any applications myself. But, it is similar to the methodology used throughout my book on modernizing legacy PHP applications, and I imagine it would be pretty close to what would actually be needed for a migration. > In summary, I like the idea of steering people away from superglobals, > appreciate the work you've put in Thanks for saying ... > and am not persuaded by the approach. ... and noted. After reading this, what modifications to the proposal *might* persuade you (if any) short of rewriting how PHP does superglobals in the first place? Regardless, my thanks for your time and effort writing a good critique. -- Paul M. Jones pmjo...@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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
