Skip to content

drop DynamicRouter::match #116

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

drop DynamicRouter::match #116

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d1523b7
drop DynamicRouter::match as its redundant and prone to information loss
dbu Sep 5, 2014
ae6c5d4
cleanup RouterInterface vs RequestMatcherInterface
dbu Sep 9, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,15 @@
Changelog
=========

* **2014-09-05**: Dropped DynamicRouter::match and DynamicRouter no longer
implements RouterInterface but only RequestMatcherInterface and
UrlGeneratorInterface. The match method is redundant with matchRequest but
there was a potential information loss by re-creating the request. If you use
DynamicRouter directly, get access to the Request object or if you are
stand-alone create the request with Request::createFromGlobals().
Deprecated ChainedRouterInterface as it adds no additional information over
VersatileGeneratorInterface.

1.3.0-RC1
---------

Expand Down
21 changes: 13 additions & 8 deletions ChainRouter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

namespace Symfony\Cmf\Component\Routing;

use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
use Symfony\Component\Routing\RouterInterface;
use Symfony\Component\Routing\Matcher\RequestMatcherInterface;
use Symfony\Component\Routing\RequestContext;
Expand Down Expand Up @@ -78,8 +79,14 @@ public function getContext()
/**
* {@inheritdoc}
*/
public function add(RouterInterface $router, $priority = 0)
public function add($router, $priority = 0)
{
if (!($router instanceof RouterInterface
|| $router instanceof RequestMatcherInterface
&& $router instanceof UrlGeneratorInterface
)) {
Copy link
Member

Choose a reason for hiding this comment

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

puh .. that is a very tricky if statement ..
seems like there should be some additional parenthesis in there.

also in the phpdoc for ChainRouterInterface you specify RouterInterface|RequestMatcherInterface which would be quite a lot simpler to express than the above ..

also shouldn't we say what a valid router is?

Copy link
Member Author

Choose a reason for hiding this comment

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

you mean paranthesis around the inner &&? i can do that if you think it helps.
where would you say what a valid router is? in the interface phpdoc? or in the exception message?

throw new \InvalidArgumentException(sprintf('%s is not a valid router.', get_class($router)));
}
if (empty($this->routers[$priority])) {
$this->routers[$priority] = array();
}
Expand Down Expand Up @@ -208,15 +215,13 @@ public function generate($name, $parameters = array(), $absolute = false)
$debug = array();

foreach ($this->all() as $router) {
// if $router does not implement ChainedRouterInterface and $name is not a string, continue
if ($name && !$router instanceof ChainedRouterInterface) {
if (! is_string($name)) {
continue;
}
// if $router does not announce it is capable of handling non-string routes and $name is not a string, continue
if ($name && !is_string($name) && !$router instanceof VersatileGeneratorInterface) {
continue;
}

// If $router implements ChainedRouterInterface but doesn't support this route name, continue
if ($router instanceof ChainedRouterInterface && !$router->supports($name)) {
// If $router is versatile and doesn't support this route name, continue
if ($router instanceof VersatileGeneratorInterface && !$router->supports($name)) {
continue;
}

Expand Down
10 changes: 6 additions & 4 deletions ChainRouterInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,14 @@
interface ChainRouterInterface extends RouterInterface, RequestMatcherInterface
{
/**
* Add a Router to the index
* Add a Router to the index.
*
* @param RouterInterface $router The router instance
* @param integer $priority The priority
* The router must implement either RouterInterface or RequestMatcherInterface and UrlGeneratorInterface.
*
* @param RouterInterface|RequestMatcherInterface $router The router instance
Copy link
Member

Choose a reason for hiding this comment

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

this phpdoc does not match the validation in ChainRouter

Copy link
Member Author

Choose a reason for hiding this comment

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

the full thruth is: RouterInterface|RequestMatcherInterface&UrlGeneratorInterface (or however the syntax for this would be). RouterInterface is the common interface of UrlMatcherInterface and UrlGeneratorInterface. for RequestMatcher there is no such common interface.

* @param integer $priority The priority
*/
public function add(RouterInterface $router, $priority = 0);
public function add($router, $priority = 0);
Copy link
Member Author

Choose a reason for hiding this comment

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

not sure if anybody is using this interface. but it was too restrictive. can we do this? otherwise we would need to keep match and throw an exception on dynamic router.

should this go into changelog as well?

Copy link
Member

Choose a reason for hiding this comment

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

the ChainRouterInterface was added in one of the RCs at the request of @dawehner (aka Drupal), so I assume we can easily change it now ..


/**
* Sorts the routers and flattens them.
Expand Down
9 changes: 6 additions & 3 deletions ChainedRouterInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,14 @@

namespace Symfony\Cmf\Component\Routing;

use Symfony\Component\Routing\RouterInterface;
use Symfony\Component\Routing\Matcher\RequestMatcherInterface;

/**
* Interface to combine the VersatileGeneratorInterface with the RouterInterface
* Interface to combine the VersatileGeneratorInterface
*
* @deprecated This interface adds no value and should not be relied on.
* Better use VersatileGeneratorInterface directly.
*/
interface ChainedRouterInterface extends RouterInterface, VersatileGeneratorInterface
interface ChainedRouterInterface extends VersatileGeneratorInterface
{
}
42 changes: 1 addition & 41 deletions DynamicRouter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\Routing\RequestContext;
use Symfony\Component\Routing\RouteCollection;
use Symfony\Component\Routing\RouterInterface;
use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
use Symfony\Component\Routing\Matcher\RequestMatcherInterface;
use Symfony\Component\Routing\Matcher\UrlMatcherInterface;
Expand All @@ -34,7 +33,7 @@
* @author Larry Garfield
* @author David Buchmann
*/
class DynamicRouter implements RouterInterface, RequestMatcherInterface, ChainedRouterInterface
class DynamicRouter implements UrlGeneratorInterface, RequestMatcherInterface, ChainedRouterInterface
Copy link
Member Author

Choose a reason for hiding this comment

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

this change has actually a higher chance of biting people, if they relied on DynamicRouter implementing RouterInterface.

Copy link
Member Author

Choose a reason for hiding this comment

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

ChainedRouterInterface extends the RouterInterface. this is a problem as we want to drop the UrlMatcherInterface. we could change ChainedRouterInterface to use RequestMatcherInterface and UrlGeneratorInterface instead of RouterInterface.

{
/**
* @var RequestMatcherInterface|UrlMatcherInterface
Expand Down Expand Up @@ -183,45 +182,6 @@ public function supports($name)
return is_string($name);
}

/**
* Tries to match a URL path with a set of routes.
*
* If the matcher can not find information, it must throw one of the
* exceptions documented below.
*
* @param string $pathinfo The path info to be parsed (raw format, i.e. not
* urldecoded)
*
* @return array An array of parameters
*
* @throws ResourceNotFoundException If the resource could not be found
* @throws MethodNotAllowedException If the resource was found but the
* request method is not allowed
*
* @api
*/
public function match($pathinfo)
{
$request = Request::create($pathinfo);
if ($this->eventDispatcher) {
$event = new RouterMatchEvent();
$this->eventDispatcher->dispatch(Events::PRE_DYNAMIC_MATCH, $event);
}

if (! empty($this->uriFilterRegexp) && ! preg_match($this->uriFilterRegexp, $pathinfo)) {
throw new ResourceNotFoundException("$pathinfo does not match the '{$this->uriFilterRegexp}' pattern");
}

$matcher = $this->getMatcher();
if (! $matcher instanceof UrlMatcherInterface) {
throw new \InvalidArgumentException('Wrong matcher type, you need to call matchRequest');
}

$defaults = $matcher->match($pathinfo);

return $this->applyRouteEnhancers($defaults, $request);
}

/**
* Tries to match a request with a set of routes and returns the array of
* information for that route.
Expand Down
13 changes: 9 additions & 4 deletions Tests/Routing/ChainRouterTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

namespace Symfony\Cmf\Component\Routing\Tests\Routing;

use Symfony\Cmf\Component\Routing\VersatileGeneratorInterface;
use Symfony\Component\Routing\Exception\MethodNotAllowedException;
use Symfony\Component\Routing\Exception\ResourceNotFoundException;
use Symfony\Component\Routing\Exception\RouteNotFoundException;
Expand Down Expand Up @@ -569,7 +570,7 @@ public function testGenerateObjectNotFoundVersatile()
$name = new \stdClass();
$parameters = array('test' => 'value');

$chainedRouter = $this->getMock('Symfony\Cmf\Component\Routing\ChainedRouterInterface');
$chainedRouter = $this->getMock('Symfony\Cmf\Component\Routing\Tests\Routing\VersatileRequestMatcher');
$chainedRouter
->expects($this->once())
->method('supports')
Expand Down Expand Up @@ -597,7 +598,7 @@ public function testGenerateObjectName()
$parameters = array('test' => 'value');

$defaultRouter = $this->getMock('Symfony\Component\Routing\RouterInterface');
$chainedRouter = $this->getMock('Symfony\Cmf\Component\Routing\ChainedRouterInterface');
$chainedRouter = $this->getMock('Symfony\Cmf\Component\Routing\Tests\Routing\VersatileRequestMatcher');

$defaultRouter
->expects($this->never())
Expand Down Expand Up @@ -683,7 +684,7 @@ public function testRouteCollection()
public function testSupport()
{

$router = $this->getMock('Symfony\Cmf\Component\Routing\ChainedRouterInterface');
$router = $this->getMock('Symfony\Cmf\Component\Routing\Tests\Routing\VersatileRequestMatcher');
$router
->expects($this->once())
->method('supports')
Expand Down Expand Up @@ -718,6 +719,10 @@ abstract class WarmableRouterMock implements \Symfony\Component\Routing\RouterIn
{
}

abstract class RequestMatcher implements \Symfony\Component\Routing\RouterInterface, \Symfony\Component\Routing\Matcher\RequestMatcherInterface
abstract class RequestMatcher implements \Symfony\Component\Routing\Generator\UrlGeneratorInterface, \Symfony\Component\Routing\Matcher\RequestMatcherInterface
{
}

abstract class VersatileRequestMatcher implements VersatileGeneratorInterface, \Symfony\Component\Routing\Matcher\RequestMatcherInterface
{
}
71 changes: 0 additions & 71 deletions Tests/Routing/DynamicRouterTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,27 +136,6 @@ public function testGetMatcher()
$this->assertSame($this->matcher, $matcher);
}

public function testMatchUrl()
{
$routeDefaults = array('foo' => 'bar');
$this->matcher->expects($this->once())
->method('match')
->with($this->url)
->will($this->returnValue($routeDefaults))
;

$expected = array('this' => 'that');
$this->enhancer->expects($this->once())
->method('enhance')
->with($this->equalTo($routeDefaults), $this->equalTo($this->request))
->will($this->returnValue($expected))
;

$results = $this->router->match($this->url);

$this->assertEquals($expected, $results);
}

public function testMatchRequestWithUrlMatcher()
{
$routeDefaults = array('foo' => 'bar');
Expand Down Expand Up @@ -205,25 +184,6 @@ public function testMatchRequest()
$this->assertEquals($expected, $router->matchRequest($this->request));
}

/**
* @expectedException \Symfony\Component\Routing\Exception\ResourceNotFoundException
*/
public function testMatchFilter()
{
$router = new DynamicRouter($this->context, $this->matcher, $this->generator, '#/different/prefix.*#');
$router->addRouteEnhancer($this->enhancer);

$this->matcher->expects($this->never())
->method('match')
;

$this->enhancer->expects($this->never())
->method('enhance')
;

$router->match($this->url);
}

/**
* @expectedException \Symfony\Component\Routing\Exception\ResourceNotFoundException
*/
Expand All @@ -245,17 +205,6 @@ public function testMatchRequestFilter()
$router->matchRequest($this->request);
}

/**
* @expectedException \InvalidArgumentException
*/
public function testMatchUrlWithRequestMatcher()
{
$matcher = $this->buildMock('Symfony\Component\Routing\Matcher\RequestMatcherInterface', array('matchRequest', 'setContext', 'getContext'));
$router = new DynamicRouter($this->context, $matcher, $this->generator);

$router->match($this->url);
}

/**
* @expectedException \InvalidArgumentException
*/
Expand All @@ -282,26 +231,6 @@ public function testRouteDebugMessageNonversatile()
$this->assertInternalType('string', $router->getRouteDebugMessage('test'));
}

public function testEventHandler()
{
$eventDispatcher = $this->buildMock('Symfony\Component\EventDispatcher\EventDispatcherInterface');
$router = new DynamicRouter($this->context, $this->matcher, $this->generator, '', $eventDispatcher);

$eventDispatcher->expects($this->once())
->method('dispatch')
->with(Events::PRE_DYNAMIC_MATCH, $this->equalTo(new RouterMatchEvent()))
;

$routeDefaults = array('foo' => 'bar');
$this->matcher->expects($this->once())
->method('match')
->with($this->url)
->will($this->returnValue($routeDefaults))
;

$this->assertEquals($routeDefaults, $router->match($this->url));
}

public function testEventHandlerRequest()
{
$eventDispatcher = $this->buildMock('Symfony\Component\EventDispatcher\EventDispatcherInterface');
Expand Down