PSR-15 compatibility #253
PSR-15 compatibility #253
Conversation
Adds support for classes implementing the `MiddlewareInterface` of the [PSR-15](https://www.php-fig.org/psr/psr-15/) recommendation. See related [discussion]( clue#185)
| @@ -22,7 +22,8 @@ | |||
| "phpstan/phpstan": "1.10.47 || 1.4.10", | |||
| "phpunit/phpunit": "^9.6 || ^7.5", | |||
| "psr/container": "^2 || ^1", | |||
| "react/promise-timer": "^1.10" | |||
| "react/promise-timer": "^1.10", | |||
| "psr/http-server-middleware": "^1" | |||
There was a problem hiding this comment.
This should(tm) only be a requirement for the tests to run, so I added it as dev-dependency
There was a problem hiding this comment.
Seems about right, we'd rather add a new dev-dependencies instead of a new requirement, as this only affects development and not all users 👍
| ``` | ||
|
|
||
| This is especially useful in order to integrate one of the many existing PSR-15 implementations as global middleware. | ||
|
|
There was a problem hiding this comment.
I acknowledge that this is the bare minimum documentation, feel free to amend *g
There was a problem hiding this comment.
I think this is a solid start, let's focus on the API first and once we have a feeling what this should look like, we can adjust the documentation accordingly 👍
| /** | ||
| * @internal | ||
| */ | ||
| class PsrAwaitRequestHandler implements RequestHandlerInterface |
There was a problem hiding this comment.
This is mostly "stolen" from https://github.com/friends-of-reactphp/http-middleware-psr15-adapter and that needs attribution – I'm not sure how you usually handle this? As comment in the class or README?
There was a problem hiding this comment.
Normally we add a comment to the specific class, something like:
# @copyright Copyright (c) 2024 Simon Frings, taken from https://github.com/clue/framework-x/pull/3 with permission
Nearly all of these projects are published under the MIT license, same goes for https://github.com/friends-of-reactphp/http-middleware-psr15-adapter
| if ($this->next === null) { | ||
| return new Response(); | ||
| } |
There was a problem hiding this comment.
This is a bit weird. But there are at least two occurrences of a handler invocation without the next parameter in the framework:
and this was the easiest solution I could think of to avoid these from exploding :)
| /** | ||
| * @internal | ||
| */ | ||
| class PsrMiddlewareAdapter |
There was a problem hiding this comment.
This should also be attributed to https://github.com/friends-of-reactphp/http-middleware-psr15-adapter
There was a problem hiding this comment.
Same as #253 (comment)
|
I can take care of the failing checks, but I would like to get an initial feedback first on the general direction |
| /** @return PromiseInterface<ResponseInterface> */ | ||
| public function __invoke(ServerRequestInterface $request, callable $next = null): PromiseInterface | ||
| { | ||
| return async(function () use ($request, $next) { |
There was a problem hiding this comment.
async is not available for PHP < 7.4 but I wonder if that's really needed here.
Couldn't this be:
$deferred = new Deferred();
$deferred->resolve($this->middleware->process($request, new PsrAwaitRequestHandler($next)));
return $deferred->promise();or even just
return $this->middleware->process($request, new PsrAwaitRequestHandler($next));since PSR-15 middlewares will most probably be blocking anyways!?
There was a problem hiding this comment.
Don't think it's needed and like you said, with our commitment to PHP 7.4 support we can't really use it here for now (except building a workaround for PHP 7.4, but then we can just use the same workaround for all versions).
There was a problem hiding this comment.
@bwaidelich Awesome, this already looks like a really high quality pull request, thanks for taking the time working on this ❤️
I will go over some of the points and give you a first feedback, but we'll have to come back to this and give it a more detailed review. Sorry if we might take a while with reviewing this (swamped with a lot of work lately), but we're also moving towards a Framework X v0.17.0 to rework the handler structure and we'll probably have a look at this before the release goes out. Can't promise this will be part of the same release, but definitely want to get this in!
Keep up the great work 👍
|
Closing to remove this from my pending PRs |
Adds support for classes implementing the
MiddlewareInterfaceof the PSR-15 recommendation.See related discussion