Skip to content

[Routing] apply deprecation triggers and fix tests #13361

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

Merged
merged 3 commits into from
Jan 13, 2015
Merged

[Routing] apply deprecation triggers and fix tests #13361

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bc1c5c8
[Routing] apply deprecation triggers and fix tests
Tobion Jan 11, 2015
bd91867
[FrameworkBundle] remove superfluous test that is already covered in …
Tobion Jan 11, 2015
9af0ff2
[FrameworkBundle] fix routing container param resolving to not access…
Tobion Jan 12, 2015
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
22 changes: 20 additions & 2 deletions src/Symfony/Bundle/FrameworkBundle/Routing/Router.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,10 @@ public function warmUp($cacheDir)
* Replaces placeholders with service container parameter values in:
* - the route defaults,
* - the route requirements,
* - the route pattern.
* - the route host.
* - the route path,
* - the route host,
* - the route schemes,
* - the route methods.
*
* @param RouteCollection $collection
*/
Expand All @@ -90,11 +92,27 @@ private function resolveParameters(RouteCollection $collection)
}

foreach ($route->getRequirements() as $name => $value) {
if ('_scheme' === $name || '_method' === $name) {
continue; // ignore deprecated requirements to not trigger deprecation warnings
}

$route->setRequirement($name, $this->resolve($value));
}

$route->setPath($this->resolve($route->getPath()));
$route->setHost($this->resolve($route->getHost()));

$schemes = array();
foreach ($route->getSchemes() as $scheme) {
$schemes = array_merge($schemes, explode('|', $this->resolve($scheme)));
}
$route->setSchemes($schemes);

$methods = array();
foreach ($route->getMethods() as $method) {
$methods = array_merge($methods, explode('|', $this->resolve($method)));
}
$route->setMethods($methods);
}
}

Expand Down
20 changes: 0 additions & 20 deletions src/Symfony/Bundle/FrameworkBundle/Tests/Routing/RedirectableUrlMatcherTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,26 +38,6 @@ public function testRedirectWhenNoSlash()
);
}

public function testSchemeRedirectBC()
{
$coll = new RouteCollection();
$coll->add('foo', new Route('/foo', array(), array('_scheme' => 'https')));

$matcher = new RedirectableUrlMatcher($coll, $context = new RequestContext());

$this->assertEquals(array(
'_controller' => 'Symfony\Bundle\FrameworkBundle\Controller\RedirectController::urlRedirectAction',
'path' => '/foo',
'permanent' => true,
'scheme' => 'https',
'httpPort' => $context->getHttpPort(),
'httpsPort' => $context->getHttpsPort(),
'_route' => 'foo',
),
$matcher->match('/foo')
);
}

public function testSchemeRedirect()
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be better to prefix the test method's name with testLegacy instead of removing it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, the test doesnt provide any value as I explained above

{
$coll = new RouteCollection();
Expand Down
2 changes: 2 additions & 0 deletions src/Symfony/Component/Routing/Loader/XmlFileLoader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,8 @@ protected function parseRoute(RouteCollection $collection, \DOMElement $node, $p
throw new \InvalidArgumentException(sprintf('The <route> element in file "%s" cannot define both a "path" and a "pattern" attribute. Use only "path".', $path));
}

trigger_error('The "pattern" is deprecated since version 2.2 and will be removed in 3.0. Use the "path" option in the route definition instead.', E_USER_DEPRECATED);

$node->setAttribute('path', $node->getAttribute('pattern'));
$node->removeAttribute('pattern');
}
Expand Down
2 changes: 2 additions & 0 deletions src/Symfony/Component/Routing/Loader/YamlFileLoader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@ public function load($file, $type = null)
throw new \InvalidArgumentException(sprintf('The file "%s" cannot define both a "path" and a "pattern" attribute. Use only "path".', $path));
}

trigger_error('The "pattern" is deprecated since version 2.2 and will be removed in 3.0. Use the "path" option in the route definition instead.', E_USER_DEPRECATED);

$config['path'] = $config['pattern'];
unset($config['pattern']);
}
Expand Down
17 changes: 6 additions & 11 deletions src/Symfony/Component/Routing/Matcher/Dumper/ApacheMatcherDumper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,10 +90,7 @@ public function dump(array $options = array())

$rules[] = $this->dumpRoute($name, $route, $options, $hostRegexUnique);

