Skip to content

Commit

Permalink
feature #31310 [DependencyInjection] Added option `ignore_errors: not…
Browse files Browse the repository at this point in the history
…_found` for imported config files (pulzarraider)

This PR was merged into the 4.4 branch.

Discussion
----------

[DependencyInjection] Added option `ignore_errors: not_found` for imported config files

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | symfony/symfony-docs#11647

If someone want to add optional config file. The only available choice was to add `ignore_errors: true` option

e.g.
```
imports:
    - { resource: parameters.yml, ignore_errors: true }
```

But this will hide all errors in imported file. We ran in many situations that broke our Symfony applications because we had a typo in this imported files.

This PR introduce new possible value `not_found` for `ignore_errors` option. It can be used for optional config files like the `ignore_errors: true`, but it will ignore only the file non-existence, not the possible syntax errors inside.

Usage:
```
imports:
    - { resource: parameters.yml, ignore_errors: not_found}
```

Commits
-------

e0ee01c [DependencyInjection] Added option `ignore_errors: not_found` while importing config files
  • Loading branch information
nicolas-grekas committed Nov 8, 2019
2 parents 7a8b85c + e0ee01c commit 585c0df
Show file tree
Hide file tree
Showing 14 changed files with 141 additions and 5 deletions.
2 changes: 1 addition & 1 deletion src/Symfony/Component/Config/Loader/FileLoader.php
Expand Up @@ -73,7 +73,7 @@ public function getLocator()
*/
public function import($resource, $type = null, $ignoreErrors = false, $sourceResource = null/*, $exclude = null*/)
{
if (\func_num_args() < 5 && __CLASS__ !== \get_class($this) && __CLASS__ !== (new \ReflectionMethod($this, __FUNCTION__))->getDeclaringClass()->getName() && !$this instanceof \PHPUnit\Framework\MockObject\MockObject && !$this instanceof \Prophecy\Prophecy\ProphecySubjectInterface) {
if (\func_num_args() < 5 && __CLASS__ !== \get_class($this) && 0 !== strpos(\get_class($this), 'Symfony\Component\\') && __CLASS__ !== (new \ReflectionMethod($this, __FUNCTION__))->getDeclaringClass()->getName() && !$this instanceof \PHPUnit\Framework\MockObject\MockObject && !$this instanceof \Prophecy\Prophecy\ProphecySubjectInterface) {
@trigger_error(sprintf('The "%s()" method will have a new "$exclude = null" argument in version 5.0, not defining it is deprecated since Symfony 4.4.', __METHOD__), E_USER_DEPRECATED);
}
$exclude = \func_num_args() >= 5 ? func_get_arg(4) : null;
Expand Down
Expand Up @@ -60,7 +60,7 @@ final public function extension(string $namespace, array $config)
$this->container->loadFromExtension($namespace, static::processValue($config));
}

final public function import(string $resource, string $type = null, bool $ignoreErrors = false)
final public function import(string $resource, string $type = null, $ignoreErrors = false)
{
$this->loader->setCurrentDir(\dirname($this->path));
$this->loader->import($resource, $type, $ignoreErrors, $this->file);
Expand Down
39 changes: 39 additions & 0 deletions src/Symfony/Component/DependencyInjection/Loader/FileLoader.php
Expand Up @@ -11,8 +11,11 @@

namespace Symfony\Component\DependencyInjection\Loader;

use Symfony\Component\Config\Exception\FileLocatorFileNotFoundException;
use Symfony\Component\Config\Exception\LoaderLoadException;
use Symfony\Component\Config\FileLocatorInterface;
use Symfony\Component\Config\Loader\FileLoader as BaseFileLoader;
use Symfony\Component\Config\Loader\Loader;
use Symfony\Component\Config\Resource\GlobResource;
use Symfony\Component\DependencyInjection\ChildDefinition;
use Symfony\Component\DependencyInjection\ContainerBuilder;
Expand Down Expand Up @@ -41,6 +44,42 @@ public function __construct(ContainerBuilder $container, FileLocatorInterface $l
parent::__construct($locator);
}

/**
* {@inheritdoc}
*
* @param bool|string $ignoreErrors Whether errors should be ignored; pass "not_found" to ignore only when the loaded resource is not found
* @param string|string[]|null $exclude Glob patterns to exclude from the import
*/
public function import($resource, $type = null, $ignoreErrors = false, $sourceResource = null/*, $exclude = null*/)
{
$args = \func_get_args();

if ($ignoreNotFound = 'not_found' === $ignoreErrors) {
$args[2] = false;
} elseif (!\is_bool($ignoreErrors)) {
@trigger_error(sprintf('Invalid argument $ignoreErrors provided to %s::import(): boolean or "not_found" expected, %s given.', \get_class($this), \gettype($ignoreErrors)), E_USER_DEPRECATED);
$args[2] = (bool) $ignoreErrors;
}

try {
parent::import(...$args);
} catch (LoaderLoadException $e) {
if (!$ignoreNotFound || !($prev = $e->getPrevious()) instanceof FileLocatorFileNotFoundException) {
throw $e;
}

foreach ($prev->getTrace() as $frame) {
if ('import' === ($frame['function'] ?? null) && is_a($frame['class'] ?? '', Loader::class, true)) {
break;
}
}

if ($args !== $frame['args']) {
throw $e;
}
}
}

/**
* Registers a set of classes as services using PSR-4 for discovery.
*
Expand Down
Expand Up @@ -105,7 +105,7 @@ private function parseImports(\DOMDocument $xml, string $file)
$defaultDirectory = \dirname($file);
foreach ($imports as $import) {
$this->setCurrentDir($defaultDirectory);
$this->import($import->getAttribute('resource'), XmlUtils::phpize($import->getAttribute('type')) ?: null, (bool) XmlUtils::phpize($import->getAttribute('ignore-errors')), $file);
$this->import($import->getAttribute('resource'), XmlUtils::phpize($import->getAttribute('type')) ?: null, XmlUtils::phpize($import->getAttribute('ignore-errors')) ?: false, $file);
}
}

Expand Down
Expand Up @@ -191,7 +191,7 @@ private function parseImports(array $content, string $file)
}

$this->setCurrentDir($defaultDirectory);
$this->import($import['resource'], isset($import['type']) ? $import['type'] : null, isset($import['ignore_errors']) ? (bool) $import['ignore_errors'] : false, $file);
$this->import($import['resource'], $import['type'] ?? null, $import['ignore_errors'] ?? false, $file);
}
}

Expand Down
Expand Up @@ -78,7 +78,7 @@
]]></xsd:documentation>
</xsd:annotation>
<xsd:attribute name="resource" type="xsd:string" use="required" />
<xsd:attribute name="ignore-errors" type="boolean" />
<xsd:attribute name="ignore-errors" type="ignore_errors" />
<xsd:attribute name="type" type="xsd:string" />
</xsd:complexType>

Expand Down Expand Up @@ -273,6 +273,12 @@
</xsd:restriction>
</xsd:simpleType>

<xsd:simpleType name="ignore_errors">
<xsd:restriction base="xsd:string">
<xsd:pattern value="(true|false|not_found)" />
</xsd:restriction>
</xsd:simpleType>

<xsd:simpleType name="invalid_sequence">
<xsd:restriction base="xsd:string">
<xsd:enumeration value="null" />
Expand Down
@@ -0,0 +1,9 @@
<?xml version="1.0" ?>

<container xmlns="http://symfony.com/schema/dic/services"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://symfony.com/schema/dic/services https://symfony.com/schema/dic/services/services-1.0.xsd">
<imports>
<import resource="foo_fake.xml" ignore-errors="not_found" />
</imports>
</container>
@@ -0,0 +1,9 @@
<?xml version="1.0" ?>

<container xmlns="http://symfony.com/schema/dic/services"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://symfony.com/schema/dic/services https://symfony.com/schema/dic/services/services-1.0.xsd">
<imports>
<import resource="nonvalid.xml" ignore-errors="not_found" />
</imports>
</container>
@@ -0,0 +1,9 @@
<?xml version="1.0" ?>

<container xmlns="http://symfony.com/schema/dic/services"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://symfony.com/schema/dic/services https://symfony.com/schema/dic/services/services-1.0.xsd">
<imports>
<import resource="foo_fake.xml" />
</imports>
</container>
@@ -0,0 +1,2 @@
imports:
- { resource: foo_fake.yml, ignore_errors: not_found }
@@ -0,0 +1,2 @@
imports:
- { resource: nonvalid2.yml, ignore_errors: not_found }
@@ -0,0 +1,2 @@
imports:
- { resource: foo_fake.yml }
Expand Up @@ -183,6 +183,37 @@ public function testLoadImports()

// Bad import throws no exception due to ignore_errors value.
$loader->load('services4_bad_import.xml');

// Bad import with nonexistent file throws no exception due to ignore_errors: not_found value.
$loader->load('services4_bad_import_file_not_found.xml');

try {
$loader->load('services4_bad_import_with_errors.xml');
$this->fail('->load() throws a LoaderLoadException if the imported xml file configuration does not exist');
} catch (\Exception $e) {
$this->assertInstanceOf('Symfony\\Component\\Config\\Exception\\LoaderLoadException', $e, '->load() throws a LoaderLoadException if the imported xml file configuration does not exist');
$this->assertRegExp(sprintf('#^The file "%1$s" does not exist \(in: .+\) in %1$s \(which is being imported from ".+%2$s"\)\.$#', 'foo_fake\.xml', 'services4_bad_import_with_errors\.xml'), $e->getMessage(), '->load() throws a LoaderLoadException if the imported xml file configuration does not exist');

$e = $e->getPrevious();
$this->assertInstanceOf('Symfony\\Component\\Config\\Exception\\FileLocatorFileNotFoundException', $e, '->load() throws a FileLocatorFileNotFoundException if the imported xml file configuration does not exist');
$this->assertRegExp(sprintf('#^The file "%s" does not exist \(in: .+\)\.$#', 'foo_fake\.xml'), $e->getMessage(), '->load() throws a FileLocatorFileNotFoundException if the imported xml file configuration does not exist');
}

try {
$loader->load('services4_bad_import_nonvalid.xml');
$this->fail('->load() throws an LoaderLoadException if the imported configuration does not validate the XSD');
} catch (\Exception $e) {
$this->assertInstanceOf('Symfony\\Component\\Config\\Exception\\LoaderLoadException', $e, '->load() throws a LoaderLoadException if the imported configuration does not validate the XSD');
$this->assertRegExp(sprintf('#^Unable to parse file ".+%s": .+.$#', 'nonvalid\.xml'), $e->getMessage(), '->load() throws a LoaderLoadException if the imported configuration does not validate the XSD');

$e = $e->getPrevious();
$this->assertInstanceOf('Symfony\\Component\\DependencyInjection\\Exception\\InvalidArgumentException', $e, '->load() throws an InvalidArgumentException if the configuration does not validate the XSD');
$this->assertRegExp(sprintf('#^Unable to parse file ".+%s": .+.$#', 'nonvalid\.xml'), $e->getMessage(), '->load() throws an InvalidArgumentException if the configuration does not validate the XSD');

$e = $e->getPrevious();
$this->assertInstanceOf('Symfony\\Component\\Config\\Util\\Exception\\XmlParsingException', $e, '->load() throws a XmlParsingException if the configuration does not validate the XSD');
$this->assertStringStartsWith('[ERROR 1845] Element \'nonvalid\': No matching global declaration available for the validation root. (in', $e->getMessage(), '->load() throws a XmlParsingException if the loaded file does not validate the XSD');
}
}

public function testLoadAnonymousServices()
Expand Down
Expand Up @@ -135,6 +135,33 @@ public function testLoadImports()

// Bad import throws no exception due to ignore_errors value.
$loader->load('services4_bad_import.yml');

// Bad import with nonexistent file throws no exception due to ignore_errors: not_found value.
$loader->load('services4_bad_import_file_not_found.yml');

try {
$loader->load('services4_bad_import_with_errors.yml');
$this->fail('->load() throws a LoaderLoadException if the imported yaml file does not exist');
} catch (\Exception $e) {
$this->assertInstanceOf('Symfony\\Component\\Config\\Exception\\LoaderLoadException', $e, '->load() throws a LoaderLoadException if the imported yaml file does not exist');
$this->assertRegExp(sprintf('#^The file "%1$s" does not exist \(in: .+\) in %1$s \(which is being imported from ".+%2$s"\)\.$#', 'foo_fake\.yml', 'services4_bad_import_with_errors\.yml'), $e->getMessage(), '->load() throws a LoaderLoadException if the imported yaml file does not exist');

$e = $e->getPrevious();
$this->assertInstanceOf('Symfony\\Component\\Config\\Exception\\FileLocatorFileNotFoundException', $e, '->load() throws a FileLocatorFileNotFoundException if the imported yaml file does not exist');
$this->assertRegExp(sprintf('#^The file "%s" does not exist \(in: .+\)\.$#', 'foo_fake\.yml'), $e->getMessage(), '->load() throws a FileLocatorFileNotFoundException if the imported yaml file does not exist');
}

try {
$loader->load('services4_bad_import_nonvalid.yml');
$this->fail('->load() throws a LoaderLoadException if the tag in the imported yaml file is not valid');
} catch (\Exception $e) {
$this->assertInstanceOf('Symfony\\Component\\Config\\Exception\\LoaderLoadException', $e, '->load() throws a LoaderLoadException if the tag in the imported yaml file is not valid');
$this->assertRegExp(sprintf('#^The service file ".+%1$s" is not valid\. It should contain an array\. Check your YAML syntax in .+%1$s \(which is being imported from ".+%2$s"\)\.$#', 'nonvalid2\.yml', 'services4_bad_import_nonvalid.yml'), $e->getMessage(), '->load() throws a LoaderLoadException if the tag in the imported yaml file is not valid');

$e = $e->getPrevious();
$this->assertInstanceOf('Symfony\\Component\\DependencyInjection\\Exception\\InvalidArgumentException', $e, '->load() throws an InvalidArgumentException if the tag in the imported yaml file is not valid');
$this->assertRegExp(sprintf('#^The service file ".+%s" is not valid\. It should contain an array\. Check your YAML syntax\.$#', 'nonvalid2\.yml'), $e->getMessage(), '->load() throws an InvalidArgumentException if the tag in the imported yaml file is not valid');
}
}

public function testLoadServices()
Expand Down

0 comments on commit 585c0df

Please sign in to comment.