Fix action caller next middleware handling

[yiiliveext]

Q	A
Is bugfix?	✔️
New feature?	❌
Breaks BC?	❌
Tests pass?	✔️

There is the next example.
Route

Route::get('/')
            ->to(new ActionCaller(SiteController::class, 'index', $container))
            ->prepend(function (ServerRequestInterface $request, RequestHandlerInterface $handler) use ($container) {
                $response = $container->get(ResponseFactoryInterface::class)->createResponse();
                if (!empty($request->getQueryParams())) {
                    $response->getBody()->write('This route not allowed with query params');
                    return $response->withStatus(403);
                }
                $response = $handler->handle($request);
                $body = $response->getBody();
                $body->rewind();
                $content = $body->getContents();
                file_put_contents('log-body.txt', $content);
                return $response;
            })->then(function (ServerRequestInterface $request, RequestHandlerInterface $handler) use ($container) {
                file_put_contents('log-uri.txt', $request->getUri());
                $response = $container->get(ResponseFactoryInterface::class)->createResponse();
                return $response;
            })
            ->name('site/index'),

Action

public function index(): ResponseInterface
{
    $response = $this->responseFactory->createResponse();
    $output = $this->render('index');
    $response->getBody()->write($output);
    return $response;
}

Don't be scared, this is just my test case:) The first middleware prepended in the prepend() method fires successfully. The second middleware prepended in the then() method does not.
Of cause, I can do like this and evrerything will be ok.

