- Notifications
You must be signed in to change notification settings - Fork90
Modify middleware listener to use stratigility pipe#217
Modify middleware listener to use stratigility pipe#217
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Apparently I didn't have an up to date |
0e0a650 to2f947e8CompareRebased correctly now \o/ |
src/MiddlewareListener.php Outdated
| $psr7Request, | ||
| $psr7ResponsePrototype, | ||
| function (PsrServerRequestInterface$request,PsrResponseInterface$response) { | ||
| thrownewReachedFinalHandlerException( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@weierophinney are you OK with moving this to a static named constructor instead? Asking because consistency...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I am, but I'm not convinced it's needed, as there's a static message with no dynamic substitutions, and no additional behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
One potential refactor I see here is to instead defineReachedFinalHandlerException as follows:
class ReachedFinalHandlerExceptionextends RuntimeException{publicstaticfunctioncreateFinalHandler() {returnnewself('Reached the final handler for middleware pipe - check the pipe configuration'); }publicfunction__invoke() {throw$this; }}
This would then allow you to do the following above:
$return =$pipe($psr7Request,$psr7Response, ReachedFinalHandlerException::createFinalHandler());
Not sure if that's abusing inheritance and SRP, though. 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yikes, I'm not really sure about the exception itself being the final handler, although I gotta say it's quite clever... does feel a bit wrong. I'll just refactor to named constructor for now and if you want to change it to be the actual final handler, I'm happy to :)
src/MiddlewareListener.php Outdated
| $middlewareName ='noMiddlewarePiped'; | ||
| $middlewareToBePiped =null; | ||
| foreach ($middlewaresToBePipedas$middlewareToBePiped) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Segregate to a private method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Probably should just return$pipe in the private method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done
src/MiddlewareListener.php Outdated
| $pipe =newMiddlewarePipe(); | ||
| $pipe->setResponsePrototype($psr7ResponsePrototype); | ||
| $middlewaresToBePiped = !is_array($middleware) ? [$middleware] :$middleware; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Don't negate: invert the ternary please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done
src/MiddlewareListener.php Outdated
| $middlewareName ='noMiddlewarePiped'; | ||
| $middlewareToBePiped =null; | ||
| foreach ($middlewaresToBePipedas$middlewareToBePiped) { | ||
| $middlewareName =is_string($middlewareToBePiped) ?$middlewareToBePiped :get_class($middlewareToBePiped); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Becauseget_class() is stupid this check is insufficient
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Didn't change this from the original code...
src/MiddlewareListener.php Outdated
| foreach ($middlewaresToBePipedas$middlewareToBePiped) { | ||
| $middlewareName =is_string($middlewareToBePiped) ?$middlewareToBePiped :get_class($middlewareToBePiped); | ||
| if (is_string($middlewareToBePiped) &&$serviceManager->has($middlewareToBePiped)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
is_string() check is duplicate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not sure what to do about it; it's just the flexibility around using a string as the middleware (load from container), or a callable (not ideal, but allowed)
src/MiddlewareListener.php Outdated
| if (is_string($middlewareToBePiped) &&$serviceManager->has($middlewareToBePiped)) { | ||
| $middlewareToBePiped =$serviceManager->get($middlewareToBePiped); | ||
| } | ||
| if (!is_callable($middlewareToBePiped)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should probably alsoNOT be a string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
An example is an incompatible function name being passed in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If it's a string, it won't be a string any more. Also just following same logic that was already there...
src/MiddlewareListener.php Outdated
| $event->setController($middlewareName); | ||
| $event->setControllerClass(get_class($middleware)); | ||
| if (null !==$middlewareToBePiped) { | ||
| $event->setControllerClass(get_class($middlewareToBePiped)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You always assume that the last piped middleware is the "responding" one here... Probably needs documenting/commenting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Removed these as there's no longer just one middleware, so doesn't make sense. I guess this causes a definite BC break which is kinda rubbish though?
src/MiddlewareListener.php Outdated
| $middlewaresToBePiped = !is_array($middleware) ? [$middleware] :$middleware; | ||
| $middlewareName ='noMiddlewarePiped'; | ||
| $middlewareToBePiped =null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can an empty pipeline be considered valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't see why not; you'll just reach the final handler anyway...
@Ocramius some changes applied as requested. |
shakes fist at PHP 5.6 support |
composer.json Outdated
| "fabpot/php-cs-fixer":"1.7.*", | ||
| "phpunit/phpunit":"^4.5" | ||
| "phpunit/phpunit":"^4.5", | ||
| "zendframework/zend-stratigility":"^2.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This should likely be^2.0.1, but I can take care of that during merge.
src/MiddlewareListener.php Outdated
| $psr7Request, | ||
| $psr7ResponsePrototype, | ||
| function (PsrServerRequestInterface$request,PsrResponseInterface$response) { | ||
| thrownewReachedFinalHandlerException( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I am, but I'm not convinced it's needed, as there's a static message with no dynamic substitutions, and no additional behavior.
| $event->setName(MvcEvent::EVENT_DISPATCH_ERROR); | ||
| $event->setError($application::ERROR_EXCEPTION); | ||
| $event->setController($middlewareName); | ||
| $event->setControllerClass(get_class($middleware)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why are these lines removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There is potentially no longer just "one" middleware name... how do we decide which is the failed one? First? Last? IIRC I don't think there's any way (outside of the exception bubble here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah, right - that makes sense, then.
| if (is_string($middlewareToBePiped) &&$serviceLocator->has($middlewareToBePiped)) { | ||
| $middlewareToBePiped =$serviceLocator->get($middlewareToBePiped); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This needs to change to be:
if (!$middlewareToBePipedinstanceof \Http\Interop\ServerMiddleware\MiddlewareInterface && !is_callable($middlewareToBePiped))
as zend-stratigility 2.0 allows either of those types. Testing only foris_callable() means that http-interop middleware cannot be used!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Once you make those changes, you'll also need to add tests for piping http-interop middleware.
src/MiddlewareListener.php Outdated
| $psr7Request, | ||
| $psr7ResponsePrototype, | ||
| function (PsrServerRequestInterface$request,PsrResponseInterface$response) { | ||
| thrownewReachedFinalHandlerException( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
One potential refactor I see here is to instead defineReachedFinalHandlerException as follows:
class ReachedFinalHandlerExceptionextends RuntimeException{publicstaticfunctioncreateFinalHandler() {returnnewself('Reached the final handler for middleware pipe - check the pipe configuration'); }publicfunction__invoke() {throw$this; }}
This would then allow you to do the following above:
$return =$pipe($psr7Request,$psr7Response, ReachedFinalHandlerException::createFinalHandler());
Not sure if that's abusing inheritance and SRP, though. 😁
Also,@asgrim — You'll need to rebase against latest develop branch, as I pushed a bunch of QA tooling changes earlier (PHPUnit 5.7/6.0 support, in particular, as well as zend-coding-standards integration). |
… instance for consistency
b8511d3 to7d89900Comparetest/MiddlewareListenerTest.php Outdated
| { | ||
| $expectedOutput =uniqid('expectedOutput',true); | ||
| $event =$this->createMvcEvent('path',newclass($expectedOutput)implements MiddlewareInterface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
bah, doesn't work on PHP 5.6 - I thoughtdevelop branches were min PHP 7.1 now? Maybe I didn't read properly?
@weierophinney all updated, thanks for review! |
Forward portzendframework#237Conflicts:src/Service/HttpDefaultRenderingStrategyFactory.php
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Few last nit-picks, but looks great!
src/MiddlewareListener.php Outdated
| $psr7Request =$psr7Request->withAttribute($key,$value); | ||
| } | ||
| $return =$middleware($psr7Request, Psr7Response::fromZend($response)); | ||
| $return =$pipe( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@asgrim So sorry I didn't notice this earlier, but... If we're pinning to Stratigility 2, we should likely use the http-interopprocess() method here instead of__invoke(), which means using aDelegateInterface instead of a callable.
May I suggest the following:
$pipe->setResponsePrototype($psr7ResponsePrototype);$return =$pipe->process($psr7Request,new \Zend\Stratigility\Delegate\CallableDelegateDecorator(function (PsrServerRequestInterface$request,PsrResponseInterface$response) {throw ReachedFinalHandlerException::create(); }));
That would accomplish the same as what you have here, and be forwards compatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, this seems reasonable, though I note that__invoke does the wrapping in theCallableDelegateDecorator for us, but I have no strong feelings either way. If__invoke is eventually going to go, then yes the switch makes sense!
setResponsePrototype is already called increatePipeFromSpec btw, so I don't think needs calling again (unless I missed something)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah, okay, nice.
As noted, usingprocess() will be forwards compatible, meaning that when Stratigility adopts a finalized PSR-15, we likely can just add an|| to the constraint and not need any actual code changes. I figure, do it once. 😄
| namespaceZend\Mvc\Exception; | ||
| finalclass MiddlewareNotCallableExceptionextends RuntimeException |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Since we're allowing either callable or http-interop middleware, perhaps we should change this toInvalidMiddlewareException instead?
| $pipe =newMiddlewarePipe(); | ||
| $pipe->setResponsePrototype($responsePrototype); | ||
| foreach ($middlewaresToBePipedas$middlewareToBePiped) { | ||
| if (null ===$middlewareToBePiped) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If we rename the exception toInvalidMiddlewareException, we could also add another named constructor:::becauseNull() (obviously, choose something better!).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Per discussion in Slack, I'll perform the last requested changes now and merge.
Thanks,@asgrim!
…stener-to-use-stratigility-pipeModify middleware listener to use stratigility pipe
Also, adds a `fromNull()` method, allowing it to be used in cases wherea `null` value is provided for middleware. This means that `null`middleware is handled exactly the same way as any other invalidmiddleware type, which required changes to several test expectations.
Instead of `__invoke()`, for forwards compatibility once PSR-15 isratified.
Thanks,@asgrim |
Taking issue#196 into play, this PR proposes to update the
MiddlewareListenerto useStratigilityto allow piped middleware. This means themiddlewareparam can become anarrayand everything is pushed into theMiddlewarePipe... as let's be fair, only allowing just a single middleware is only marginally useful.For this to work,
zendframework/zend-stratigilityneeds to become a requirement for those usingMiddlewareListener(I added it asrequire-devfor now), so could be seen as a BC break potentially; so happy to negotiate on how we'd like this added tocomposer.json. Otherwise, I believe (pending CI results) all the other tests still pass though.