jshannon63 / laravel-psr15-middleware

Use your PSR-15 compliant middleware in Laravel
MIT License
13 stars 5 forks source link

Attributes get lost between middlewares #2

Closed dandjo closed 6 years ago

dandjo commented 6 years ago

It seems, that the attributes passed to ServerRequestInterface with withAttribute() get lost somewhere in the Dispatcher. Reproduce with the following setup:

psr15middleware.php

return [
    'middleware' => [
        Middlewares\ClientIp::class,
        App\Http\Middleware\Psr15\Example::class,
    ],
    'groups' => [
        'web' => [
        ],
        'api' => [
        ],
        'custom' => [
        ],
    ],
    'aliases' => [
    ]
];

The Example Middleware tries to fetch the attribute, which is always null.

App\Http\Middleware\Psr15\Example.php

    public function process(ServerRequestInterface $request,
                            RequestHandlerInterface $handler): ResponseInterface
    {
        return $handler->handle($request)
            ->withAddedHeader('client-ip', $request->getAttribute('client-ip', '-'));
    }
jshannon63 commented 6 years ago

Sorry it took me a while to get to this. v1.53 Should function correctly now. Thanks for the heads up!

dandjo commented 6 years ago

I think there is still a problem with alias middlewares. Could be an issue in Laravel? Are alias middlewares in routes processed before global middlewares?

psr15middleware.php

return [
    'middleware' => [
        Middlewares\ClientIp::class,
        App\Http\Middleware\Psr15\Example::class,
    ],
    'groups' => [
        'web' => [
        ],
        'api' => [
        ],
        'custom' => [
        ],
    ],
    'aliases' => [
        'customheader' => App\Http\Middleware\Psr15\CustomHeader::class,
    ]
];

App\Http\Middleware\Psr15\Example.php

namespace App\Http\Middleware\Psr15;

use Interop\Http\Server\RequestHandlerInterface;
use Interop\Http\Server\MiddlewareInterface;
use Psr\Http\Message\ServerRequestInterface;
use Psr\Http\Message\ResponseInterface;

class Example implements MiddlewareInterface
{
    public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
    {
        $response = $handler->handle($request->withAttribute('attributetest', 'foobar'));
        return $response
            ->withAddedHeader(
                'client-ip',
                $request->getAttribute('client-ip', '-')
            );
    }
}

App\Http\Middleware\Psr15\CustomHeader.php

namespace App\Http\Middleware\Psr15;

use Interop\Http\Server\RequestHandlerInterface;
use Interop\Http\Server\MiddlewareInterface;
use Psr\Http\Message\ServerRequestInterface;
use Psr\Http\Message\ResponseInterface;

class CustomHeader implements MiddlewareInterface
{
    public function __construct($key = null, $value = null)
    {
        $this->key = $key;
        $this->value = $value;
    }

    public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
    {
        return $handler->handle($request)
            ->withAddedHeader(
                'attributetest',
                $request->getAttribute('attributetest', '-')
            )->withAddedHeader($this->key, $this->value ?: '-');
    }
}

routes/web.php

Route::group(['middleware' => ['customheader:hello,world']], function() {
    Route::get('/psr15middleware', function() {
        return 'some output';
    });
});

The header "attributetest" in the response is still null, output in browser console "-".

jshannon63 commented 6 years ago

Yes. I see the issue. It is unfortunate that my test and usage of the library did not reveal this flaw even after the repair. My test method simply checked that the request was properly updated. It seems that we are going to need to identify a PSR-15 Middleware as either a "before", "after" or "terminable" middleware in order to properly time the actions within the laravel pipeline.

Currently the library's middleware is being run as "after" middleware, in other words after the application has seen the request. Quite embarrassing... the application for this library focused on read of request and write to response so this was not uncovered.

I have a few ideas how to fix this, but certainly welcome any suggestions... most likely to be a breaking change. I really appreciate the second set of eyes and hope to avoid another oversight like this one.

jshannon63 commented 6 years ago

Issue should be solved with latest release v1.6. Breaking changes.