public function index(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
{
    $response = $this->responseFactory->createResponse();
    $output = $this->render('index');

    $response->getBody()->write($output);
    $handler->handle($request);
    return $response;
}

But is it what we expect? I dont think so.

P.S. Basically I do not know why then() method is needed. Everything I want I can do with a prepended middleware.

[samdark]

P.S. Basically I do not know why then() method is needed. Everything I want I can do with a prepended middleware.

That is likely true. Maybe instead of fixing then() we should remove it.

[yiiliveext]

That is likely true. Maybe instead of fixing then() we should remove it.

Nothing will happen, it is just an alias to the to() method.
That to() method is a bad idea, it leads to problems in the future.
So this is my suggestion

Route::get('/', new ActionCaller(SiteController::class, 'index', $container))
           ->prepend(function (ServerRequestInterface $request, RequestHandlerInterface $handler) use ($container) {
               $response = $container->get(ResponseFactoryInterface::class)->createResponse();
               return $response;
            })

or a little bit better

Route::get('/', new ActionCaller(SiteController::class, 'index', $container))
           ->addMiddleware(function (ServerRequestInterface $request, RequestHandlerInterface $handler) use ($container) {
               $response = $container->get(ResponseFactoryInterface::class)->createResponse();
               return $response;
            })

[rustamwin]

Maybe instead of fixing then() we should remove it.

Maybe instead of removing then() we can support before() and after() middlewares?

[xepozz]

@rustamwin

$route
    ->before($middleware1)
    ->before($middleware2)
    ->before($middleware3)

Which order it will have after compiling rules?

[yiiliveext]

Maybe instead of removing then() we can support before() and after() middlewares?

Names don't matter.
With that to() method we have a yii2 array-like style.
There is no logical difference between this and that

Route::get('/')
            ->to(function () {
                //doing something  
            })
            ->to(function () {
                //doing something  
            })
            ->to(new ActionCaller(SiteController::class, 'index', $container))
            ->to(function () {
                //doing something  
            })
            ->to(function () {
                //doing something  
            })
            ->name('site/index'),

Route::get([
    '/' => 'site/index',
    [
        function () {
                //doing something  
            },
        function () {
                //doing something  
            },
        new ActionCaller(SiteController::class, 'index', $container),
        function () {
                //doing something  
            },
        function () {
                //doing something  
            }
    ]
]);

[viktorprogger]

Maybe instead of fixing then() we should remove it.

Maybe instead of removing then() we can support before() and after() middlewares?

There is no differ between before and after, then and to since only position of $handler->handle() matters:

function (ServerRequestInterface $request, RequestHandlerInterface $handler) {
    foo();
    $handler->handle($request);
}

function (ServerRequestInterface $request, RequestHandlerInterface $handler) {
    $handler->handle($request);
    bar();
}

function (ServerRequestInterface $request, RequestHandlerInterface $handler) {
    foo();
    $handler->handle($request);
    bar();
}

[yiiliveext]

I think we should leave one method, that adds (prepends in our case) a middleware.
There are two options:

with the "main" action method (a middleware that will be prepended first)

Route::get('/', new ActionCaller(SiteController::class, 'index', $container))
           ->addMiddleware($middleware1) // ->prepend($middleware1)
           ->addMiddleware($middleware2) // ->prepend($middleware2);

or without

Route::get('/')
           ->addMiddleware(new ActionCaller(SiteController::class, 'index', $container)) //or ->prepend(new ActionCaller(SiteController::class, 'index', $container))
           ->addMiddleware($middleware1) // or ->prepend($middleware1)
           ->addMiddleware($middleware2) //or ->prepend($middleware2);

They are equal. But in my opinion the first option is easier for beginners.

[rustamwin]

I think we should leave both methods, just need to fix the middlewares (ActionCaller, Callback, WebActionsCaller)

[yiiliveext]

I think we should leave both methods

For what purpose?
One hundred and one ways to do the same?
Then I propose to add such methods as after(), before(), last(), first(), afterAfter(), beforeAfter() etc.

[xepozz]

@yiiliveext keep calm please.
Would be nice to declare routes as a squental attached handlers as you propose:

Route::get('/')
    ->next($beforeMiddleware)
    ->next($controllerAction)
    ->next($afterMiddleware)
    ->next($afterMiddleware)

But short syntax Route::get('/', $contollerAction) is necessary because it will used more often than with middlewares IMHO

[samdark]

A single way to do it preferred but we should name it right.

[roxblnfk]

use Psr\Http\Server\MiddlewareInterface as Middleware;
use Psr\Http\Server\RequestHandlerInterface as RequestHandler;

Route::method(string $path)
    ->to(RequestHandler|Closuer|string $controller)
    ->add(Middleware|Closure|string $middleware)
    ->add(Middleware|Closure|string $middleware)
    ->name(string $name);

// or

Route::method(string $path, RequestHandler|Closuer|string|null $controller = null)
    ->add(Middleware|Closure|string $middleware)
    ->add(Middleware|Closure|string $middleware)
    ->name(string $name);

In both cases, the add() method works like in a pipeline;
Order is important only for middleware definitions;
The controller returns Response and it is always executed last (because RequestHandlerInterface do not have next handler)

[yiiliveext]

But short syntax Route::get('/', $contollerAction) is necessary because it will used more often than with middlewares IMHO

I agree and proposed it above.

@yiiliveext keep calm please.

Don't worry, I'm as cool as a cucumber.

Would be nice to declare routes as a squental attached handlers as you propose:

Route::get('/')
    ->next($beforeMiddleware)
    ->next($controllerAction)
    ->next($afterMiddleware)
    ->next($afterMiddleware)

next() is not an appropriate method name. Middlewares work as a stack, which means the LIFO (Last In First Out) way used, so add(), addMiddleware() are more appropriate method names. The last added middleware will be processed first. The second option is the prependMiddleware() method name, maybe it will be more clear for beginners.
So we have two options.

Route::get('/', new ActionCaller(SiteController::class, 'index', $container))
           ->addMiddleware($middleware1)
           ->addMiddleware($middleware2);

Route::get('/', new ActionCaller(SiteController::class, 'index', $container))
           ->prependMiddleware($middleware1)
           ->prependMiddleware($middleware2);

[rustamwin]

Route::get('/')
            ->to($beforeMiddleware)
            ->to(new ActionCaller(SiteController::class, 'index', $container))
            ->to($afterMiddleware)

Route::get('/')
            ->to(new ActionCaller(SiteController::class, 'index', $container))
            ->prepend($beforeMiddleware)
            ->then($afterMiddleware)

They are the same, there is no logical difference, as @yiiliveext said above. But they differ slightly in design, the second case is more readable than the first.

[yiiliveext]

They are the same, there is no logical difference, as @yiiliveext said above. But they differ slightly in design, the second case is more readable than the first.

What about this

Route::get('/')
            ->to(new ActionCaller(SiteController::class, 'index', $container))
            ->prepend($middleware1)
            ->to($middleware2)
            ->then($middleware3)
            ->prepend($middleware4)
            ->then($middleware5)

Looks like it is not very clear. What do you think?

[rustamwin]

Looks like it is not very clear. What do you think?

Your opinion is also not clear

Route::get('/', new ActionCaller(SiteController::class, 'index', $container))
           ->prependMiddleware($middleware1)
           ->addMiddleware($middleware2)
           ->prependMiddleware($middleware3)
           ->addMiddleware($middleware4); // can be infinity

We cannot manage the approach for each user

[yiiliveext]

Looks like it is not very clear. What do you think?

Your opinion is also not clear

Route::get('/', new ActionCaller(SiteController::class, 'index', $container))
           ->prependMiddleware($middleware1)
           ->addMiddleware($middleware2)
           ->prependMiddleware($middleware3)
           ->addMiddleware($middleware4); // can be infinity

We cannot manage the approach for each user

You don't understand. We have either the addMiddleware() or the prependMiddleware() method, but not both together. This is just different names for the same method. So it looks very clear

Route::get('/', new ActionCaller(SiteController::class, 'index', $container))
     ->addMiddleware($middleware1)
     ->addMiddleware($middleware2)
     ->addMiddleware($middleware3)
     ->addMiddleware($middleware4);

$middleware4 will be processed first, $middleware3 will be processed when $middleware4 has called $handler->handle($request) and so on.

[rustamwin]

Middlewares work as a stack, which means the LIFO (Last In First Out) way used, so add(), addMiddleware() are more appropriate method names. The last added middleware will be processed first.

OK. I got you, so that the ActionCaller will be processed last?

[yiiliveext]

Middlewares work as a stack, which means the LIFO (Last In First Out) way used, so add(), addMiddleware() are more appropriate method names. The last added middleware will be processed first.

OK. I got you, so that the ActionCaller will be processed last?

Yes, it is.

[rustamwin]

Yes, it is.

If someone wants to add middleware after ActionCaller what should to do it?

[yiiliveext]

Yes, it is.

If someone wants to add middleware after ActionCaller what should to do it?

If you want to execute some code after an action you can do it in any middleware

Route::get('/', new ActionCaller(SiteController::class, 'index', $container))
     ->addMiddleware(function (ServerRequestInterface $request, RequestHandlerInterface $handler) {
         //do something before the index action
         $response = $handler->handle($request);
         //do something after the index action 
         return $response;
    });

See an example in the first post #210 (comment). I check the query params before the test action and log the body after.

[samdark]

@rustamwin add it before and design middleware like:

function (ServerRequestInterface $request, RequestHandlerInterface $handler) {
    $response = $handler->handle($request);
    // do your stuff
    return $response;
}

[samdark]

This particular change is 👍 Let's remove confusing methods separately.

[rustamwin]

@samdark yes I know, probably we should add it to docs