Added support for http-interop/http-middleware 0.5.0 (No BC Break)
#46
Conversation
|
I've left composer.lock with http-middleware version 0.4.1 so tests run with:
|
| "repositories": [ | ||
| { | ||
| "type": "git", | ||
| "url": "https://github.com/webimpress/zend-expressive-router.git" |
There was a problem hiding this comment.
Why is this package not published on packagist.org?
There was a problem hiding this comment.
@zf2timo This PR requires another one to be merged before - zendframework/zend-expressive-router#36
After merge PR in expressive-router, this PR will be updated.
| @@ -77,7 +79,7 @@ public function clearStrategies() | |||
| public function process(ServerRequestInterface $request, DelegateInterface $delegate) | |||
| { | |||
| if (in_array($request->getMethod(), $this->nonBodyRequests)) { | |||
| return $delegate->process($request); | |||
| return $delegate->{HANDLER_METHOD}($request); | |||
There was a problem hiding this comment.
Why is this changed? It increases the complexity and I think it has an negative impact on the performance.
There was a problem hiding this comment.
The purpose of this PR is to provide Non-BC-Break change to support http-interop/http-middleware in version 0.4.1 and 0.5.0. We accomplish it by using separate repository - webimpress/http-middleware-compatibility. Now consumer of the library can decide what version of http-interop middleware to use. Please note that handler/delegator interface has been changed in 0.5.0 release and this is the simplest way (I can see right now) to provide that functionality.
Please see webimpress/http-middleware-compatibility library for more information.
There was a problem hiding this comment.
Okay, I can see your intend now. But does it really need a solution like this? The http-interop is not a stable release yet. So it could be possible that not only the name of the method changes, but also the signature changes. These changes can not be covered by your solution.
The signature changes can only be handled by a wrapper module - which is my preferred solution, because you do not have the "magic" with {HANDLER_METHOD}($request) .
There was a problem hiding this comment.
@zf2timo Please note that signature has been changed (of middleware) and interfaces has different namespace. I covered it all in my library. This "magic" trick with method name is the simples solution what I can think of right now, and probably it will work until we have a final PSR-15.
Of course wrapper will be better, but unfortunately wrapper is going to provide BC break. We would rather avoid this right now, but we don't want stop users of using the latest version of http-interop middleware. With that solution you can use any version. This is temporary solution, the final one will be when PSR-15 is released.
I wouldn't be worried about performance. We define the constant only on autoloading. Other than that we just using aliases also defined on autoloading. In your library which consumes that library you can use http-interop directly. Whatever is easier for you. Finally, when PSR-15 will be released you will need to update your application anyway. And using this solution, it's quite possible that you will be able to do before all zend libraries implement that changes 😄
There was a problem hiding this comment.
I don't see a likely performance change due to this, TBH. As @webimpress notes, the constant and aliases are defined when the autoloaders are first created, which means in production it will happen the first time the application is hit only, and not on subsequent requests.
Second, regarding this:
it could be possible that not only the name of the method changes, but also the signature changes
While this change from 0.4.1 to 0.5.0 is pretty major, I think this will likely be the last major change we have in the specification. 0.4.1 proved itself quite well; 0.5.0 is mainly around some re-naming to allow usage of the "request handler" (formerly the "delegate") outside the scope of the middleware interface; in terms of how the interfaces relate and work, they are exactly the same.
The main concern I have is that we have to make two changes: one now, to allow for both backwards and forwards compatibility, and another for the next major release, which removes this workaround in favor of the final PSR-15 interfaces. Having to make changes twice, across multiple libraries, is painful and often fragile (we forget the order in which the changes need to be made, or forget which libraries need changes, etc.).
The chief benefit I see to making the change now is that it provides an implementation of the current version of the PSR-15 specification, which will allow it to move forward towards an acceptance vote. As such, I'm weighing in as being in favor of this change at this time.
I've pointed out the library @webimpress has made to other members of the PSR-15 working group, and want to discuss with them briefly before I merge these, to make sure nothing is being missed. If they think it looks reasonable, I'll proceed in merging this and the other related changes.
We keep also support for previous version. It is accomplished by `webimpress/http-middleware-compatibility` repository, which allow to use any version of http-middleware. Consumer of the library can decide what version of http-middleware to use, and can easily migrate to the latest version at any time.
|
Rebased and changed target do |
Added support for `http-interop/http-middleware` 0.5.0 (No BC Break) Conflicts: composer.lock
|
Thanks, @webimpress! I've resolved the merge conflicts before pushing. |
We keep also support for previosu version. It is accomplished by
webimpress/http-middleware-compatibilityrepository, which allow to use any version of http-middleware. Consumer of the library can decide what version of http-middleware to use, and can easily migrate to the latest version at any time.Requires zendframework/zend-expressive-router#36 to be merged before and released. Then
composer.jsonin this PR should be updated.