Skip to content

Commit

Permalink
changed priorities for kernel.request listeners
Browse files Browse the repository at this point in the history
The Firewall is now executed after the Router. This was needed to have access
to the locale and other request attributes that are set by the Router. This
change implies that all Firewall specific URLs have proper (empty) routes like
`/login_check` and `/logout`.
  • Loading branch information
fabpot committed Nov 17, 2011
1 parent 46e5fa5 commit e3655f3
Show file tree
Hide file tree
Showing 6 changed files with 15 additions and 24 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG-2.1.md
Expand Up @@ -36,6 +36,9 @@ To get the diff between two versions, go to https://github.com/symfony/symfony/c

### SecurityBundle

* [BC BREAK] The Firewall listener is now registered after the Router one.
It means that specific Firewall URLs (like /login_check and /logout must now have proper
route defined in your routing configuration)
* added a validator for the user password

### SwiftmailerBundle
Expand Down
Expand Up @@ -102,7 +102,7 @@

<!-- Firewall related services -->
<service id="security.firewall" class="%security.firewall.class%">
<tag name="kernel.event_listener" event="kernel.request" method="onKernelRequest" priority="64" />
<tag name="kernel.event_listener" event="kernel.request" method="onKernelRequest" priority="8" />
<argument type="service" id="security.firewall.map" />
<argument type="service" id="event_dispatcher" />
</service>
Expand Down
Expand Up @@ -76,8 +76,11 @@ static public function getSubscribedEvents()
{
return array(
KernelEvents::REQUEST => array(
array('onEarlyKernelRequest', 253),
array('onKernelRequest', -1)
// must be registered after the session listener
array('onEarlyKernelRequest', 255),

// must be registered after the Router to have access to the _locale
array('onKernelRequest', 16),
),
);
}
Expand Down
Expand Up @@ -130,7 +130,10 @@ public function onKernelResponse(FilterResponseEvent $event)
static public function getSubscribedEvents()
{
return array(
// kernel.request must be registered as early as possible to not break
// when an exception is thrown in any other kernel.request listener
KernelEvents::REQUEST => array('onKernelRequest', 1024),

KernelEvents::RESPONSE => array('onKernelResponse', -100),
KernelEvents::EXCEPTION => 'onKernelException',
);
Expand Down
Expand Up @@ -94,8 +94,9 @@ static public function getSubscribedEvents()
{
return array(
KernelEvents::REQUEST => array(
array('onEarlyKernelRequest', 255),
array('onKernelRequest', 10)
// the early method must be called before the Firewall to be able to generate URLs correctly
array('onEarlyKernelRequest', 128),
array('onKernelRequest', 32),
),
);
}
Expand Down
19 changes: 0 additions & 19 deletions src/Symfony/Component/Security/Http/HttpUtils.php
Expand Up @@ -52,7 +52,6 @@ public function createRedirectResponse(Request $request, $path, $status = 302)
if ('/' === $path[0]) {
$path = $request->getUriForPath($path);
} elseif (0 !== strpos($path, 'http')) {
$this->resetLocale($request);
$path = $this->generateUrl($path, true);
}

Expand All @@ -70,7 +69,6 @@ public function createRedirectResponse(Request $request, $path, $status = 302)
public function createRequest(Request $request, $path)
{
if ($path && '/' !== $path[0] && 0 !== strpos($path, 'http')) {
$this->resetLocale($request);
$path = $this->generateUrl($path, true);
}
if (0 !== strpos($path, 'http')) {
Expand Down Expand Up @@ -120,23 +118,6 @@ public function checkRequestPath(Request $request, $path)
return $path === $request->getPathInfo();
}

// hack (don't have a better solution for now)
private function resetLocale(Request $request)
{
$context = $this->router->getContext();
if ($context->getParameter('_locale')) {
return;
}

try {
$parameters = $this->router->match($request->getPathInfo());

$context->setParameter('_locale', isset($parameters['_locale']) ? $parameters['_locale'] : $request->getLocale());
} catch (\Exception $e) {
// let's hope user doesn't use the locale in the path
}
}

private function generateUrl($route, $absolute = false)
{
if (null === $this->router) {
Expand Down

1 comment on commit e3655f3

@schmittjoh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a pretty bold move. I think it would be worth stating this as a BC break and documenting the priority changes in the changelog. I'm sure there are some listeners out there which rely on the priorities and which need to be updated.

Also imo it would be good to really add tests for all the comments that you have added inline. This is just a matter of time before we break stuff if we do not add tests.

Please sign in to comment.