From 358e9c47b995cc103aa5706269db22ac637c56b2 Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Sun, 2 Dec 2012 15:36:36 +0100 Subject: [PATCH 01/10] fix some phpdoc --- src/Symfony/Component/Routing/Loader/ClosureLoader.php | 3 +++ src/Symfony/Component/Routing/Loader/PhpFileLoader.php | 5 ++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/Symfony/Component/Routing/Loader/ClosureLoader.php b/src/Symfony/Component/Routing/Loader/ClosureLoader.php index 0ec442d87d..8212c2916d 100644 --- a/src/Symfony/Component/Routing/Loader/ClosureLoader.php +++ b/src/Symfony/Component/Routing/Loader/ClosureLoader.php @@ -12,6 +12,7 @@ namespace Symfony\Component\Routing\Loader; use Symfony\Component\Config\Loader\Loader; +use Symfony\Component\Routing\RouteCollection; /** * ClosureLoader loads routes from a PHP closure. @@ -30,6 +31,8 @@ class ClosureLoader extends Loader * @param \Closure $closure A Closure * @param string|null $type The resource type * + * @return RouteCollection A RouteCollection instance + * * @api */ public function load($closure, $type = null) diff --git a/src/Symfony/Component/Routing/Loader/PhpFileLoader.php b/src/Symfony/Component/Routing/Loader/PhpFileLoader.php index b93dd255b9..98b7efbf47 100644 --- a/src/Symfony/Component/Routing/Loader/PhpFileLoader.php +++ b/src/Symfony/Component/Routing/Loader/PhpFileLoader.php @@ -11,8 +11,9 @@ namespace Symfony\Component\Routing\Loader; -use Symfony\Component\Config\Resource\FileResource; use Symfony\Component\Config\Loader\FileLoader; +use Symfony\Component\Config\Resource\FileResource; +use Symfony\Component\Routing\RouteCollection; /** * PhpFileLoader loads routes from a PHP file. @@ -31,6 +32,8 @@ class PhpFileLoader extends FileLoader * @param string $file A PHP file path * @param string|null $type The resource type * + * @return RouteCollection A RouteCollection instance + * * @api */ public function load($file, $type = null) From 51fbffec2604d53e6220cba3fbea4eea7fa858c2 Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Sun, 2 Dec 2012 18:47:28 +0100 Subject: [PATCH 02/10] remove unneeded cast --- src/Symfony/Component/Routing/Loader/XmlFileLoader.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Symfony/Component/Routing/Loader/XmlFileLoader.php b/src/Symfony/Component/Routing/Loader/XmlFileLoader.php index d5a7bf3f93..1885c96ffd 100644 --- a/src/Symfony/Component/Routing/Loader/XmlFileLoader.php +++ b/src/Symfony/Component/Routing/Loader/XmlFileLoader.php @@ -155,13 +155,13 @@ class XmlFileLoader extends FileLoader switch ($node->tagName) { case 'default': - $defaults[$node->getAttribute('key')] = trim((string) $node->nodeValue); + $defaults[$node->getAttribute('key')] = trim($node->nodeValue); break; case 'option': - $options[$node->getAttribute('key')] = trim((string) $node->nodeValue); + $options[$node->getAttribute('key')] = trim($node->nodeValue); break; case 'requirement': - $requirements[$node->getAttribute('key')] = trim((string) $node->nodeValue); + $requirements[$node->getAttribute('key')] = trim($node->nodeValue); break; default: throw new \InvalidArgumentException(sprintf('Unable to parse tag "%s"', $node->tagName)); From 02e01b9798ed288ca41eab91edef088a2c5c966f Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Mon, 3 Dec 2012 00:18:31 +0100 Subject: [PATCH 03/10] improve exception messages in xml loader --- src/Symfony/Component/Routing/Loader/XmlFileLoader.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Symfony/Component/Routing/Loader/XmlFileLoader.php b/src/Symfony/Component/Routing/Loader/XmlFileLoader.php index 1885c96ffd..8e1a17814a 100644 --- a/src/Symfony/Component/Routing/Loader/XmlFileLoader.php +++ b/src/Symfony/Component/Routing/Loader/XmlFileLoader.php @@ -100,7 +100,7 @@ class XmlFileLoader extends FileLoader $options[$n->getAttribute('key')] = trim($n->nodeValue); break; default: - throw new \InvalidArgumentException(sprintf('Unable to parse tag "%s"', $n->tagName)); + throw new \InvalidArgumentException(sprintf('Unknown tag "%s" used in file "%s". Expected "default", "requirement" or "option".', $n->tagName, $file)); } } @@ -119,7 +119,7 @@ class XmlFileLoader extends FileLoader $collection->addCollection($subCollection); break; default: - throw new \InvalidArgumentException(sprintf('Unable to parse tag "%s"', $node->tagName)); + throw new \InvalidArgumentException(sprintf('Unknown tag "%s" used in file "%s". Expected "route" or "import".', $node->tagName, $file)); } } @@ -164,7 +164,7 @@ class XmlFileLoader extends FileLoader $requirements[$node->getAttribute('key')] = trim($node->nodeValue); break; default: - throw new \InvalidArgumentException(sprintf('Unable to parse tag "%s"', $node->tagName)); + throw new \InvalidArgumentException(sprintf('Unknown tag "%s" used in file "%s". Expected "default", "requirement" or "option".', $node->tagName, $file)); } } From 1a60a3ceb58b9c70b9a42af1f67b3ae4dce25adf Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Mon, 3 Dec 2012 10:20:00 +0100 Subject: [PATCH 04/10] make resource and key attributes required in xsd --- .../Component/Routing/Loader/schema/routing/routing-1.0.xsd | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Symfony/Component/Routing/Loader/schema/routing/routing-1.0.xsd b/src/Symfony/Component/Routing/Loader/schema/routing/routing-1.0.xsd index 5346237edb..c9bd3d6a6d 100644 --- a/src/Symfony/Component/Routing/Loader/schema/routing/routing-1.0.xsd +++ b/src/Symfony/Component/Routing/Loader/schema/routing/routing-1.0.xsd @@ -43,7 +43,7 @@ - + @@ -52,7 +52,7 @@ - + From 83fc5ff72f1532fbc4d966beb8677db04935d3ba Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Mon, 3 Dec 2012 11:37:45 +0100 Subject: [PATCH 05/10] fix namespace handling in xml loader; it could not handle prefixes --- .../Routing/Loader/XmlFileLoader.php | 47 +++++++++---------- 1 file changed, 22 insertions(+), 25 deletions(-) diff --git a/src/Symfony/Component/Routing/Loader/XmlFileLoader.php b/src/Symfony/Component/Routing/Loader/XmlFileLoader.php index 8e1a17814a..9415619416 100644 --- a/src/Symfony/Component/Routing/Loader/XmlFileLoader.php +++ b/src/Symfony/Component/Routing/Loader/XmlFileLoader.php @@ -25,6 +25,9 @@ use Symfony\Component\Config\Loader\FileLoader; */ class XmlFileLoader extends FileLoader { + const NAMESPACE_URI = 'http://symfony.com/schema/routing'; + const SCHEME_PATH = '/schema/routing/routing-1.0.xsd'; + /** * Loads an XML file. * @@ -70,7 +73,11 @@ class XmlFileLoader extends FileLoader */ protected function parseNode(RouteCollection $collection, \DOMElement $node, $path, $file) { - switch ($node->tagName) { + if (self::NAMESPACE_URI !== $node->namespaceURI) { + return; + } + + switch ($node->localName) { case 'route': $this->parseRoute($collection, $node, $path); break; @@ -84,23 +91,19 @@ class XmlFileLoader extends FileLoader $requirements = array(); $options = array(); - foreach ($node->childNodes as $n) { - if (!$n instanceof \DOMElement) { - continue; - } - - switch ($n->tagName) { + foreach ($node->getElementsByTagNameNS(self::NAMESPACE_URI, '*') as $n) { + switch ($n->localName) { case 'default': - $defaults[$n->getAttribute('key')] = trim($n->nodeValue); + $defaults[$n->getAttribute('key')] = trim($n->textContent); break; case 'requirement': - $requirements[$n->getAttribute('key')] = trim($n->nodeValue); + $requirements[$n->getAttribute('key')] = trim($n->textContent); break; case 'option': - $options[$n->getAttribute('key')] = trim($n->nodeValue); + $options[$n->getAttribute('key')] = trim($n->textContent); break; default: - throw new \InvalidArgumentException(sprintf('Unknown tag "%s" used in file "%s". Expected "default", "requirement" or "option".', $n->tagName, $file)); + throw new \InvalidArgumentException(sprintf('Unknown tag "%s" used in file "%s". Expected "default", "requirement" or "option".', $n->localName, $file)); } } @@ -119,7 +122,7 @@ class XmlFileLoader extends FileLoader $collection->addCollection($subCollection); break; default: - throw new \InvalidArgumentException(sprintf('Unknown tag "%s" used in file "%s". Expected "route" or "import".', $node->tagName, $file)); + throw new \InvalidArgumentException(sprintf('Unknown tag "%s" used in file "%s". Expected "route" or "import".', $node->localName, $file)); } } @@ -148,23 +151,19 @@ class XmlFileLoader extends FileLoader $requirements = array(); $options = array(); - foreach ($definition->childNodes as $node) { - if (!$node instanceof \DOMElement) { - continue; - } - - switch ($node->tagName) { + foreach ($definition->getElementsByTagNameNS(self::NAMESPACE_URI, '*') as $node) { + switch ($node->localName) { case 'default': - $defaults[$node->getAttribute('key')] = trim($node->nodeValue); + $defaults[$node->getAttribute('key')] = trim($node->textContent); break; case 'option': - $options[$node->getAttribute('key')] = trim($node->nodeValue); + $options[$node->getAttribute('key')] = trim($node->textContent); break; case 'requirement': - $requirements[$node->getAttribute('key')] = trim($node->nodeValue); + $requirements[$node->getAttribute('key')] = trim($node->textContent); break; default: - throw new \InvalidArgumentException(sprintf('Unknown tag "%s" used in file "%s". Expected "default", "requirement" or "option".', $node->tagName, $file)); + throw new \InvalidArgumentException(sprintf('Unknown tag "%s" used in file "%s". Expected "default", "requirement" or "option".', $node->localName, $file)); } } @@ -220,12 +219,10 @@ class XmlFileLoader extends FileLoader */ protected function validate(\DOMDocument $dom) { - $location = __DIR__.'/schema/routing/routing-1.0.xsd'; - $current = libxml_use_internal_errors(true); libxml_clear_errors(); - if (!$dom->schemaValidate($location)) { + if (!$dom->schemaValidate(__DIR__ . static::SCHEME_PATH)) { throw new \InvalidArgumentException(implode("\n", $this->getXmlErrors($current))); } libxml_use_internal_errors($current); From 8361b5a58bbb90b5b36f5ba15694fb83d5ecca0d Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Mon, 3 Dec 2012 12:51:35 +0100 Subject: [PATCH 06/10] refactor the XMlFileLoader --- .../Routing/Loader/XmlFileLoader.php | 156 ++++++++++-------- 1 file changed, 86 insertions(+), 70 deletions(-) diff --git a/src/Symfony/Component/Routing/Loader/XmlFileLoader.php b/src/Symfony/Component/Routing/Loader/XmlFileLoader.php index 9415619416..109b883d3b 100644 --- a/src/Symfony/Component/Routing/Loader/XmlFileLoader.php +++ b/src/Symfony/Component/Routing/Loader/XmlFileLoader.php @@ -20,6 +20,7 @@ use Symfony\Component\Config\Loader\FileLoader; * XmlFileLoader loads XML routing files. * * @author Fabien Potencier + * @author Tobias Schultze * * @api */ @@ -36,7 +37,8 @@ class XmlFileLoader extends FileLoader * * @return RouteCollection A RouteCollection instance * - * @throws \InvalidArgumentException When a tag can't be parsed + * @throws \InvalidArgumentException When the file cannot be loaded or when the XML cannot be + * parsed because it does not validate against the scheme. * * @api */ @@ -64,12 +66,12 @@ class XmlFileLoader extends FileLoader /** * Parses a node from a loaded XML file. * - * @param RouteCollection $collection the collection to associate with the node - * @param \DOMElement $node the node to parse - * @param string $path the path of the XML file being processed - * @param string $file + * @param RouteCollection $collection Collection to associate with the node + * @param \DOMElement $node Element to parse + * @param string $path Full path of the XML file being processed + * @param string $file Loaded file name * - * @throws \InvalidArgumentException When a tag can't be parsed + * @throws \InvalidArgumentException When the XML is invalid */ protected function parseNode(RouteCollection $collection, \DOMElement $node, $path, $file) { @@ -82,47 +84,10 @@ class XmlFileLoader extends FileLoader $this->parseRoute($collection, $node, $path); break; case 'import': - $resource = $node->getAttribute('resource'); - $type = $node->getAttribute('type'); - $prefix = $node->getAttribute('prefix'); - $hostnamePattern = $node->hasAttribute('hostname-pattern') ? $node->getAttribute('hostname-pattern') : null; - - $defaults = array(); - $requirements = array(); - $options = array(); - - foreach ($node->getElementsByTagNameNS(self::NAMESPACE_URI, '*') as $n) { - switch ($n->localName) { - case 'default': - $defaults[$n->getAttribute('key')] = trim($n->textContent); - break; - case 'requirement': - $requirements[$n->getAttribute('key')] = trim($n->textContent); - break; - case 'option': - $options[$n->getAttribute('key')] = trim($n->textContent); - break; - default: - throw new \InvalidArgumentException(sprintf('Unknown tag "%s" used in file "%s". Expected "default", "requirement" or "option".', $n->localName, $file)); - } - } - - $this->setCurrentDir(dirname($path)); - - $subCollection = $this->import($resource, ('' !== $type ? $type : null), false, $file); - /* @var $subCollection RouteCollection */ - $subCollection->addPrefix($prefix); - if (null !== $hostnamePattern) { - $subCollection->setHostnamePattern($hostnamePattern); - } - $subCollection->addDefaults($defaults); - $subCollection->addRequirements($requirements); - $subCollection->addOptions($options); - - $collection->addCollection($subCollection); + $this->parseImport($collection, $node, $path, $file); break; default: - throw new \InvalidArgumentException(sprintf('Unknown tag "%s" used in file "%s". Expected "route" or "import".', $node->localName, $file)); + throw new \InvalidArgumentException(sprintf('Unknown tag "%s" used in file "%s". Expected "route" or "import".', $node->localName, $path)); } } @@ -139,37 +104,53 @@ class XmlFileLoader extends FileLoader /** * Parses a route and adds it to the RouteCollection. * - * @param RouteCollection $collection A RouteCollection instance - * @param \DOMElement $definition Route definition - * @param string $file An XML file path + * @param RouteCollection $collection RouteCollection instance + * @param \DOMElement $node Element to parse that represents a Route + * @param string $path Full path of the XML file being processed * - * @throws \InvalidArgumentException When the definition cannot be parsed + * @throws \InvalidArgumentException When the XML is invalid */ - protected function parseRoute(RouteCollection $collection, \DOMElement $definition, $file) + protected function parseRoute(RouteCollection $collection, \DOMElement $node, $path) { - $defaults = array(); - $requirements = array(); - $options = array(); + list($defaults, $requirements, $options) = $this->parseConfigs($node, $path); - foreach ($definition->getElementsByTagNameNS(self::NAMESPACE_URI, '*') as $node) { - switch ($node->localName) { - case 'default': - $defaults[$node->getAttribute('key')] = trim($node->textContent); - break; - case 'option': - $options[$node->getAttribute('key')] = trim($node->textContent); - break; - case 'requirement': - $requirements[$node->getAttribute('key')] = trim($node->textContent); - break; - default: - throw new \InvalidArgumentException(sprintf('Unknown tag "%s" used in file "%s". Expected "default", "requirement" or "option".', $node->localName, $file)); - } + $route = new Route($node->getAttribute('pattern'), $defaults, $requirements, $options, $node->getAttribute('hostname-pattern')); + + $collection->add($node->getAttribute('id'), $route); + } + + /** + * Parses an import and adds the routes in the resource to the RouteCollection. + * + * @param RouteCollection $collection RouteCollection instance + * @param \DOMElement $node Element to parse that represents a Route + * @param string $path Full path of the XML file being processed + * @param string $file Loaded file name + * + * @throws \InvalidArgumentException When the XML is invalid + */ + protected function parseImport(RouteCollection $collection, \DOMElement $node, $path, $file) + { + $resource = $node->getAttribute('resource'); + $type = $node->getAttribute('type'); + $prefix = $node->getAttribute('prefix'); + $hostnamePattern = $node->hasAttribute('hostname-pattern') ? $node->getAttribute('hostname-pattern') : null; + + list($defaults, $requirements, $options) = $this->parseConfigs($node, $path); + + $this->setCurrentDir(dirname($path)); + + $subCollection = $this->import($resource, ('' !== $type ? $type : null), false, $file); + /* @var $subCollection RouteCollection */ + $subCollection->addPrefix($prefix); + if (null !== $hostnamePattern) { + $subCollection->setHostnamePattern($hostnamePattern); } + $subCollection->addDefaults($defaults); + $subCollection->addRequirements($requirements); + $subCollection->addOptions($options); - $route = new Route($definition->getAttribute('pattern'), $defaults, $requirements, $options, $definition->getAttribute('hostname-pattern')); - - $collection->add($definition->getAttribute('id'), $route); + $collection->addCollection($subCollection); } /** @@ -252,4 +233,39 @@ class XmlFileLoader extends FileLoader return $errors; } + + /** + * Parses the config elements (default, requirement, option). + * + * @param \DOMElement $node Element to parse that contains the configs + * @param string $path Full path of the XML file being processed + * + * @return array An array with the defaults as first item, requirements as second and options as third. + * + * @throws \InvalidArgumentException When the XML is invalid + */ + private function parseConfigs(\DOMElement $node, $path) + { + $defaults = array(); + $requirements = array(); + $options = array(); + + foreach ($node->getElementsByTagNameNS(self::NAMESPACE_URI, '*') as $n) { + switch ($n->localName) { + case 'default': + $defaults[$n->getAttribute('key')] = trim($n->textContent); + break; + case 'requirement': + $requirements[$n->getAttribute('key')] = trim($n->textContent); + break; + case 'option': + $options[$n->getAttribute('key')] = trim($n->textContent); + break; + default: + throw new \InvalidArgumentException(sprintf('Unknown tag "%s" used in file "%s". Expected "default", "requirement" or "option".', $n->localName, $path)); + } + } + + return array($defaults, $requirements, $options); + } } From b20d6a79600225f3867abe934c04ff6419075f01 Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Mon, 3 Dec 2012 13:03:01 +0100 Subject: [PATCH 07/10] ensure id, pattern and resource are specified the yaml loader also requires the pattern to be given, so the xml loader should do the same --- .../Component/Routing/Loader/XmlFileLoader.php | 12 +++++++++--- .../Routing/Loader/schema/routing/routing-1.0.xsd | 2 +- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/Symfony/Component/Routing/Loader/XmlFileLoader.php b/src/Symfony/Component/Routing/Loader/XmlFileLoader.php index 109b883d3b..30fc1dac6a 100644 --- a/src/Symfony/Component/Routing/Loader/XmlFileLoader.php +++ b/src/Symfony/Component/Routing/Loader/XmlFileLoader.php @@ -112,11 +112,14 @@ class XmlFileLoader extends FileLoader */ protected function parseRoute(RouteCollection $collection, \DOMElement $node, $path) { + if ('' === ($id = $node->getAttribute('id')) || !$node->hasAttribute('pattern')) { + throw new \InvalidArgumentException(sprintf('The element in file "%s" must have an "id" and a "pattern" attribute.', $path)); + } + list($defaults, $requirements, $options) = $this->parseConfigs($node, $path); $route = new Route($node->getAttribute('pattern'), $defaults, $requirements, $options, $node->getAttribute('hostname-pattern')); - - $collection->add($node->getAttribute('id'), $route); + $collection->add($id, $route); } /** @@ -131,7 +134,10 @@ class XmlFileLoader extends FileLoader */ protected function parseImport(RouteCollection $collection, \DOMElement $node, $path, $file) { - $resource = $node->getAttribute('resource'); + if ('' === $resource = $node->getAttribute('resource')) { + throw new \InvalidArgumentException(sprintf('The element in file "%s" must have a "resource" attribute.', $path)); + } + $type = $node->getAttribute('type'); $prefix = $node->getAttribute('prefix'); $hostnamePattern = $node->hasAttribute('hostname-pattern') ? $node->getAttribute('hostname-pattern') : null; diff --git a/src/Symfony/Component/Routing/Loader/schema/routing/routing-1.0.xsd b/src/Symfony/Component/Routing/Loader/schema/routing/routing-1.0.xsd index c9bd3d6a6d..3143b3ac72 100644 --- a/src/Symfony/Component/Routing/Loader/schema/routing/routing-1.0.xsd +++ b/src/Symfony/Component/Routing/Loader/schema/routing/routing-1.0.xsd @@ -36,7 +36,7 @@ - + From 45987eb2b7e12a28590ad3de807d68bac65f692c Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Fri, 7 Dec 2012 17:24:28 +0100 Subject: [PATCH 08/10] added tests for the previous XmlFileLoader fixes --- .../Component/Routing/Loader/XmlFileLoader.php | 4 +++- .../Routing/Tests/Fixtures/missing_id.xml | 8 ++++++++ .../Routing/Tests/Fixtures/missing_path.xml | 8 ++++++++ .../Routing/Tests/Fixtures/namespaceprefix.xml | 13 +++++++++++++ .../Routing/Tests/Loader/XmlFileLoaderTest.php | 17 ++++++++++++++++- 5 files changed, 48 insertions(+), 2 deletions(-) create mode 100644 src/Symfony/Component/Routing/Tests/Fixtures/missing_id.xml create mode 100644 src/Symfony/Component/Routing/Tests/Fixtures/missing_path.xml create mode 100644 src/Symfony/Component/Routing/Tests/Fixtures/namespaceprefix.xml diff --git a/src/Symfony/Component/Routing/Loader/XmlFileLoader.php b/src/Symfony/Component/Routing/Loader/XmlFileLoader.php index 30fc1dac6a..a07c7f80a5 100644 --- a/src/Symfony/Component/Routing/Loader/XmlFileLoader.php +++ b/src/Symfony/Component/Routing/Loader/XmlFileLoader.php @@ -166,7 +166,9 @@ class XmlFileLoader extends FileLoader * * @return \DOMDocument * - * @throws \InvalidArgumentException When loading of XML file returns error + * @throws \InvalidArgumentException When loading of XML file fails because of syntax errors + * or when the XML structure is not as expected by the scheme - + * see validate() */ protected function loadFile($file) { diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/missing_id.xml b/src/Symfony/Component/Routing/Tests/Fixtures/missing_id.xml new file mode 100644 index 0000000000..52719be167 --- /dev/null +++ b/src/Symfony/Component/Routing/Tests/Fixtures/missing_id.xml @@ -0,0 +1,8 @@ + + + + + + diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/missing_path.xml b/src/Symfony/Component/Routing/Tests/Fixtures/missing_path.xml new file mode 100644 index 0000000000..a7a8e7b39b --- /dev/null +++ b/src/Symfony/Component/Routing/Tests/Fixtures/missing_path.xml @@ -0,0 +1,8 @@ + + + + + + \ No newline at end of file diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/namespaceprefix.xml b/src/Symfony/Component/Routing/Tests/Fixtures/namespaceprefix.xml new file mode 100644 index 0000000000..efc4da9b37 --- /dev/null +++ b/src/Symfony/Component/Routing/Tests/Fixtures/namespaceprefix.xml @@ -0,0 +1,13 @@ + + + + + + MyBundle:Blog:show + \w+ + en|fr|de + RouteCompiler + + diff --git a/src/Symfony/Component/Routing/Tests/Loader/XmlFileLoaderTest.php b/src/Symfony/Component/Routing/Tests/Loader/XmlFileLoaderTest.php index 235a9d13af..993a121071 100644 --- a/src/Symfony/Component/Routing/Tests/Loader/XmlFileLoaderTest.php +++ b/src/Symfony/Component/Routing/Tests/Loader/XmlFileLoaderTest.php @@ -56,6 +56,21 @@ class XmlFileLoaderTest extends \PHPUnit_Framework_TestCase $this->assertEquals('RouteCompiler', $route->getOption('compiler_class')); } + public function testLoadWithNamespacePrefix() + { + $loader = new XmlFileLoader(new FileLocator(array(__DIR__.'/../Fixtures'))); + $routeCollection = $loader->load('namespaceprefix.xml'); + + $this->assertCount(1, $routeCollection, 'One route is loaded'); + $route = $routeCollection->get('blog_show'); + $this->assertEquals('/blog/{slug}', $route->getPattern()); + $this->assertEquals('MyBundle:Blog:show', $route->getDefault('_controller')); + $this->assertEquals('\w+', $route->getRequirement('slug')); + $this->assertEquals('en|fr|de', $route->getRequirement('_locale')); + $this->assertEquals('{_locale}.example.com', $route->getHostnamePattern()); + $this->assertEquals('RouteCompiler', $route->getOption('compiler_class')); + } + public function testLoadWithImport() { $loader = new XmlFileLoader(new FileLocator(array(__DIR__.'/../Fixtures'))); @@ -94,7 +109,7 @@ class XmlFileLoaderTest extends \PHPUnit_Framework_TestCase public function getPathsToInvalidFiles() { - return array(array('nonvalidnode.xml'), array('nonvalidroute.xml'), array('nonvalid.xml')); + return array(array('nonvalidnode.xml'), array('nonvalidroute.xml'), array('nonvalid.xml'), array('missing_id.xml'), array('missing_path.xml')); } /** From 392d785d912542ffcdab66778f2a35df73a35705 Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Fri, 7 Dec 2012 17:40:18 +0100 Subject: [PATCH 09/10] removed covers annotation from loader tests and unneeded use statements --- .../Routing/Tests/Loader/AnnotationClassLoaderTest.php | 6 ------ .../Routing/Tests/Loader/AnnotationDirectoryLoaderTest.php | 3 --- .../Routing/Tests/Loader/AnnotationFileLoaderTest.php | 3 --- .../Component/Routing/Tests/Loader/ClosureLoaderTest.php | 6 ------ .../Component/Routing/Tests/Loader/PhpFileLoaderTest.php | 4 ---- .../Component/Routing/Tests/Loader/XmlFileLoaderTest.php | 4 ---- .../Component/Routing/Tests/Loader/YamlFileLoaderTest.php | 4 ---- 7 files changed, 30 deletions(-) diff --git a/src/Symfony/Component/Routing/Tests/Loader/AnnotationClassLoaderTest.php b/src/Symfony/Component/Routing/Tests/Loader/AnnotationClassLoaderTest.php index 002d7449b8..42bad2df32 100644 --- a/src/Symfony/Component/Routing/Tests/Loader/AnnotationClassLoaderTest.php +++ b/src/Symfony/Component/Routing/Tests/Loader/AnnotationClassLoaderTest.php @@ -11,8 +11,6 @@ namespace Symfony\Component\Routing\Tests\Loader; -use Symfony\Component\Routing\Loader\AnnotationClassLoader; - class AnnotationClassLoaderTest extends AbstractAnnotationLoaderTest { protected $loader; @@ -42,7 +40,6 @@ class AnnotationClassLoaderTest extends AbstractAnnotationLoaderTest } /** - * @covers Symfony\Component\Routing\Loader\AnnotationClassLoader::supports * @dataProvider provideTestSupportsChecksResource */ public function testSupportsChecksResource($resource, $expectedSupports) @@ -63,9 +60,6 @@ class AnnotationClassLoaderTest extends AbstractAnnotationLoaderTest ); } - /** - * @covers Symfony\Component\Routing\Loader\AnnotationClassLoader::supports - */ public function testSupportsChecksTypeIfSpecified() { $this->assertTrue($this->loader->supports('class', 'annotation'), '->supports() checks the resource type if specified'); diff --git a/src/Symfony/Component/Routing/Tests/Loader/AnnotationDirectoryLoaderTest.php b/src/Symfony/Component/Routing/Tests/Loader/AnnotationDirectoryLoaderTest.php index a8ac733120..29126ba4f2 100644 --- a/src/Symfony/Component/Routing/Tests/Loader/AnnotationDirectoryLoaderTest.php +++ b/src/Symfony/Component/Routing/Tests/Loader/AnnotationDirectoryLoaderTest.php @@ -40,9 +40,6 @@ class AnnotationDirectoryLoaderTest extends AbstractAnnotationLoaderTest $this->loader->load(__DIR__.'/../Fixtures/AnnotatedClasses'); } - /** - * @covers Symfony\Component\Routing\Loader\AnnotationDirectoryLoader::supports - */ public function testSupports() { $fixturesDir = __DIR__.'/../Fixtures'; diff --git a/src/Symfony/Component/Routing/Tests/Loader/AnnotationFileLoaderTest.php b/src/Symfony/Component/Routing/Tests/Loader/AnnotationFileLoaderTest.php index 02d0576dbb..f0a8a0e329 100644 --- a/src/Symfony/Component/Routing/Tests/Loader/AnnotationFileLoaderTest.php +++ b/src/Symfony/Component/Routing/Tests/Loader/AnnotationFileLoaderTest.php @@ -34,9 +34,6 @@ class AnnotationFileLoaderTest extends AbstractAnnotationLoaderTest $this->loader->load(__DIR__.'/../Fixtures/AnnotatedClasses/FooClass.php'); } - /** - * @covers Symfony\Component\Routing\Loader\AnnotationFileLoader::supports - */ public function testSupports() { $fixture = __DIR__.'/../Fixtures/annotated.php'; diff --git a/src/Symfony/Component/Routing/Tests/Loader/ClosureLoaderTest.php b/src/Symfony/Component/Routing/Tests/Loader/ClosureLoaderTest.php index d9e3ff50d7..64d1b086ef 100644 --- a/src/Symfony/Component/Routing/Tests/Loader/ClosureLoaderTest.php +++ b/src/Symfony/Component/Routing/Tests/Loader/ClosureLoaderTest.php @@ -24,9 +24,6 @@ class ClosureLoaderTest extends \PHPUnit_Framework_TestCase } } - /** - * @covers Symfony\Component\Routing\Loader\ClosureLoader::supports - */ public function testSupports() { $loader = new ClosureLoader(); @@ -40,9 +37,6 @@ class ClosureLoaderTest extends \PHPUnit_Framework_TestCase $this->assertFalse($loader->supports($closure, 'foo'), '->supports() checks the resource type if specified'); } - /** - * @covers Symfony\Component\Routing\Loader\ClosureLoader::load - */ public function testLoad() { $loader = new ClosureLoader(); diff --git a/src/Symfony/Component/Routing/Tests/Loader/PhpFileLoaderTest.php b/src/Symfony/Component/Routing/Tests/Loader/PhpFileLoaderTest.php index 0ea7b32e17..3ff425d981 100644 --- a/src/Symfony/Component/Routing/Tests/Loader/PhpFileLoaderTest.php +++ b/src/Symfony/Component/Routing/Tests/Loader/PhpFileLoaderTest.php @@ -13,7 +13,6 @@ namespace Symfony\Component\Routing\Tests\Loader; use Symfony\Component\Config\FileLocator; use Symfony\Component\Routing\Loader\PhpFileLoader; -use Symfony\Component\Routing\Route; class PhpFileLoaderTest extends \PHPUnit_Framework_TestCase { @@ -24,9 +23,6 @@ class PhpFileLoaderTest extends \PHPUnit_Framework_TestCase } } - /** - * @covers Symfony\Component\Routing\Loader\PhpFileLoader::supports - */ public function testSupports() { $loader = new PhpFileLoader($this->getMock('Symfony\Component\Config\FileLocator')); diff --git a/src/Symfony/Component/Routing/Tests/Loader/XmlFileLoaderTest.php b/src/Symfony/Component/Routing/Tests/Loader/XmlFileLoaderTest.php index 993a121071..17e33ef297 100644 --- a/src/Symfony/Component/Routing/Tests/Loader/XmlFileLoaderTest.php +++ b/src/Symfony/Component/Routing/Tests/Loader/XmlFileLoaderTest.php @@ -13,7 +13,6 @@ namespace Symfony\Component\Routing\Tests\Loader; use Symfony\Component\Config\FileLocator; use Symfony\Component\Routing\Loader\XmlFileLoader; -use Symfony\Component\Routing\Route; use Symfony\Component\Routing\Tests\Fixtures\CustomXmlFileLoader; class XmlFileLoaderTest extends \PHPUnit_Framework_TestCase @@ -25,9 +24,6 @@ class XmlFileLoaderTest extends \PHPUnit_Framework_TestCase } } - /** - * @covers Symfony\Component\Routing\Loader\XmlFileLoader::supports - */ public function testSupports() { $loader = new XmlFileLoader($this->getMock('Symfony\Component\Config\FileLocator')); diff --git a/src/Symfony/Component/Routing/Tests/Loader/YamlFileLoaderTest.php b/src/Symfony/Component/Routing/Tests/Loader/YamlFileLoaderTest.php index e2e0f98eeb..d28a499642 100644 --- a/src/Symfony/Component/Routing/Tests/Loader/YamlFileLoaderTest.php +++ b/src/Symfony/Component/Routing/Tests/Loader/YamlFileLoaderTest.php @@ -13,7 +13,6 @@ namespace Symfony\Component\Routing\Tests\Loader; use Symfony\Component\Config\FileLocator; use Symfony\Component\Routing\Loader\YamlFileLoader; -use Symfony\Component\Routing\Route; use Symfony\Component\Config\Resource\FileResource; class YamlFileLoaderTest extends \PHPUnit_Framework_TestCase @@ -29,9 +28,6 @@ class YamlFileLoaderTest extends \PHPUnit_Framework_TestCase } } - /** - * @covers Symfony\Component\Routing\Loader\YamlFileLoader::supports - */ public function testSupports() { $loader = new YamlFileLoader($this->getMock('Symfony\Component\Config\FileLocator')); From 237bbd057869206a2ffba2c5da209ab672b79bb3 Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Fri, 7 Dec 2012 19:10:43 +0100 Subject: [PATCH 10/10] fixed and refactored YamlFileLoader in the same sense as the XmlFileLoader some nonsesense configs were not validated correctly like an imported resource with a pattern key. added tests for such things too. --- .../Routing/Loader/YamlFileLoader.php | 124 +++++++++++------- .../Fixtures/nonesense_resource_plus_path.yml | 3 + .../nonesense_type_without_resource.yml | 3 + .../Routing/Tests/Fixtures/nonvalid2.yml | 1 + .../Tests/Loader/YamlFileLoaderTest.php | 31 +---- 5 files changed, 87 insertions(+), 75 deletions(-) create mode 100644 src/Symfony/Component/Routing/Tests/Fixtures/nonesense_resource_plus_path.yml create mode 100644 src/Symfony/Component/Routing/Tests/Fixtures/nonesense_type_without_resource.yml create mode 100644 src/Symfony/Component/Routing/Tests/Fixtures/nonvalid2.yml diff --git a/src/Symfony/Component/Routing/Loader/YamlFileLoader.php b/src/Symfony/Component/Routing/Loader/YamlFileLoader.php index 326414e78e..7a5e268b27 100644 --- a/src/Symfony/Component/Routing/Loader/YamlFileLoader.php +++ b/src/Symfony/Component/Routing/Loader/YamlFileLoader.php @@ -21,13 +21,14 @@ use Symfony\Component\Config\Loader\FileLoader; * YamlFileLoader loads Yaml routing files. * * @author Fabien Potencier + * @author Tobias Schultze * * @api */ class YamlFileLoader extends FileLoader { private static $availableKeys = array( - 'type', 'resource', 'prefix', 'pattern', 'options', 'defaults', 'requirements', 'hostname_pattern', + 'resource', 'type', 'prefix', 'pattern', 'hostname_pattern', 'defaults', 'requirements', 'options', ); /** @@ -38,7 +39,7 @@ class YamlFileLoader extends FileLoader * * @return RouteCollection A RouteCollection instance * - * @throws \InvalidArgumentException When route can't be parsed + * @throws \InvalidArgumentException When a route can't be parsed because YAML is invalid * * @api */ @@ -53,38 +54,19 @@ class YamlFileLoader extends FileLoader // empty file if (null === $config) { - $config = array(); + return $collection; } // not an array if (!is_array($config)) { - throw new \InvalidArgumentException(sprintf('The file "%s" must contain a YAML array.', $file)); + throw new \InvalidArgumentException(sprintf('The file "%s" must contain a YAML array.', $path)); } foreach ($config as $name => $config) { - $config = $this->normalizeRouteConfig($config); + $this->validate($config, $name, $path); if (isset($config['resource'])) { - $type = isset($config['type']) ? $config['type'] : null; - $prefix = isset($config['prefix']) ? $config['prefix'] : ''; - $defaults = isset($config['defaults']) ? $config['defaults'] : array(); - $requirements = isset($config['requirements']) ? $config['requirements'] : array(); - $options = isset($config['options']) ? $config['options'] : array(); - $hostnamePattern = isset($config['hostname_pattern']) ? $config['hostname_pattern'] : null; - - $this->setCurrentDir(dirname($path)); - - $subCollection = $this->import($config['resource'], $type, false, $file); - /* @var $subCollection RouteCollection */ - $subCollection->addPrefix($prefix); - if (null !== $hostnamePattern) { - $subCollection->setHostnamePattern($hostnamePattern); - } - $subCollection->addDefaults($defaults); - $subCollection->addRequirements($requirements); - $subCollection->addOptions($options); - - $collection->addCollection($subCollection); + $this->parseImport($collection, $config, $path, $file); } else { $this->parseRoute($collection, $name, $config, $path); } @@ -109,46 +91,90 @@ class YamlFileLoader extends FileLoader * @param RouteCollection $collection A RouteCollection instance * @param string $name Route name * @param array $config Route definition - * @param string $file A Yaml file path - * - * @throws \InvalidArgumentException When config pattern is not defined for the given route + * @param string $path Full path of the YAML file being processed */ - protected function parseRoute(RouteCollection $collection, $name, $config, $file) + protected function parseRoute(RouteCollection $collection, $name, array $config, $path) { $defaults = isset($config['defaults']) ? $config['defaults'] : array(); $requirements = isset($config['requirements']) ? $config['requirements'] : array(); $options = isset($config['options']) ? $config['options'] : array(); $hostnamePattern = isset($config['hostname_pattern']) ? $config['hostname_pattern'] : null; - if (!isset($config['pattern'])) { - throw new \InvalidArgumentException(sprintf('You must define a "pattern" for the "%s" route.', $name)); - } - $route = new Route($config['pattern'], $defaults, $requirements, $options, $hostnamePattern); $collection->add($name, $route); } /** - * Normalize route configuration. + * Parses an import and adds the routes in the resource to the RouteCollection. * - * @param array $config A resource config - * - * @return array - * - * @throws \InvalidArgumentException if one of the provided config keys is not supported + * @param RouteCollection $collection A RouteCollection instance + * @param array $config Route definition + * @param string $path Full path of the YAML file being processed + * @param string $file Loaded file name */ - private function normalizeRouteConfig(array $config) + protected function parseImport(RouteCollection $collection, array $config, $path, $file) { - foreach ($config as $key => $value) { - if (!in_array($key, self::$availableKeys)) { - throw new \InvalidArgumentException(sprintf( - 'Yaml routing loader does not support given key: "%s". Expected one of the (%s).', - $key, implode(', ', self::$availableKeys) - )); - } - } + $type = isset($config['type']) ? $config['type'] : null; + $prefix = isset($config['prefix']) ? $config['prefix'] : ''; + $defaults = isset($config['defaults']) ? $config['defaults'] : array(); + $requirements = isset($config['requirements']) ? $config['requirements'] : array(); + $options = isset($config['options']) ? $config['options'] : array(); + $hostnamePattern = isset($config['hostname_pattern']) ? $config['hostname_pattern'] : null; - return $config; + $this->setCurrentDir(dirname($path)); + + $subCollection = $this->import($config['resource'], $type, false, $file); + /* @var $subCollection RouteCollection */ + $subCollection->addPrefix($prefix); + if (null !== $hostnamePattern) { + $subCollection->setHostnamePattern($hostnamePattern); + } + $subCollection->addDefaults($defaults); + $subCollection->addRequirements($requirements); + $subCollection->addOptions($options); + + $collection->addCollection($subCollection); + } + + /** + * Validates the route configuration. + * + * @param array $config A resource config + * @param string $name The config key + * @param string $path The loaded file path + * + * @throws \InvalidArgumentException If one of the provided config keys is not supported, + * something is missing or the combination is nonesense + */ + protected function validate($config, $name, $path) + { + if (!is_array($config)) { + throw new \InvalidArgumentException(sprintf('The definition of "%s" in "%s" must be a YAML array.', $name, $path)); + } + if ($extraKeys = array_diff(array_keys($config), self::$availableKeys)) { + throw new \InvalidArgumentException(sprintf( + 'The routing file "%s" contains unsupport keys for "%s": "%s". Expected one of: "%s".', + $path, $name, implode('", "', $extraKeys), implode('", "', self::$availableKeys) + )); + } + if (isset($config['resource']) && isset($config['pattern'])) { + throw new \InvalidArgumentException(sprintf( + 'The routing file "%s" must not specify both the "resource" key and the "pattern" key for "%s". Choose between an import and a route definition.', + $path, $name + )); + } + if (!isset($config['resource']) && isset($config['type'])) { + throw new \InvalidArgumentException(sprintf( + 'The "type" key for the route definition "%s" in "%s" is unsupported. It is only available for imports in combination with the "resource" key.', + $name, $path + )); + } + if (!isset($config['resource']) && !isset($config['pattern'])) { + throw new \InvalidArgumentException(sprintf( + 'You must define a "pattern" for the route "%s" in file "%s".', + $name, $path + )); + } } } diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/nonesense_resource_plus_path.yml b/src/Symfony/Component/Routing/Tests/Fixtures/nonesense_resource_plus_path.yml new file mode 100644 index 0000000000..eba64e56bb --- /dev/null +++ b/src/Symfony/Component/Routing/Tests/Fixtures/nonesense_resource_plus_path.yml @@ -0,0 +1,3 @@ +blog_show: + resource: validpattern.yml + pattern: /test diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/nonesense_type_without_resource.yml b/src/Symfony/Component/Routing/Tests/Fixtures/nonesense_type_without_resource.yml new file mode 100644 index 0000000000..3b4c76898b --- /dev/null +++ b/src/Symfony/Component/Routing/Tests/Fixtures/nonesense_type_without_resource.yml @@ -0,0 +1,3 @@ +blog_show: + pattern: /blog/{slug} + type: custom diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/nonvalid2.yml b/src/Symfony/Component/Routing/Tests/Fixtures/nonvalid2.yml new file mode 100644 index 0000000000..cfa9992bbc --- /dev/null +++ b/src/Symfony/Component/Routing/Tests/Fixtures/nonvalid2.yml @@ -0,0 +1 @@ +route: string diff --git a/src/Symfony/Component/Routing/Tests/Loader/YamlFileLoaderTest.php b/src/Symfony/Component/Routing/Tests/Loader/YamlFileLoaderTest.php index d28a499642..f99e789486 100644 --- a/src/Symfony/Component/Routing/Tests/Loader/YamlFileLoaderTest.php +++ b/src/Symfony/Component/Routing/Tests/Loader/YamlFileLoaderTest.php @@ -50,29 +50,17 @@ class YamlFileLoaderTest extends \PHPUnit_Framework_TestCase /** * @expectedException \InvalidArgumentException + * @dataProvider getPathsToInvalidFiles */ - public function testLoadThrowsExceptionIfNotAnArray() + public function testLoadThrowsExceptionWithInvalidFile($filePath) { $loader = new YamlFileLoader(new FileLocator(array(__DIR__.'/../Fixtures'))); - $loader->load('nonvalid.yml'); + $loader->load($filePath); } - /** - * @expectedException \InvalidArgumentException - */ - public function testLoadThrowsExceptionIfArrayHasUnsupportedKeys() + public function getPathsToInvalidFiles() { - $loader = new YamlFileLoader(new FileLocator(array(__DIR__.'/../Fixtures'))); - $loader->load('nonvalidkeys.yml'); - } - - /** - * @expectedException \InvalidArgumentException - */ - public function testLoadThrowsExceptionWhenIncomplete() - { - $loader = new YamlFileLoader(new FileLocator(array(__DIR__.'/../Fixtures'))); - $loader->load('incomplete.yml'); + return array(array('nonvalid.yml'), array('nonvalid2.yml'), array('incomplete.yml'), array('nonvalidkeys.yml'), array('nonesense_resource_plus_path.yml'), array('nonesense_type_without_resource.yml')); } public function testLoadSpecialRouteName() @@ -117,13 +105,4 @@ class YamlFileLoaderTest extends \PHPUnit_Framework_TestCase $this->assertEquals('bar', $routes['blog_show']->getOption('foo')); $this->assertEquals('{locale}.example.com', $routes['blog_show']->getHostnamePattern()); } - - /** - * @expectedException \InvalidArgumentException - */ - public function testParseRouteThrowsExceptionWithMissingPattern() - { - $loader = new YamlFileLoader(new FileLocator(array(__DIR__.'/../Fixtures'))); - $loader->load('incomplete.yml'); - } }