opened
conrad10781
username_0
I've done my best to research the documentation or another issue that clarifies this, and I apologize if there is something that provides clarity to this.

Using the LIFO ( last in first out ) approach, my thought would be when executing the following:

```php
class Middleware_Test
{
    protected $_name;
    
    public function __construct($name)
    {
        $this->_name = $name;
    }
    
    public function __invoke(\Slim\Http\Request $request, \Slim\Http\Response $response, $next)
    {
        print "\nHELLO FROM {$this->_name} Middleware";
        
        $response = $next($request, $response);
        
        return $response;
    }
}

require "../vendor/autoload.php";

$app = new \Slim\App();

$app->add(new Middleware_Test("Middleware1"));
$app->add(new Middleware_Test("Middleware2"));
$app->add(new Middleware_Test("Middleware3"));

$app->group("/some/path", function ()  {
    $this->get("/{testing}", function ($request, $response, $args) {
        return $response->write("Middleware Test Response");
    })->add(new Middleware_Test("Middleware6"))->add(new Middleware_Test("Middleware7"));
})->add(new Middleware_Test("Middleware4"))->add(new Middleware_Test("Middleware5"));
    
$app->run();



```

That the order of execution would be:

```
HELLO FROM Middleware7 Middleware
HELLO FROM Middleware6 Middleware
HELLO FROM Middleware5 Middleware
HELLO FROM Middleware4 Middleware
HELLO FROM Middleware3 Middleware
HELLO FROM Middleware2 Middleware
HELLO FROM Middleware1 Middleware
```

The logic being that the sequence is following what I believe to be the declaration and ultimately the application's logic. 

Middleware1/Middleware2 are added before any routes are set, before any routing logic was processed.

Then, as FastRoute matches the group the groups Middleware is added, and finally the matching route's middleware inside the group gets added.

Instead the order is:

```
HELLO FROM Middleware3 Middleware
HELLO FROM Middleware2 Middleware
HELLO FROM Middleware1 Middleware
HELLO FROM Middleware5 Middleware
HELLO FROM Middleware4 Middleware
HELLO FROM Middleware7 Middleware
HELLO FROM Middleware6 Middleware
```

I'm having some difficulty following the logic of the order. It's the LIFO of the application, then the group specific LIFO ( if applicable ), then the route specific LIFO. 

I could /possibly/ see if the order was 5,4,7,6,3,2,1 that the order of the group and specific route were just swapped, but still ahead of the application. However with the application, group and route all seemingly out of order from their declaration order, I'm a bit confused.

Provided this is expected, can someone provide the explanation on this order of execution?
Slim 3.X middleware order verification
issue
created
l0gicgate
username_1
This is the correct behavior, the reason being is that the group's path will be matched first before the final route's path. And then middleware ordering works the same while being nested (last in, first out).

This is the way it'll work:
```php
/*
 * These will all be executed prior to routing being done
 * Unless you set `determineRouteBeforeAppMiddleware` in your App settings
 * Then these would execute after routing in the same LIFO order (3, 2, 1) which means
 * Middleware 5, 4, 7 and 6 would execute first and then Middleware 3, 2 and 1
 */
$app->add(new Middleware_Test("Middleware1"));
$app->add(new Middleware_Test("Middleware2"));
$app->add(new Middleware_Test("Middleware3"));
```
```php
/*
 * As for the middleware added here, as mentioned above the $app->group path will match first
 * which will cause Middleware 5 and 4 to be executed
 * Then the $this->get("/{testing}") path will be executed after which will cause
 * Middleware 7 and 6 to be executed.
 */
$app->group("/some/path", function ()  {
    $this->get("/{testing}", function ($request, $response, $args) {
        return $response->write("Middleware Test Response");
    })->add(new Middleware_Test("Middleware6"))->add(new Middleware_Test("Middleware7"));
})->add(new Middleware_Test("Middleware4"))->add(new Middleware_Test("Middleware5"));
```

