Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop compatibility with callable response factories on MethodNotAllowedMiddleware and ImplicitOptionsMiddleware #69

Merged
merged 4 commits into from
Jul 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions psalm-baseline.xml
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,4 @@
<code><![CDATA[$this->services[$id]]]></code>
</MixedReturnStatement>
</file>
<file src="test/Response/CallableResponseFactoryDecoratorTest.php">
<InternalMethod>
<code><![CDATA[new CallableResponseFactoryDecorator(fn (): ResponseInterface => $this->response)]]></code>
</InternalMethod>
</file>
</files>
21 changes: 1 addition & 20 deletions src/Middleware/ImplicitOptionsMiddleware.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
namespace Mezzio\Router\Middleware;

use Fig\Http\Message\RequestMethodInterface as RequestMethod;
use Mezzio\Router\Response\CallableResponseFactoryDecorator;
use Mezzio\Router\RouteResult;
use Psr\Http\Message\ResponseFactoryInterface;
use Psr\Http\Message\ResponseInterface;
Expand All @@ -16,7 +15,6 @@
use function assert;
use function implode;
use function is_array;
use function is_callable;

/**
* Handle implicit OPTIONS requests.
Expand All @@ -43,25 +41,8 @@
*/
final class ImplicitOptionsMiddleware implements MiddlewareInterface
{
private ResponseFactoryInterface $responseFactory;

/**
* @param (callable():ResponseInterface)|ResponseFactoryInterface $responseFactory A factory capable of returning an
* empty ResponseInterface instance to return for implicit OPTIONS
* requests.
*/
public function __construct(callable|ResponseFactoryInterface $responseFactory)
public function __construct(private readonly ResponseFactoryInterface $responseFactory)
{
if (is_callable($responseFactory)) {
// Factories are wrapped in a closure in order to enforce return type safety.
$responseFactory = new CallableResponseFactoryDecorator(
static function () use ($responseFactory): ResponseInterface {
return $responseFactory();
}
);
}

$this->responseFactory = $responseFactory;
}

/**
Expand Down
18 changes: 11 additions & 7 deletions src/Middleware/ImplicitOptionsMiddlewareFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,27 +6,31 @@

use Mezzio\Router\Exception\MissingDependencyException;
use Psr\Container\ContainerInterface;
use Psr\Http\Message\ResponseFactoryInterface;

/**
* Create and return an ImplicitOptionsMiddleware instance.
*
* This factory depends on one other service:
*
* - Psr\Http\Message\ResponseInterface, which should resolve to a callable
* that will produce an empty Psr\Http\Message\ResponseInterface instance.
* - Psr\Http\Message\ResponseFactoryInterface, which should resolve to an instance of that interface.
*/
final class ImplicitOptionsMiddlewareFactory
{
use Psr17ResponseFactoryTrait;

/**
* @throws MissingDependencyException If the Psr\Http\Message\ResponseInterface
* service is missing.
* @throws MissingDependencyException If the Psr\Http\Message\ResponseFactoryInterface service is missing.
*/
public function __invoke(ContainerInterface $container): ImplicitOptionsMiddleware
{
if (! $container->has(ResponseFactoryInterface::class)) {
throw MissingDependencyException::dependencyForService(
ResponseFactoryInterface::class,
ImplicitOptionsMiddleware::class,
);
}
Comment on lines +25 to +30
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really want to have this kind of exception handling in each factory?

Feels simpler to improve laminas/laminas-servicemanager to better inspect traces, perhaps?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think that the traces are already easy enough to figure out, without this kind of specific exception, but I was just following convention here and didn't think it was worth breaking BC with documented @throws

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me, it's just something we should perhaps improve in upstream, by rendering better traces in exceptions in a future iteration.

We can decide to drop them overall from this component in a later patch: the redundant ->has() calls all add up quickly, if every service needs to do them


return new ImplicitOptionsMiddleware(
$this->detectResponseFactory($container)
$container->get(ResponseFactoryInterface::class),
);
}
}
27 changes: 1 addition & 26 deletions src/Middleware/MethodNotAllowedMiddleware.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
namespace Mezzio\Router\Middleware;

use Fig\Http\Message\StatusCodeInterface as StatusCode;
use Mezzio\Router\Response\CallableResponseFactoryDecorator;
use Mezzio\Router\RouteResult;
use Psr\Http\Message\ResponseFactoryInterface;
use Psr\Http\Message\ResponseInterface;
Expand All @@ -16,7 +15,6 @@
use function assert;
use function implode;
use function is_array;
use function is_callable;

/**
* Emit a 405 Method Not Allowed response
Expand All @@ -31,23 +29,8 @@
*/
final class MethodNotAllowedMiddleware implements MiddlewareInterface
{
private ResponseFactoryInterface $responseFactory;

/**
* @param (callable():ResponseInterface)|ResponseFactoryInterface $responseFactory
*/
public function __construct(callable|ResponseFactoryInterface $responseFactory)
public function __construct(private readonly ResponseFactoryInterface $responseFactory)
{
if (is_callable($responseFactory)) {
// Factories are wrapped in a closure in order to enforce return type safety.
$responseFactory = new CallableResponseFactoryDecorator(
function () use ($responseFactory): ResponseInterface {
return $responseFactory();
}
);
}

$this->responseFactory = $responseFactory;
}

public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
Expand All @@ -63,12 +46,4 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface
return $this->responseFactory->createResponse(StatusCode::STATUS_METHOD_NOT_ALLOWED)
->withHeader('Allow', implode(',', $allowedMethods));
}

/**
* @internal This method is only available for unit tests.
*/
public function getResponseFactory(): ResponseFactoryInterface
{
return $this->responseFactory;
}
}
18 changes: 11 additions & 7 deletions src/Middleware/MethodNotAllowedMiddlewareFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,27 +6,31 @@

