From 5183501e0b3c9ea2b8e155e247113a0af9cbe53e Mon Sep 17 00:00:00 2001 From: Christophe Coevoet Date: Fri, 11 Jul 2014 13:56:54 +0200 Subject: [PATCH] [DI] Added safeguards against invalid config in the YamlFileLoader Exceptions explaining the mistake are better than fatal errors or weird notices appearing when trying to deal with such invalid data. Closes #11333 --- .../Loader/YamlFileLoader.php | 44 ++++++++++++++++--- .../Tests/Fixtures/yaml/bad_calls.yml | 4 ++ .../Tests/Fixtures/yaml/bad_import.yml | 2 + .../Tests/Fixtures/yaml/bad_imports.yml | 2 + .../Tests/Fixtures/yaml/bad_parameters.yml | 2 + .../Tests/Fixtures/yaml/bad_service.yml | 2 + .../Tests/Fixtures/yaml/bad_services.yml | 1 + .../Tests/Fixtures/yaml/badtag4.yml | 6 +++ .../Tests/Loader/YamlFileLoaderTest.php | 36 ++++++++++++++- 9 files changed, 90 insertions(+), 9 deletions(-) create mode 100644 src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/bad_calls.yml create mode 100644 src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/bad_import.yml create mode 100644 src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/bad_imports.yml create mode 100644 src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/bad_parameters.yml create mode 100644 src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/bad_service.yml create mode 100644 src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/bad_services.yml create mode 100644 src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/badtag4.yml diff --git a/src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php b/src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php index cf4cfc75bd..8fee1bfefa 100644 --- a/src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php +++ b/src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php @@ -55,6 +55,10 @@ class YamlFileLoader extends FileLoader // parameters if (isset($content['parameters'])) { + if (!is_array($content['parameters'])) { + throw new InvalidArgumentException(sprintf('The "parameters" key should contain an array in %s. Check your YAML syntax.', $file)); + } + foreach ($content['parameters'] as $key => $value) { $this->container->setParameter($key, $this->resolveServices($value)); } @@ -92,7 +96,15 @@ class YamlFileLoader extends FileLoader return; } + if (!is_array($content['imports'])) { + throw new InvalidArgumentException(sprintf('The "imports" key should contain an array in %s. Check your YAML syntax.', $file)); + } + foreach ($content['imports'] as $import) { + if (!is_array($import)) { + throw new InvalidArgumentException(sprintf('The values in the "imports" key should be arrays in %s. Check your YAML syntax.', $file)); + } + $this->setCurrentDir(dirname($file)); $this->import($import['resource'], null, isset($import['ignore_errors']) ? (bool) $import['ignore_errors'] : false, $file); } @@ -110,6 +122,10 @@ class YamlFileLoader extends FileLoader return; } + if (!is_array($content['services'])) { + throw new InvalidArgumentException(sprintf('The "services" key should contain an array in %s. Check your YAML syntax.', $file)); + } + foreach ($content['services'] as $id => $service) { $this->parseDefinition($id, $service, $file); } @@ -130,7 +146,13 @@ class YamlFileLoader extends FileLoader $this->container->setAlias($id, substr($service, 1)); return; - } elseif (isset($service['alias'])) { + } + + if (!is_array($service)) { + throw new InvalidArgumentException(sprintf('A service definition must be an array or a string starting with "@" but %s found for service "%s" in %s. Check your YAML syntax.', gettype($service), $id, $file)); + } + + if (isset($service['alias'])) { $public = !array_key_exists('public', $service) || (bool) $service['public']; $this->container->setAlias($id, new Alias($service['alias'], $public)); @@ -204,6 +226,10 @@ class YamlFileLoader extends FileLoader } if (isset($service['calls'])) { + if (!is_array($service['calls'])) { + throw new InvalidArgumentException(sprintf('Parameter "calls" must be an array for service "%s" in %s. Check your YAML syntax.', $id, $file)); + } + foreach ($service['calls'] as $call) { $args = isset($call[1]) ? $this->resolveServices($call[1]) : array(); $definition->addMethodCall($call[0], $args); @@ -212,10 +238,14 @@ class YamlFileLoader extends FileLoader if (isset($service['tags'])) { if (!is_array($service['tags'])) { - throw new InvalidArgumentException(sprintf('Parameter "tags" must be an array for service "%s" in %s.', $id, $file)); + throw new InvalidArgumentException(sprintf('Parameter "tags" must be an array for service "%s" in %s. Check your YAML syntax.', $id, $file)); } foreach ($service['tags'] as $tag) { + if (!is_array($tag)) { + throw new InvalidArgumentException(sprintf('A "tags" entry must be an array for service "%s" in %s. Check your YAML syntax.', $id, $file)); + } + if (!isset($tag['name'])) { throw new InvalidArgumentException(sprintf('A "tags" entry is missing a "name" key for service "%s" in %s.', $id, $file)); } @@ -223,9 +253,9 @@ class YamlFileLoader extends FileLoader $name = $tag['name']; unset($tag['name']); - foreach ($tag as $attribute => $value) { + foreach ($tag as $value) { if (!is_scalar($value)) { - throw new InvalidArgumentException(sprintf('A "tags" attribute must be of a scalar-type for service "%s", tag "%s" in %s.', $id, $name, $file)); + throw new InvalidArgumentException(sprintf('A "tags" attribute must be of a scalar-type for service "%s", tag "%s" in %s. Check your YAML syntax.', $id, $name, $file)); } } @@ -279,7 +309,7 @@ class YamlFileLoader extends FileLoader } if (!is_array($content)) { - throw new InvalidArgumentException(sprintf('The service file "%s" is not valid.', $file)); + throw new InvalidArgumentException(sprintf('The service file "%s" is not valid. It should contain an array. Check your YAML syntax.', $file)); } foreach (array_keys($content) as $namespace) { @@ -305,9 +335,9 @@ class YamlFileLoader extends FileLoader /** * Resolves services. * - * @param string $value + * @param string|array $value * - * @return Reference + * @return array|string|Reference */ private function resolveServices($value) { diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/bad_calls.yml b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/bad_calls.yml new file mode 100644 index 0000000000..3f34b07eb0 --- /dev/null +++ b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/bad_calls.yml @@ -0,0 +1,4 @@ +services: + method_call1: + class: FooClass + calls: foo diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/bad_import.yml b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/bad_import.yml new file mode 100644 index 0000000000..0765dc8dd0 --- /dev/null +++ b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/bad_import.yml @@ -0,0 +1,2 @@ +imports: + - foo.yml diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/bad_imports.yml b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/bad_imports.yml new file mode 100644 index 0000000000..1ce9d57c6c --- /dev/null +++ b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/bad_imports.yml @@ -0,0 +1,2 @@ +imports: + foo:bar diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/bad_parameters.yml b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/bad_parameters.yml new file mode 100644 index 0000000000..bbd13ac0b3 --- /dev/null +++ b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/bad_parameters.yml @@ -0,0 +1,2 @@ +parameters: + foo:bar diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/bad_service.yml b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/bad_service.yml new file mode 100644 index 0000000000..811af3c0ef --- /dev/null +++ b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/bad_service.yml @@ -0,0 +1,2 @@ +services: + foo: bar diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/bad_services.yml b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/bad_services.yml new file mode 100644 index 0000000000..cfbf17ce5f --- /dev/null +++ b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/bad_services.yml @@ -0,0 +1 @@ +services: foo diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/badtag4.yml b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/badtag4.yml new file mode 100644 index 0000000000..e8e99395b1 --- /dev/null +++ b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/badtag4.yml @@ -0,0 +1,6 @@ +services: + foo_service: + class: FooClass + tags: + # tag is not an array + - foo diff --git a/src/Symfony/Component/DependencyInjection/Tests/Loader/YamlFileLoaderTest.php b/src/Symfony/Component/DependencyInjection/Tests/Loader/YamlFileLoaderTest.php index 008a6c591f..e7887fa922 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/Loader/YamlFileLoaderTest.php +++ b/src/Symfony/Component/DependencyInjection/Tests/Loader/YamlFileLoaderTest.php @@ -13,7 +13,6 @@ namespace Symfony\Component\DependencyInjection\Tests\Loader; use Symfony\Component\DependencyInjection\ContainerBuilder; use Symfony\Component\DependencyInjection\Reference; -use Symfony\Component\DependencyInjection\Definition; use Symfony\Component\Config\Loader\Loader; use Symfony\Component\DependencyInjection\Loader\XmlFileLoader; use Symfony\Component\DependencyInjection\Loader\YamlFileLoader; @@ -80,6 +79,29 @@ class YamlFileLoaderTest extends \PHPUnit_Framework_TestCase } } + /** + * @dataProvider provideInvalidFiles + * @expectedException \Symfony\Component\DependencyInjection\Exception\InvalidArgumentException + */ + public function testLoadInvalidFile($file) + { + $loader = new YamlFileLoader(new ContainerBuilder(), new FileLocator(self::$fixturesPath.'/yaml')); + + $loader->load($file.'.yml'); + } + + public function provideInvalidFiles() + { + return array( + array('bad_parameters'), + array('bad_imports'), + array('bad_import'), + array('bad_services'), + array('bad_service'), + array('bad_calls'), + ); + } + public function testLoadParameters() { $container = new ContainerBuilder(); @@ -179,7 +201,7 @@ class YamlFileLoaderTest extends \PHPUnit_Framework_TestCase $this->assertFalse($loader->supports('foo.foo'), '->supports() returns true if the resource is loadable'); } - public function testNonArrayTagThrowsException() + public function testNonArrayTagsThrowsException() { $loader = new YamlFileLoader(new ContainerBuilder(), new FileLocator(self::$fixturesPath.'/yaml')); try { @@ -191,6 +213,16 @@ class YamlFileLoaderTest extends \PHPUnit_Framework_TestCase } } + /** + * @expectedException \Symfony\Component\DependencyInjection\Exception\InvalidArgumentException + * @expectedExceptionMessage A "tags" entry must be an array for service + */ + public function testNonArrayTagThrowsException() + { + $loader = new YamlFileLoader(new ContainerBuilder(), new FileLocator(self::$fixturesPath.'/yaml')); + $loader->load('badtag4.yml'); + } + public function testTagWithoutNameThrowsException() { $loader = new YamlFileLoader(new ContainerBuilder(), new FileLocator(self::$fixturesPath.'/yaml'));