Hope this answers your questions.
comment
created
l0gicgate
username_1
@username_0 you are correct actually. That setting does not affect how the middleware stack is being run.

I just wrote two test cases. My assumption was in fact correct though in the way the middleware is being executed, however I was wrong about the setting. All the setting does it provide the `$request` object with the attribute `routeInfo` and doesn't change the way the middleware stack is being run.

Here are the test cases I wrote and for the record the second test fails:
```php
class Middleware
{
    /**
     * @var string
     */
    protected $name;

    /**
     * Middleware constructor.
     * @param $name
     */
    public function __construct($name)
    {
        $this->name = $name;
    }

    public function __invoke(Request $request, Response $response, callable $next)
    {
        $text = $request->getAttribute('text', '');
        $text .= "MW {$this->name}, ";
        $request = $request->withAttribute('text', $text);
        return $next($request, $response);
    }
}
```
```php
class MiddlewareOrderTest extends TestCase
{
    public function testMiddlewareOrderWithDetermineRouteBeforeAppMiddlewareOn()
    {
        $app = new App([
            'settings' => [
                'determineRouteBeforeAppMiddleware' => true,
            ],
        ]);

        $mw1 = new Middleware('1');
        $mw2 = new Middleware('2');
        $mw3 = new Middleware('3');
        $app->add($mw1);
        $app->add($mw2);
        $app->add($mw3);


        $mw4 = new Middleware('4');
        $mw5 = new Middleware('5');
        $mw6 = new Middleware('6');
        $mw7 = new Middleware('7');
        $app->group("/foo", function () use ($mw6, $mw7)  {
            $this->get("/bar", function (Request $request, Response $response, $args) {
                $text = $request->getAttribute('text');
                return $response->write($text);
            })
                ->add($mw6)
                ->add($mw7);
        })
        ->add($mw4)
        ->add($mw5);

        $request = $this->requestFactory('/foo/bar');
        $response = new Response();
        $response = $app->process($request, $response);
        $app->respond($response);

        $this->expectOutputString("MW 3, MW 2, MW 1, MW 5, MW 4, MW 7, MW 6, ");
    }

    public function testMiddlewareOrderWithDetermineRouteBeforeAppMiddlewareOff()
    {
        $app = new App([
            'settings' => [
                'determineRouteBeforeAppMiddleware' => false,
            ],
        ]);

        $mw1 = new Middleware('1');
        $mw2 = new Middleware('2');
        $mw3 = new Middleware('3');
        $app->add($mw1);
        $app->add($mw2);
        $app->add($mw3);


        $mw4 = new Middleware('4');
        $mw5 = new Middleware('5');
        $mw6 = new Middleware('6');
        $mw7 = new Middleware('7');
        $app->group("/foo", function () use ($mw6, $mw7)  {
            $this->get("/bar", function (Request $request, Response $response, $args) {
                $text = $request->getAttribute('text');
                return $response->write($text);
            })
                ->add($mw6)
                ->add($mw7);
        })
            ->add($mw4)
            ->add($mw5);

        $request = $this->requestFactory('/foo/bar');
        $response = new Response();
        $response = $app->process($request, $response);
        $app->respond($response);

        $this->expectOutputString("MW 5, MW 4, MW 7, MW 6, MW 3, MW 2, MW 1, ");
    }

    /**
     * @param $requestUri
     * @param string $method
     * @return Request
     */
    public function requestFactory($requestUri, $method = 'GET')
    {
        $env = Environment::mock([
            'SCRIPT_NAME' => '/index.php',
            'REQUEST_URI' => $requestUri,
            'REQUEST_METHOD' => $method,
        ]);
        $uri = Uri::createFromEnvironment($env);
        $headers = Headers::createFromEnvironment($env);
        $cookies = [];
        $serverParams = $env->all();
        $body = new RequestBody();
        $request = new Request($method, $uri, $headers, $cookies, $serverParams, $body);

        return $request;
    }
}
```

