Closed wirebox-platform closed 4 years ago
Sounds reasonable to me. Will have to see what @kevinsmith thinks.
I see your point, and I'm open to it. And thanks for taking the time to submit #48!
But I do have some concerns. Consider the situation where a dev (likely the one who didn't initially set up the middleware queue) adds middleware after the RequestHandlerInterface
item in the queue. The request will never make it past that first RequestHandlerInterface
item (since it must return a response), and the issues it causes would be completely non-obvious. I can imagine someone spending hours trying to figure out the problem.
Of course, we could add checks to ensure there's nothing added to the queue after the first RequestHandlerInterface
item, but then we're solving for problems created by an unintended implementation of PSR-15.
On first blush, it seems like it would introduce some real potential costs (especially in usability) for very little gain. I'm inclined to think that if someone wants to kick off an entirely new queue, it would be a good thing that they have to set that up a little more manually. It wouldn't be too much work, just an anonymous class would be enough. Seems like it would be a rare case.
Thoughts? Any other potential use-cases you can imagine other than kicking off a new queue?
Totally see where you're coming from, but I guess it depends on (like always) how code is written and documented.
In any case, because Relay itself is a RequestHandlerInterface
you have to cap the end with something that returns a response for the whole thing to work as promised. Working with your example:
So the thing here is that it's not really a hugely functional change - you can do it with a short-circuiting middleware as it stands - but one of being clear with intent, and it feels more like using the right PSR-15 tool for the job. There has to be an end, however it's done. And maybe using a RequestHandlerInterface
would be easier to spot than a dead-end middleware.
Zend Expressive I believe also supports both: https://docs.zendframework.com/zend-expressive/v3/features/middleware-types/#middleware-types
Still, thanks for taking the time to look over it anyway :)
Hmm. Good arguments. And it's good to see a project supporting it in the wild. I'll put some more thought into this...
p.s, this is the sort of approach I'm using now in the meantime [specifically the $resolver
part], just for reference and perhaps for anyone else looking at this with similar intentions. As Relay supports callables, this just wraps the RequestHandler
up as a callable - so it'll work and not throw an error.
<?php
use Psr\Http\Server\RequestHandlerInterface;
$queue = [
new FooMiddleware(), // implements MiddlewareInterface
new BarMiddleware(), // implements MiddlewareInterface
new BazHandler() // implements RequestHandlerInterface
];
$resolver = function($handler) {
if($handler instanceof RequestHandlerInterface) {
$handler = function($request, $next) use ($handler) {
return $handler->handle($request);
};
}
// other resolvers here...
return $handler;
}
$relay = new Relay($queue, $resolver);
Hmm. Good arguments. And it's good to see a project supporting it in the wild. I'll put some more thought into this...
Hey Kevin, just checking in to see whether you've had a chance to look this one over? I've got my workaround above doing its thing, I just can't help thinking that supporting the whole of PSR-15 is the way to go
Yup, sorry! I kept meaning to get back to this.
Seems like a reasonable thing to support. I opened #49 with the changes, including a test, before I remembered that I was just duplicating the work you did in #48. I'll move conversation over to that PR.
My understanding - with PSR-15, you have MiddlewareInterface and RequestHandlerInterface - both of which essentially are means to the same end - getting a Response instance back. In Relay, if you don't "cap" the end, you'll get an error. So in the documentation, there's this:
My question - would it not be better to add support for RequestHandlerInterface items in the queue and not just MiddlewareInterface ones? So in Runner.php
This way, you can add a definitive end to your queue, or even pass off to an entirely different Relay queue thanks to Relay implementing RequestHandlerInterface...
It's not a wild change and sure, I could typically wrap what I need in an open-ended Middleware. It feels though that adding the above would avoid such workarounds whilst supporting the rest of PSR-15 and allowing you to cap things off properly.