Thanks for the thorough review, Larry! Comments inline.
On Sun, Dec 10, 2017 at 2:09 PM, Larry Garfield <la...@garfieldtech.com> wrote: > On Tuesday, December 5, 2017 11:49:43 AM CST Matthew Weier O'Phinney wrote: >> As of today, we formally begin the REVIEW phase of the proposed PSR-15 >> (HTTP Server Request Handlers) specification. The proposed >> specification is in the fig-standards repository at the following >> locations: >> >> - Specification: >> https://github.com/php-fig/fig-standards/blob/52a479b55f5c235ab44aed2254de7e >> f1f085982e/proposed/http-handlers/request-handlers.md - Metadocument: >> https://github.com/php-fig/fig-standards/blob/52a479b55f5c235ab44aed2254de7e >> f1f085982e/proposed/http-handlers/request-handlers-meta.md > > *puts on CC member hat* > > Commentary as I come across it: > > 1.1: > * "as defined by PSR-7". We should learn from PSR-1's PSR-0 reference and say > something like "as defined by PSR-7 or subsequent replacement PSRs." > Although... Hm, there's no guarantee of exact compatibility there as these are > interfaces, not just text. Open to input on this one. Considering that this specification consumes PSR-7 _specifically_ (the packages depend on it, and the interfaces code against the interfaces defined in PSR-7), I think referencing it makes sense here, particularly as there is no guarantee that a replacement would be compatible. > * "The type of exception is not defined." - As other specs have done, I'd > prefer to see an exception interface that must be included. The exception can > be whatever, I suppose, but having an interface to look for to see if it's a > handler error is consistent with what other specs have done. This was a deliberate decision, and we should likely document it. The reason is pretty straight-forward: both middleware and request handlers may work with other class instances and/or PHP internals which might raise exceptions. We do not want to dictate a design whereby a developer implementing either interface is now required to catch any such exception and re-cast it, as doing so convolutes usage of PHP exception handlers and/or application-level exception handlers. (Additionally, middleware can often be used to _provide_ such exception handling; see Slim and Expressive for examples!) We _could_ document some common solutions to exception handling if you would like; I personally feel it is likely beyond the scope of the specification, however. > 1.2: > * "An middleware". Middleware begins with a consonant, so it should be "A > middleware". Could you provide a pull request for this change, please? > 1.3: > * I am not sure how I feel about the PSR-17 mention here. Clearly I agree > with the concept, but referencing a still not-passed PSR feels dangerous. > Other CC folks, your thoughts? I tend to agree here. I think we can also make a clarification change here that is not substantive. How about something like this: It is RECOMMENDED that any middleware or request handler that generates a response will either compose a prototype of a PSR-7 ResponseInterface or a factory capable of generating a `ResponseInterface` instance in order to prevent dependence on a specific HTTP message implementation. Woody, thoughts? If there is general agreement, I'll create a pull request. > 1.4: > * :thumbs up emoji: > > 2: > * Shouldn't the docblock first lines be single-line? I'm totally fine with a > much longer description block but the first line should be a short single-line > statement. The proposed PSR-5 section 5.1 states, "A Summary MUST contain an abstract of the "Structural Element" defining the purpose. It is RECOMMENDED for Summaries to span a single line or at most two but not more than that." As such, the only one that violates that is the class-level docblock for MiddlewareInterface. Considering it is only a RECOMMENDATION, and not a requirement (MUST), it's debatable whether we need to change it. Since any such changes would be clarifications only, I'm willing to entertain pull requests, however. > Meta document: > > 6.2: > * "external requests are typically..." External requests is a wonky word to > use here. All requests come from something external; that's the whole point. > "Outbound requests" seems more descriptive. Again, a PR would be great. :) > Otherwise, the Metadoc looks awesome! > > What I feel is missing is how one would assemble the handler and middleware. > My first thought looking at the interfaces and descriptions is "wait, so a > middleware can only take a request handler, so that means I can only stack one > level deep? That's kinda pointless, unless an object implements both, in which > case, erm, how's that work?" > > I know that the WG has discussed the assembly process to death already and has > a good sense of how the pieces "should" be assembled, but from reading the > specs I cannot determine what that is. Even if it's just a "one possible way" > in the metadoc, there should be some guidance for how one actually leverages > the interfaces in practice, although something in the spec itself would be > helpful as well. Considering the follow-up from John Porter, this seems to be a common point needing clarification. I'll see if I can work up something for the metadocument later today. > As that would not change the spec, just clarify it, I don't believe that would > necessitate coming out of Review state. The other comments above are all > fairly minor. > > (Recall that under FIG 3 the Review state is more of a "beta", not an "RC", so > larger changes than typos are fine as long as BC is more or less maintained. > Nothing noted above should have any BC implications.) > > Overall: Aside from that additional guidance, this looks really well done. > Nice work, everyone. Once we can get some better guidance included in one or > the other documents this would have my vote for passage. -- Matthew Weier O'Phinney mweierophin...@gmail.com https://mwop.net/ -- You received this message because you are subscribed to the Google Groups "PHP Framework Interoperability Group" group. To unsubscribe from this group and stop receiving emails from it, send an email to php-fig+unsubscr...@googlegroups.com. To post to this group, send email to php-fig@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/CAJp_myXBn_gOOYinOUr1fawvWQvYeSuR%2Bp8Sfij-vnTWXi5nHQ%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