The output from PHPUnit on the second test case:
```
There was 1 failure:

1) Tests\MockRepo\AppTest::testMiddlewareOrderWithDetermineRouteBeforeAppMiddlewareOff
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'MW 5, MW 4, MW 7, MW 6, MW 3, MW 2, MW 1, '
+'MW 3, MW 2, MW 1, MW 5, MW 4, MW 7, MW 6, '

FAILURES!
Tests: 2, Assertions: 2, Failures: 1.
```
comment
created
akrabat
username_2
I wonder if we should change to FIFO for Slim 4?
comment
created
conrad10781
username_0
If there is a vote, mine would be for FIFO by default.

I don't think it really creates a lot of issues after seeing the order in which Middleware is executed now in Slim 3. 

For anyone migrating to Slim 4, it would still execute app, route group, route as it does now, just the order in which you ->add() Middleware only on those levels would change.

If it was changed to be more LIFO where it was route, route group, app, it seems to get a bit more hairy when considering users who are migrating because it's the same terminology but actually behaves different. Especially if that is relying on, or affected by the "determineRouteBeforeAppMiddleware" setting, or it's equivalent in Slim 4. Meaning, the idea that this setting could affect the order of Middleware execution seems like it would potentially create it's own issue.

So even if someone just read FIFO in the Slim 4 docs and thought "this changes everything". If they went back to Slim 3 and saw what was reported here, they would see it wasn't really that big of a change. As currently it's LIFO of FIFO declaration, and a change to FIFO would just be FIFO of FIFO declaration.
comment
created
odan
username_3
I would be happy if the the order of the middleware stack changes in v4. I think the first middleware must be the first to be executed. That would just be more logical.
comment
created
geggleto
username_4
If we change the ordering now, migrating projects is going to be a bit more tricky. We will have to make sure we document the changes extensively.
comment
created
conrad10781
username_0
Please see https://github.com/slimphp/Slim/issues/2403#issuecomment-366967671 if you haven't already. I don't personally think this is going to be a huge change. Provided again it remains in the same order it does now of app, route group, route. The only change anyone would need is the ordering on those individual "levels", because currently it's LIFO of FIFO declaration, and a change to FIFO would just be FIFO of FIFO declaration. For me at least, this is more confusing.
comment
created
l0gicgate
username_1
I think that FIFO makes more sense, it's easier to comprehend for the end user. I also believe that Slim 4 is a new direction where we introduce new concepts and do things differently. There are already breaking changes, we should steer in the direction that makes the most sense. For the handful of users that will migrate entire projects from 3 to 4, we can provide support.
comment
created
geggleto
username_4
@username_0 we can't change how it works in 3.x (it would be a backwards compatibility break) however we can in 4.
comment
created
geggleto
username_4
Going to close this one as resolved. We can continue the discussions in the other issue about LIFO vs FIFO.
comment
closed
geggleto
username_4
issue
created
conrad10781
username_0
@username_4 , sorry for the confusion. I wasn't implying to change it for 3, just that if it changed in 4, despite it sounding like a huge change, it really wouldn't be if the declaration hierarchy order ( App, Route Group, Route ) stayed the same. Because as it is, it's LIFO of FIFO declaration, which honestly didn't match the expectation I had at least when reading the documentation and implementing.
comment
created
tobiascapin
username_5
Is this order still valid for Slim 3.12? I'm trying that test and I get FIFO order, is something changed?
comment
created
tobiascapin
username_5
Do you confirm that there is no way to have a valid (LIFO or  FIFO) following the declaration order? As far as I know the current fixed order is:
Application MW LIFO, then Group LIFO, then Route LIFO.
Is there any way to follow the declaration order instead of general MW always before the router or group MW? thanks
comment
created
l0gicgate
username_1
@username_5 the order is fixed LIFO for everything in both Slim 3 and Slim 4. 

Unless you rewrite the `MiddlewareAwareTrait` in Slim 3 and most of the App’s core you won’t be able to change that. 

As for Slim 4 you’d need to rewrite the `MiddlewareDispatcher` component and the App’s constructor to use your rewritten dispatcher to make it possible.
comment