If app->before return Response, routes before middlewares called anyway

[pqr]

Found two strange behaviours.

Quote from documentation: If any of the before middlewares returns a Symfony HTTP Response, it will short-circuit the whole rendering: the next middlewares won't be run, neither the route callback. You can also redirect to another page by returning a redirect response, which you can create by calling the Application redirect method.

Let us try following code:

<?php
require_once __DIR__ . '/../vendor/autoload.php';

use Silex\Application;
use Symfony\Component\HttpFoundation\Response;

$app = new Application();

$app['debug'] = true;

$app->before(function() {
    return new Response('This is app->before response');
});

$app->get('/test', function() {
    return new Response('This is /test callback response - should never be called because of app->before');
})->before(function() {
    return new Response('This is /test route->before response - should never be called because of app->before');
});

$app->run();

1

If I get http://localhost/myapp/ - it shows Exeption NotFoundHttpException: No route found for "GET /" From my understanding I should receive $app->before Responce even if route not defined

2

If I get http://localhost/myapp/test - it shows 'This is /test route->before response - should never be called because of app->before' I used Xdebug to check if $app->before was called - yes, it was called:

1. $app->before was called and Response returned from the callback
2. then route->before was called and new Response returned from route middleware callback - feels like a bug

[arlaneenalra]

I ran into this issue today and I think I've dug up what's going on. It looks like the RouterListener class in Symfony has an explicit priority set for the method it registers and Silex does not. As a result, RouterListener's hook is being called before the Silex onKernelRequest listener and preventing the before filters from functioning as documented. In case one, the RouterListener throws an exception and immediately causes the dispatcher to exit and a kernel.exception event to fire. In your second case, RouterListener's handler fires first, returns a response and stops event propagation.

What all that means is that before filters happen AFTER routes are processed not before. Fixing this looks like a one line change, that I have in a fork ( https://github.com/arlaneenalra/Silex/commit/6015e935c71ef255e97197ea5aff595fc4b7cb21 ). I set a priority on the Silex handler that should cause it to occur prior to the RouterListener. The number I picked is arbitrary based on what I saw in the RouterListener code. There may be better choices but this one appears to work.

[arlaneenalra]

My patch breaks some of the tests . . . It also looks like there are some mutually exclusive elements between the documentation and the test suite.

In the API documentation for before: "Before filters are run before any route has been matched."

Yet the test suite has two tests in the file BeforeAfterFilterTest.php that contradict this. Specifically: testBeforeFilterExceptionWhenHandlingAnException and testRequestShouldBePopulatedOnBefore.

For testRequestShouldBePopulatedOnBefore to succeed the requested url must be matched to a route and parsed to populate route variables. If it's not matched, there are no defined route variables to populate on the request.

For testBeforeFilterExceptionWhenHandlingAnException the 404 cannot occur. The before filter's exception would stop propagation of the KernelEvents::REQUEST event in HttpKernel unless something eats that exception or the RouteListener runs prior to the before filter and tries to match a url violating the documentation.

To me, these two tests look like they are invalid as written, maybe they should be on route before filters rather than application ones?

[fabpot]

Can you test the patch I've just made and tell me if it fixes all your issues? At least, it fixes a bug exhibited by the original report.

[arlaneenalra]

I'll give it a shot . . . it may take me a moment, I don't have that dev environment setup at home right now.

On Sat, Sep 15, 2012 at 1:10 AM, Fabien Potencier notifications@github.comwrote:

Can you test the patch I've just made and tell me if it fixes all your issues? At least, it fixes a bug exhibited by the original report.

— Reply to this email directly or view it on GitHubhttps://github.com/fabpot/Silex/issues/452#issuecomment-8582226.

[arlaneenalra]

No joy with your patch. Try this test:

    public function testBeforeFilterPriorToRouteMatching()
    {
        $app = new Application();

        $app->before(function () {
            return new Response('before');
        });

        // Issue 452 case 1
        $app->match('/', function() { return new Response('bad'); })
            ->before(function () {
                // Issue 452 case 2
                return new Response('Should never happen!');
            });

        $request = Request::create('/bad_url');
        $this->assertEquals('before', $app->handle($request)->getContent());

    }

I have something that fixes this in my fork, but seems to have issues with the two test I mentioned above. I spotted some issues with my previous pull request so I'll send you another one with the updates to take a look at.

Try that : https://github.com/fabpot/Silex/pull/488

Chris S.

[stof]

@arlaneenalra your PR is wrong. The silex.before event is triggered at the end of kernel.request intentionally.

Moving it with a high priority is a BC break and can cause many issues for users: it makes it impossible to use the router or the security context in a before listener (as they would be called before they are initialized) for instance.

[arlaneenalra]

The api documentation for the application before event says that it should be called before routes are matched . . . What am I missing here? On Sep 15, 2012 9:34 PM, "Christophe Coevoet" notifications@github.com wrote:

@arlaneenalra https://github.com/arlaneenalra your PR is wrong. The silex.before event is triggered at the end of kernel.requestintentionally.

Moving it with a high priority is a BC break and can cause many issues for users: it makes it impossible to use the router or the security context in a before listener (as they would be called before they are initialized) for instance.

— Reply to this email directly or view it on GitHubhttps://github.com/fabpot/Silex/issues/452#issuecomment-8591712.

[arlaneenalra]

If I'm understanding this everything correctly here. . . . it sounds like the the documentation on the application before filter is wrong and that only way to do this what is described in the original bug report is to add a KernelEvents::REQUEST listener instead. Is that accurate?

[fabpot]

Code and docs have been updated to be more consistent and flexible (PR #525).