if ($req = $route->getRequirement('_method')) {
$methods = explode('|', strtoupper($req));
$methodVars = array_merge($methodVars, $methods);
}
$methodVars = array_merge($methodVars, $route->getMethods());
}
if (0 < count($methodVars)) {
$rule = array('# 405 Method Not Allowed');
Expand Down Expand Up @@ -200,13 +197,11 @@ private function dumpRoute($name, $route, array $options, $hostRegexUnique)
*/
private function getRouteMethods(Route $route)
{
$methods = array();
if ($req = $route->getRequirement('_method')) {
$methods = explode('|', strtoupper($req));
// GET and HEAD are equivalent
if (in_array('GET', $methods) && !in_array('HEAD', $methods)) {
$methods[] = 'HEAD';
}
$methods = $route->getMethods();

// GET and HEAD are equivalent
if (in_array('GET', $methods) && !in_array('HEAD', $methods)) {
$methods[] = 'HEAD';
}

return $methods;
Expand Down
11 changes: 4 additions & 7 deletions src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,14 +215,11 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren
$hasTrailingSlash = false;
$matches = false;
$hostMatches = false;
$methods = array();
$methods = $route->getMethods();

if ($req = $route->getRequirement('_method')) {
$methods = explode('|', strtoupper($req));
// GET and HEAD are equivalent
if (in_array('GET', $methods) && !in_array('HEAD', $methods)) {
$methods[] = 'HEAD';
}
// GET and HEAD are equivalent
if (in_array('GET', $methods) && !in_array('HEAD', $methods)) {
$methods[] = 'HEAD';
}

$supportsTrailingSlash = $supportsRedirections && (!$methods || in_array('HEAD', $methods));
Expand Down
10 changes: 5 additions & 5 deletions src/Symfony/Component/Routing/Matcher/TraceableUrlMatcher.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,16 +78,16 @@ protected function matchCollection($pathinfo, RouteCollection $routes)
}

// check HTTP method requirement
if ($req = $route->getRequirement('_method')) {
if ($requiredMethods = $route->getMethods()) {
// HEAD and GET are equivalent as per RFC
if ('HEAD' === $method = $this->context->getMethod()) {
$method = 'GET';
}

if (!in_array($method, $req = explode('|', strtoupper($req)))) {
$this->allow = array_merge($this->allow, $req);
if (!in_array($method, $requiredMethods)) {
$this->allow = array_merge($this->allow, $requiredMethods);

$this->addTrace(sprintf('Method "%s" does not match the requirement ("%s")', $this->context->getMethod(), implode(', ', $req)), self::ROUTE_ALMOST_MATCHES, $name, $route);
$this->addTrace(sprintf('Method "%s" does not match any of the required methods (%s)', $this->context->getMethod(), implode(', ', $requiredMethods)), self::ROUTE_ALMOST_MATCHES, $name, $route);

continue;
}
Expand All @@ -107,7 +107,7 @@ protected function matchCollection($pathinfo, RouteCollection $routes)
$scheme = $this->context->getScheme();

if (!$route->hasScheme($scheme)) {
$this->addTrace(sprintf('Scheme "%s" does not match any of the required schemes ("%s"); the user will be redirected to first required scheme', $scheme, implode(', ', $requiredSchemes)), self::ROUTE_ALMOST_MATCHES, $name, $route);
$this->addTrace(sprintf('Scheme "%s" does not match any of the required schemes (%s); the user will be redirected to first required scheme', $scheme, implode(', ', $requiredSchemes)), self::ROUTE_ALMOST_MATCHES, $name, $route);

return true;
}
Expand Down
8 changes: 4 additions & 4 deletions src/Symfony/Component/Routing/Matcher/UrlMatcher.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ public function match($pathinfo)
}

throw 0 < count($this->allow)
? new MethodNotAllowedException(array_unique(array_map('strtoupper', $this->allow)))
? new MethodNotAllowedException(array_unique($this->allow))
: new ResourceNotFoundException(sprintf('No routes found for "%s".', $pathinfo));
}

Expand Down Expand Up @@ -152,14 +152,14 @@ protected function matchCollection($pathinfo, RouteCollection $routes)
}

// check HTTP method requirement
if ($req = $route->getRequirement('_method')) {
if ($requiredMethods = $route->getMethods()) {
// HEAD and GET are equivalent as per RFC
if ('HEAD' === $method = $this->context->getMethod()) {
$method = 'GET';
}

if (!in_array($method, $req = explode('|', strtoupper($req)))) {
$this->allow = array_merge($this->allow, $req);
if (!in_array($method, $requiredMethods)) {
$this->allow = array_merge($this->allow, $requiredMethods);

continue;
}
Expand Down
14 changes: 12 additions & 2 deletions src/Symfony/Component/Routing/Route.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ public function unserialize($serialized)
*/
public function getPattern()
{
trigger_error('The '.__METHOD__.' method is deprecated since version 2.2 and will be removed in 3.0. Use the getPath() method instead and use the "path" option instead of the "pattern" option in the route definition.', E_USER_DEPRECATED);
trigger_error('The '.__METHOD__.' method is deprecated since version 2.2 and will be removed in 3.0. Use the getPath() method instead.', E_USER_DEPRECATED);

return $this->path;
}
Expand All @@ -169,7 +169,7 @@ public function getPattern()
*/
public function setPattern($pattern)
{
trigger_error('The '.__METHOD__.' method is deprecated since version 2.2 and will be removed in 3.0. Use the setPath() method instead and use the "path" option instead of the "pattern" option in the route definition.', E_USER_DEPRECATED);
trigger_error('The '.__METHOD__.' method is deprecated since version 2.2 and will be removed in 3.0. Use the setPath() method instead.', E_USER_DEPRECATED);

return $this->setPath($pattern);
}
Expand Down Expand Up @@ -548,6 +548,12 @@ public function addRequirements(array $requirements)
*/
public function getRequirement($key)
{
if ('_scheme' === $key) {
trigger_error('The "_scheme" requirement is deprecated since version 2.2 and will be removed in 3.0. Use getSchemes() instead.', E_USER_DEPRECATED);
} elseif ('_method' === $key) {
trigger_error('The "_method" requirement is deprecated since version 2.2 and will be removed in 3.0. Use getMethods() instead.', E_USER_DEPRECATED);
}

return isset($this->requirements[$key]) ? $this->requirements[$key] : null;
}

Expand Down Expand Up @@ -649,8 +655,12 @@ private function sanitizeRequirement($key, $regex)

// this is to keep BC and will be removed in a future version
if ('_scheme' === $key) {
trigger_error('The "_scheme" requirement is deprecated since version 2.2 and will be removed in 3.0. Use the setSchemes() method instead or the "schemes" option in the route definition.', E_USER_DEPRECATED);

$this->setSchemes(explode('|', $regex));
} elseif ('_method' === $key) {
trigger_error('The "_method" requirement is deprecated since version 2.2 and will be removed in 3.0. Use the setMethods() method instead or the "methods" option in the route definition.', E_USER_DEPRECATED);

$this->setMethods(explode('|', $regex));
}

Expand Down
6 changes: 3 additions & 3 deletions src/Symfony/Component/Routing/Tests/Annotation/RouteTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,14 @@ public function getValidParameters()
{
return array(
array('value', '/Blog', 'getPath'),
array('requirements', array('_method' => 'GET'), 'getRequirements'),
array('requirements', array('locale' => 'en'), 'getRequirements'),
array('options', array('compiler_class' => 'RouteCompiler'), 'getOptions'),
array('name', 'blog_index', 'getName'),
array('defaults', array('_controller' => 'MyBlogBundle:Blog:index'), 'getDefaults'),
array('schemes', array('https'), 'getSchemes'),
array('methods', array('GET', 'POST'), 'getMethods'),
array('host', array('{locale}.example.com'), 'getHost'),
array('condition', array('context.getMethod() == "GET"'), 'getCondition'),
array('host', '{locale}.example.com', 'getHost'),
array('condition', 'context.getMethod() == "GET"', 'getCondition'),
);
}

Expand Down
16 changes: 16 additions & 0 deletions src/Symfony/Component/Routing/Tests/Fixtures/legacy_validpattern.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<?xml version="1.0" encoding="UTF-8" ?>

<routes xmlns="http://symfony.com/schema/routing"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://symfony.com/schema/routing http://symfony.com/schema/routing/routing-1.0.xsd">

<route id="blog_show_legacy" pattern="/blog/{slug}" host="{locale}.example.com">
<default key="_controller">MyBundle:Blog:show</default>
<default key="slug" xsi:nil="true" />
<requirement key="_method">GET|POST|put|OpTiOnS</requirement>
<requirement key="_scheme">hTTps</requirement>
<requirement key="locale">\w+</requirement>
<option key="compiler_class">RouteCompiler</option>
<condition>context.getMethod() == "GET"</condition>
</route>
</routes>
8 changes: 8 additions & 0 deletions src/Symfony/Component/Routing/Tests/Fixtures/legacy_validpattern.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
blog_show_legacy:
pattern: /blog/{slug}
defaults: { _controller: "MyBundle:Blog:show" }
host: "{locale}.example.com"
requirements: { '_method': 'GET|POST|put|OpTiOnS', _scheme: https, 'locale': '\w+' }
condition: 'context.getMethod() == "GET"'
options:
compiler_class: RouteCompiler
1 change: 0 additions & 1 deletion src/Symfony/Component/Routing/Tests/Fixtures/nonvalid.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,5 @@

<route id="blog_show" path="/blog/{slug}">
<default key="_controller">MyBundle:Blog:show</default>
<requirement key="_method">GET</requirement>
<!-- </route> -->
</routes>
1 change: 0 additions & 1 deletion src/Symfony/Component/Routing/Tests/Fixtures/nonvalidroute.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

<route id="blog_show" path="/blog/{slug}">
<default key="_controller">MyBundle:Blog:show</default>
<requirement key="_method">GET</requirement>
<option key="compiler_class">RouteCompiler</option>
<foo key="bar">baz</foo>
</route>
Expand Down
10 changes: 0 additions & 10 deletions src/Symfony/Component/Routing/Tests/Fixtures/validpattern.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,5 @@
array('GET', 'POST', 'put', 'OpTiOnS'),
'context.getMethod() == "GET"'
));
$collection->add('blog_show_legacy', new Route(
'/blog/{slug}',
array('_controller' => 'MyBlogBundle:Blog:show'),
array('_method' => 'GET|POST|put|OpTiOnS', '_scheme' => 'https', 'locale' => '\w+'),
array('compiler_class' => 'RouteCompiler'),
'{locale}.example.com',
array(),
array(),
'context.getMethod() == "GET"'
));

return $collection;
10 changes: 0 additions & 10 deletions src/Symfony/Component/Routing/Tests/Fixtures/validpattern.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,5 @@
<condition>context.getMethod() == "GET"</condition>
</route>

<route id="blog_show_legacy" pattern="/blog/{slug}" host="{locale}.example.com">
<default key="_controller">MyBundle:Blog:show</default>
<default key="slug" xsi:nil="true" />
<requirement key="_method">GET|POST|put|OpTiOnS</requirement>
<requirement key="_scheme">hTTps</requirement>
<requirement key="locale">\w+</requirement>
<option key="compiler_class">RouteCompiler</option>
<condition>context.getMethod() == "GET"</condition>
</route>

<route id="blog_show_inherited" path="/blog/{slug}" />
</routes>
9 changes: 0 additions & 9 deletions src/Symfony/Component/Routing/Tests/Fixtures/validpattern.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,5 @@ blog_show:
options:
compiler_class: RouteCompiler

blog_show_legacy:
pattern: /blog/{slug}
defaults: { _controller: "MyBundle:Blog:show" }
host: "{locale}.example.com"
requirements: { '_method': 'GET|POST|put|OpTiOnS', _scheme: https, 'locale': '\w+' }
condition: 'context.getMethod() == "GET"'
options:
compiler_class: RouteCompiler

blog_show_inherited:
path: /blog/{slug}
9 changes: 0 additions & 9 deletions src/Symfony/Component/Routing/Tests/Generator/Dumper/PhpGeneratorDumperTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,33 +118,24 @@ public function testDumpForRouteWithDefaults()
public function testDumpWithSchemeRequirement()
{
$this->routeCollection->add('Test1', new Route('/testing', array(), array(), array(), '', array('ftp', 'https')));
$this->routeCollection->add('Test2', new Route('/testing_bc', array(), array('_scheme' => 'https'))); // BC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of these tests are useless because we already tested that logic inside the Route class that _scheme/_methods is backwards compatible and will result in the same route representation.

file_put_contents($this->testTmpFilepath, $this->generatorDumper->dump(array('class' => 'SchemeUrlGenerator')));
include $this->testTmpFilepath;

$projectUrlGenerator = new \SchemeUrlGenerator(new RequestContext('/app.php'));

$absoluteUrl = $projectUrlGenerator->generate('Test1', array(), true);
$absoluteUrlBC = $projectUrlGenerator->generate('Test2', array(), true);
$relativeUrl = $projectUrlGenerator->generate('Test1', array(), false);
$relativeUrlBC = $projectUrlGenerator->generate('Test2', array(), false);

$this->assertEquals($absoluteUrl, 'ftp://localhost/app.php/testing');
$this->assertEquals($absoluteUrlBC, 'https://localhost/app.php/testing_bc');
$this->assertEquals($relativeUrl, 'ftp://localhost/app.php/testing');
$this->assertEquals($relativeUrlBC, 'https://localhost/app.php/testing_bc');

$projectUrlGenerator = new \SchemeUrlGenerator(new RequestContext('/app.php', 'GET', 'localhost', 'https'));

$absoluteUrl = $projectUrlGenerator->generate('Test1', array(), true);
$absoluteUrlBC = $projectUrlGenerator->generate('Test2', array(), true);
$relativeUrl = $projectUrlGenerator->generate('Test1', array(), false);
$relativeUrlBC = $projectUrlGenerator->generate('Test2', array(), false);

$this->assertEquals($absoluteUrl, 'https://localhost/app.php/testing');
$this->assertEquals($absoluteUrlBC, 'https://localhost/app.php/testing_bc');
$this->assertEquals($relativeUrl, '/app.php/testing');
$this->assertEquals($relativeUrlBC, '/app.php/testing_bc');
}
}
Loading