Hi Paul, On 6 Jan 2017 3:19 p.m., "Paul Jones" <pmjone...@gmail.com> wrote:
Hi all, Today marks two weeks since this RFC was introduced. As I said at the start, it's the holidays, so I expect to keep the discussion period open for longer than two weeks, but I am surprised at how little discussion there has been. I find it hard to imagine that all criticisms have been raised, and that the remainder of the RFC is otherwise unobjectionable. So, if there are further questions, please let me know! As mentioned before, the fact that the RFC proposes *yet another* (XKCD 927) request/response API simply increases fragmentation within the ecosystem. This is not useful, especially now that a common interoperability interface for request and response that works quite decently and is getting traction (PSR-7) exists. Bundling the interface is no biggie, so I suggest steering away and going that way, or else you are just adding API that everyone would just need to build wasteful adapters for. 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. * 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. * 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. * As per current API, I'd expect the ctor to throw an exception when used outside the web SAPI context * 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 * 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. Anyway, this is just a partial review of what I got to read with a few minutes of time, yet loads of issues are evident. I am obviously defensive and biased here, but if something new is being designed for inclusion in php-src, and it has to reach millions of developers, it better be *reeeeeealli* good. The issues above are just the tip of the iceberg, and many developers obviously didn't use nor try this extension so far. 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. Greets, Marco Pivetta
