woohoolabs / harmony

A simple and flexible PHP middleware dispatcher based on PSR-7, PSR-11, and PSR-15
MIT License
163 stars 16 forks source link

Direct return of response by middleware seems not possible when using a condition #34

Open holtkamp opened 2 weeks ago

holtkamp commented 2 weeks ago

I have a middleware class which checks for the existence of a cache entry and either:

  1. cache entry is found, assign the content to the response object and return the response object directly / do not invoke the remaining middleware
  2. cache entry is not found, invoke the remaining middleware

Something like this:

class MyCacheMiddleware implements MiddlewareInterface
{
    public function process(ServerRequestInterface $request, RequestHandlerInterface $handler) : ResponseInterface
    {
        if($content = $this->getCacheContent($request)){
            $responseFactory = new ResponseFactory();
            $response = $responseFactory->createResponse();
            $response->getBody()->write($content);
            return $response; // No need to invoke remaining middleware: return response directly
        }
    }

    return $handler->handle($request); // Invoke remaining middleware
}

This works properly when adding it to Harmony using:

$harmony->addMiddleware(new MyCacheMiddleware());
$harmony->addMiddleware(new MyNextMiddleware1());
$harmony->addMiddleware(new MyNextMiddlewareN());

However when using a condition, it seems that the remaining middleware is always invoked, even when the response was returned directly:

$harmony->addCondition(new SomeCondition(), fn (Harmony $harmony) => $harmony->addMiddleware(new MyCacheMiddleware()));
$harmony->addMiddleware(new MyNextMiddleware1()); //Always invoked
$harmony->addMiddleware(new MyNextMiddlewareN()); //Always invoked

This also breaks the AuthenticationMiddleware in the documented example, since the returned response is not detected/used properly:

    public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
    {
        // Return Error 401 "Unauthorized" if the provided API key doesn't match the expected one
        if ($request->getHeader("x-api-key") !== [$this->apiKey]) {
            return $this->errorResponsePrototype->withStatus(401); //Will not have effect?
        }

        // Invoke the remaining middleware if authentication was successful
        return $handler->handle($request);
    }

Now my question: is this known (undocumented) behaviour, or is it a bug? I suppose the latter, but I might be wrong.

Analysis I think it is caused by the way the condition functionality is implemented. In case it evaluates to true, a new Harmony instance is created and executed, after which the remaining middleware is always processed: https://github.com/woohoolabs/harmony/blob/2a02f490258ff990de4609694d17be938edb8522/src/Harmony.php#L138

Possible solution A solution might be to add remaining middleware to the new Harmony instance when a condition evaluates to true, something like:

//When condition evaluates to true:
$harmony = new Harmony($this->request, $this->response);
$callable($harmony, $this->request);
//Add remaining middleware of the current Harmony instance to the new/branched Harmony instance
for($i = $this->currentMiddleware + 1; $i< count($this->middleware); $i++){
    $harmony->middleware[] = $this->middleware[$i];
}
$harmony->run();

$this->request = $harmony->request;
$this->response = $harmony->response;

//$this->handle($this->request); //Not needed anymore

Not sure whether this works properly with nested conditions and such?

@kocsismate ?

kocsismate commented 1 week ago

Hi @holtkamp,

Thank you for the report, I'll have a look in the coming days :)