Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Routing] Added an optional event trigger for handleRouteRequirements #5651

Closed
wants to merge 7 commits into from
24 changes: 24 additions & 0 deletions src/Symfony/Component/Routing/Matcher/UrlMatcher.php
Expand Up @@ -16,6 +16,8 @@
use Symfony\Component\Routing\RouteCollection;
use Symfony\Component\Routing\RequestContext;
use Symfony\Component\Routing\Route;
use Symfony\Component\EventDispatcher\EventDispatcher;
use Symfony\Component\EventDispatcher\Event;
Copy link
Contributor

Choose a reason for hiding this comment

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

you can now remove this one i think


/**
* UrlMatcher matches URL based on a set of routes.
Expand All @@ -30,9 +32,13 @@ class UrlMatcher implements UrlMatcherInterface
const REQUIREMENT_MISMATCH = 1;
const ROUTE_MATCH = 2;

const EVENT_HANDLE_REQUIREMENTS = 'routing.match.requirements';

protected $context;
protected $allow;

protected $dispatcher;

private $routes;

/**
Expand Down Expand Up @@ -65,6 +71,14 @@ public function getContext()
return $this->context;
}

/**
* @param \Symfony\Component\EventDispatcher\EventDispatcher $event_dispatcher
*/
public function setEventDispatcher(EventDispatcher $event_dispatcher)
Copy link
Member

Choose a reason for hiding this comment

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

please typehint the interface. This code would be broken in symfony 2.2 in debug mode as the traceable dispatcher now uses composition instead of inheritance

{
$this->dispatcher = $event_dispatcher;
}

/**
* {@inheritdoc}
*/
Expand Down Expand Up @@ -161,6 +175,16 @@ protected function handleRouteRequirements($pathinfo, $name, Route $route)
$scheme = $route->getRequirement('_scheme');
$status = $scheme && $scheme !== $this->context->getScheme() ? self::REQUIREMENT_MISMATCH : self::REQUIREMENT_MATCH;

if (0 === $status) {
return array($status, null);
Copy link
Contributor

Choose a reason for hiding this comment

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

if on the previous line $status is either

self::REQUIREMENT_MISMATCH or
self::REQUIREMENT_MATCH

then why not checking either one of them and doing a check on 0?

Copy link
Author

Choose a reason for hiding this comment

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

Good point. :) -- Fixing...

}

if ($this->dispatcher) {
$event = new UrlMatcherEvent($route);
$this->dispatcher->dispatch(self::EVENT_HANDLE_REQUIREMENTS, $event);
$status = $event->getStatus();
}

return array($status, null);
}

Expand Down
61 changes: 61 additions & 0 deletions src/Symfony/Component/Routing/Matcher/UrlMatcherEvent.php
@@ -0,0 +1,61 @@
<?php

namespace Symfony\Component\Routing\Matcher;

use Symfony\Component\EventDispatcher\Event;
use Symfony\Component\Routing\Route;

class UrlMatcherEvent extends Event
{
private $route;

private $status;

const REQUIREMENT_MISMATCH = 2;
const REQUIREMENT_MATCH = 1;
Copy link
Member

Choose a reason for hiding this comment

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

why duplicating the constants here ?


public function __construct(Route $route)
{
$this->route = $route;
}

/**
* Returns the route being matched
*
* @return \Symfony\Component\Routing\Route
Copy link
Member

Choose a reason for hiding this comment

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

Please use a short class name

Copy link
Contributor

Choose a reason for hiding this comment

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

still not implemented

*/
public function getRoute()
{
return $this->route;
}

/**
* Set the result of the requirement match
*
* 0 - Mismatch
* 1 - Match
* null - No vote
*
* First listener to set to Mismatch wins
*
* @param $status boolean|null
Copy link
Contributor

Choose a reason for hiding this comment

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

integer not boolean right?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure. It is essentially a nullable boolean (true / false / null are the options).

I will leave it to the community to decide it's boolean nature. :D

Copy link
Member

Choose a reason for hiding this comment

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

@jmather It is not a boolean. You are setting 1 or 2. A boolean is true or false

Copy link
Contributor

Choose a reason for hiding this comment

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

@stof also said something regarding this that was wasted thanks to Github removing old comments

community is speaking through comments remember :)

*/
public function setStatus($status)
{
$this->status = $status;

if ($this->status == self::REQUIREMENT_MISMATCH) {
$this->stopPropagation();
}
}

/**
* Returns the set status
*
* @return boolean|null
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

*/
public function getStatus()
{
return $this->status;
}
}