use Mezzio\Router\Exception\MissingDependencyException;
use Psr\Container\ContainerInterface;
use Psr\Http\Message\ResponseFactoryInterface;

/**
* Create and return a MethodNotAllowedMiddleware instance.
*
* This factory depends on one other service:
*
* - Psr\Http\Message\ResponseInterface, which should resolve to a callable
* that will produce an empty Psr\Http\Message\ResponseInterface instance.
* - Psr\Http\Message\ResponseFactoryInterface, which should resolve to an instance of that interface.
*/
final class MethodNotAllowedMiddlewareFactory
{
use Psr17ResponseFactoryTrait;

/**
* @throws MissingDependencyException If the Psr\Http\Message\ResponseInterface
* service is missing.
* @throws MissingDependencyException If the Psr\Http\Message\ResponseFactoryInterface service is missing.
*/
public function __invoke(ContainerInterface $container): MethodNotAllowedMiddleware
{
if (! $container->has(ResponseFactoryInterface::class)) {
throw MissingDependencyException::dependencyForService(
ResponseFactoryInterface::class,
MethodNotAllowedMiddleware::class,
);
}

return new MethodNotAllowedMiddleware(
$this->detectResponseFactory($container)
$container->get(ResponseFactoryInterface::class),
);
}
}
81 changes: 0 additions & 81 deletions src/Middleware/Psr17ResponseFactoryTrait.php

This file was deleted.

36 changes: 0 additions & 36 deletions src/Response/CallableResponseFactoryDecorator.php

This file was deleted.

26 changes: 14 additions & 12 deletions src/Test/AbstractImplicitMethodsIntegrationTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,11 @@
use Mezzio\Router\Route;
use Mezzio\Router\RouteResult;
use Mezzio\Router\RouterInterface;
use MezzioTest\Router\Asset\FixedResponseFactory;
use PHPUnit\Framework\Assert;
use PHPUnit\Framework\Attributes\DataProvider;
use PHPUnit\Framework\TestCase;
use Psr\Http\Message\ResponseFactoryInterface;
use Psr\Http\Message\ResponseInterface;
use Psr\Http\Message\ServerRequestInterface;
use Psr\Http\Server\MiddlewareInterface;
Expand All @@ -46,9 +48,9 @@ abstract public function getRouter(): RouterInterface;
public function getImplicitOptionsMiddleware(?ResponseInterface $response = null): ImplicitOptionsMiddleware
{
return new ImplicitOptionsMiddleware(
function () use ($response): ResponseInterface {
return $response ?? new Response();
}
new FixedResponseFactory(
$response ?? new Response(),
),
);
}

Expand All @@ -60,13 +62,13 @@ public function getImplicitHeadMiddleware(RouterInterface $router): ImplicitHead
);
}

/**
* @return callable(): never
*/
public function createInvalidResponseFactory(): callable
public function createInvalidResponseFactory(): ResponseFactoryInterface
{
return static function (): ResponseInterface {
self::fail('Response generated when it should not have been');
return new class implements ResponseFactoryInterface {
public function createResponse(int $code = 200, string $reasonPhrase = ''): ResponseInterface
{
TestCase::fail('Response generated when it should not have been');
}
};
}

Expand Down Expand Up @@ -236,9 +238,9 @@ public function testWithoutImplicitMiddleware(string $requestMethod, array $allo

$pipeline = new MiddlewarePipe();
$pipeline->pipe(new RouteMiddleware($router));
$pipeline->pipe(new MethodNotAllowedMiddleware(static function () use ($finalResponse): ResponseInterface {
return $finalResponse;
}));
$pipeline->pipe(new MethodNotAllowedMiddleware(
new FixedResponseFactory($finalResponse),
));

$finalHandler = $this->createMock(RequestHandlerInterface::class);
$finalHandler
Expand Down
20 changes: 20 additions & 0 deletions test/Asset/FixedResponseFactory.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
<?php

declare(strict_types=1);

namespace MezzioTest\Router\Asset;

use Psr\Http\Message\ResponseFactoryInterface;
use Psr\Http\Message\ResponseInterface;

final class FixedResponseFactory implements ResponseFactoryInterface
{
public function __construct(public readonly ResponseInterface $response)
{
}

public function createResponse(int $code = 200, string $reasonPhrase = ''): ResponseInterface
{
return $this->response->withStatus($code);
}
}
Loading