Skip to content

[Routing][Config] Allow patterns of resources to be excluded from config loading #31587

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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions UPGRADE-4.4.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,11 @@ Debug
* Deprecated the `FlattenException` class, use the one from the `ErrorRenderer` component instead
* Deprecated the component in favor of the `ErrorHandler` component

Config
------

* Deprecated overriding the `FilerLoader::import()` method without declaring the optional `$exclude` argument

DependencyInjection
-------------------

Expand Down
2 changes: 2 additions & 0 deletions UPGRADE-5.0.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ Config
* Removed `FileLoaderLoadException`, use `LoaderLoadException` instead.
* Using environment variables with `cannotBeEmpty()` if the value is validated with `validate()` will throw an exception.
* Removed the `root()` method in `TreeBuilder`, pass the root node information to the constructor instead
* The `FilerLoader::import()` method has a new `$exclude` argument.

Console
-------
Expand Down Expand Up @@ -411,6 +412,7 @@ Routing
with the new serialization methods in PHP 7.4.
* Removed `ServiceRouterLoader` and `ObjectRouteLoader`.
* Service route loaders must be tagged with `routing.route_loader`.
* The `RoutingConfigurator::import()` method has a new optional `$exclude` argument.

Security
--------
Expand Down
5 changes: 5 additions & 0 deletions src/Symfony/Component/Config/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
CHANGELOG
=========

4.4.0
-----

* added a way to exclude patterns of resources from being imported by the `import()` method

4.3.0
-----

Expand Down
26 changes: 20 additions & 6 deletions src/Symfony/Component/Config/Loader/FileLoader.php
Original file line number Diff line number Diff line change
Expand Up @@ -59,23 +59,37 @@ public function getLocator()
/**
* Imports a resource.
*
* @param mixed $resource A Resource
* @param string|null $type The resource type or null if unknown
* @param bool $ignoreErrors Whether to ignore import errors or not
* @param string|null $sourceResource The original resource importing the new resource
* @param mixed $resource A Resource
* @param string|null $type The resource type or null if unknown
* @param bool $ignoreErrors Whether to ignore import errors or not
* @param string|null $sourceResource The original resource importing the new resource
* @param string|string[]|null $exclude Glob patterns to exclude from the import
*
* @return mixed
*
* @throws LoaderLoadException
* @throws FileLoaderImportCircularReferenceException
* @throws FileLocatorFileNotFoundException
*/
public function import($resource, $type = null, $ignoreErrors = false, $sourceResource = null)
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) {
@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;

if (\is_string($resource) && \strlen($resource) !== $i = strcspn($resource, '*?{[')) {
$excluded = [];
Copy link
Contributor

@gharlan gharlan Feb 11, 2020

Choose a reason for hiding this comment

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

Is it expected, that exclude is only handled when the resource contains glob patterns (*?{[)?
exclude does not have any effect when using a directory as resource, like in default recipe:
https://github.com/symfony/recipes/blob/b364e60751fba4fc5da103304c1f61259d370e10/doctrine/annotations/1.0/config/routes/annotations.yaml#L2

Copy link

@Growiel Growiel Jun 10, 2020

Choose a reason for hiding this comment

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

I stumbled upon the same issue and it cost me quite a lot of time to figure it out !

I do not believe this is working as intended, as it's different from what the documentation says.

To get my exclude to work, I had to change my resource too (add *):

controllers:
    resource: '../../src/Controller'
    exclude: '../../src/Controller/Backoffice/{HomepageController}.php'

does NOT work (HomepageController is not excluded)..

controllers:
    resource: '../../src/Controller/*'
    exclude: '../../src/Controller/Backoffice/{HomepageController}.php'

does work, the HomepageController is correctly excluded.

Without the *, the condition is not satisfied and the exclude is ignored totally, which makes no sense.

Should we open a new issue for this, @tristanbes ?

Choose a reason for hiding this comment

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

Thx @gharlan and @Growiel I stumbled upon this strange behaviour today and thanks to your hints I did not have to spend too much time debugging this :) Did you open an issue for this though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello, I guess you should open an issue if you think it's a bug or if this case should be handled

Copy link

Choose a reason for hiding this comment

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

It is indeed a bug. Do you need a new issue or handle it here ?

Copy link
Member

Choose a reason for hiding this comment

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

Please create a new issue. Comments on already merged pull requests are likely to get lost.

foreach ((array) $exclude as $pattern) {
foreach ($this->glob($pattern, true, $_, false, true) as $path => $info) {
// normalize Windows slashes
$excluded[str_replace('\\', '/', $path)] = true;
}
}

$ret = [];
$isSubpath = 0 !== $i && false !== strpos(substr($resource, 0, $i), '/');
foreach ($this->glob($resource, false, $_, $ignoreErrors || !$isSubpath) as $path => $info) {
foreach ($this->glob($resource, false, $_, $ignoreErrors || !$isSubpath, false, $excluded) as $path => $info) {
if (null !== $res = $this->doImport($path, $type, $ignoreErrors, $sourceResource)) {
$ret[] = $res;
}
Expand Down
Empty file.
Empty file.
8 changes: 8 additions & 0 deletions src/Symfony/Component/Config/Tests/Loader/FileLoaderTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,14 @@ public function testImportWithSimpleGlob()

$this->assertSame(__FILE__, strtr($loader->import('FileLoaderTest.*'), '/', \DIRECTORY_SEPARATOR));
}

public function testImportWithExclude()
{
$loader = new TestFileLoader(new FileLocator(__DIR__.'/../Fixtures'));
$loadedFiles = $loader->import('Include/*', null, false, null, __DIR__.'/../Fixtures/Include/{ExcludeFile.txt}');
$this->assertCount(2, $loadedFiles);
$this->assertNotContains('ExcludeFile.txt', $loadedFiles);
}
}

class TestFileLoader extends FileLoader
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ public function testLoadAddsTheGlobResourceToTheContainer()

class GlobFileLoaderWithoutImport extends GlobFileLoader
{
public function import($resource, $type = null, $ignoreErrors = false, $sourceResource = null)
public function import($resource, $type = null, $ignoreErrors = false, $sourceResource = null, $exclude = null)
{
}
}
1 change: 1 addition & 0 deletions src/Symfony/Component/Routing/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ CHANGELOG

* Deprecated `ServiceRouterLoader` in favor of `ContainerLoader`.
* Deprecated `ObjectRouteLoader` in favor of `ObjectLoader`.
* Added a way to exclude patterns of resources from being imported by the `import()` method

4.3.0
-----
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,14 @@ public function __construct(RouteCollection $collection, PhpFileLoader $loader,
$this->file = $file;
}

final public function import($resource, string $type = null, bool $ignoreErrors = false): ImportConfigurator
/**
* @param string|string[]|null $exclude Glob patterns to exclude from the import
*/
final public function import($resource, string $type = null, bool $ignoreErrors = false, $exclude = null): ImportConfigurator
{
$this->loader->setCurrentDir(\dirname($this->path));
$imported = $this->loader->import($resource, $type, $ignoreErrors, $this->file) ?: [];

$imported = $this->loader->import($resource, $type, $ignoreErrors, $this->file, $exclude) ?: [];
if (!\is_array($imported)) {
return new ImportConfigurator($this->collection, $imported);
}
Expand Down
16 changes: 15 additions & 1 deletion src/Symfony/Component/Routing/Loader/XmlFileLoader.php
Original file line number Diff line number Diff line change
Expand Up @@ -163,10 +163,24 @@ protected function parseImport(RouteCollection $collection, \DOMElement $node, $
throw new \InvalidArgumentException(sprintf('The <route> element in file "%s" must not have both a "prefix" attribute and <prefix> child nodes.', $path));
}

$exclude = [];
foreach ($node->childNodes as $child) {
if ($child instanceof \DOMElement && $child->localName === $exclude && self::NAMESPACE_URI === $child->namespaceURI) {
$exclude[] = $child->nodeValue;
}
}

if ($node->hasAttribute('exclude')) {
if ($exclude) {
throw new \InvalidArgumentException('You cannot use both the attribute "exclude" and <exclude> tags at the same time.');
}
$exclude = [$node->getAttribute('exclude')];
}

$this->setCurrentDir(\dirname($path));

/** @var RouteCollection[] $imported */
$imported = $this->import($resource, ('' !== $type ? $type : null), false, $file) ?: [];
$imported = $this->import($resource, ('' !== $type ? $type : null), false, $file, $exclude) ?: [];

if (!\is_array($imported)) {
$imported = [$imported];
Expand Down
5 changes: 3 additions & 2 deletions src/Symfony/Component/Routing/Loader/YamlFileLoader.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
class YamlFileLoader extends FileLoader
{
private static $availableKeys = [
'resource', 'type', 'prefix', 'path', 'host', 'schemes', 'methods', 'defaults', 'requirements', 'options', 'condition', 'controller', 'name_prefix', 'trailing_slash_on_root', 'locale', 'format', 'utf8',
'resource', 'type', 'prefix', 'path', 'host', 'schemes', 'methods', 'defaults', 'requirements', 'options', 'condition', 'controller', 'name_prefix', 'trailing_slash_on_root', 'locale', 'format', 'utf8', 'exclude',
];
private $yamlParser;

Expand Down Expand Up @@ -169,6 +169,7 @@ protected function parseImport(RouteCollection $collection, array $config, $path
$schemes = isset($config['schemes']) ? $config['schemes'] : null;
$methods = isset($config['methods']) ? $config['methods'] : null;
$trailingSlashOnRoot = $config['trailing_slash_on_root'] ?? true;
$exclude = $config['exclude'] ?? null;

if (isset($config['controller'])) {
$defaults['_controller'] = $config['controller'];
Expand All @@ -185,7 +186,7 @@ protected function parseImport(RouteCollection $collection, array $config, $path

$this->setCurrentDir(\dirname($path));

$imported = $this->import($config['resource'], $type, false, $file) ?: [];
$imported = $this->import($config['resource'], $type, false, $file, $exclude) ?: [];

if (!\is_array($imported)) {
$imported = [$imported];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,11 @@
<xsd:sequence maxOccurs="unbounded" minOccurs="0">
<xsd:group ref="configs" minOccurs="0" maxOccurs="unbounded" />
<xsd:element name="prefix" type="localized-path" minOccurs="0" maxOccurs="unbounded" />
<xsd:element name="exclude" type="xsd:string" minOccurs="0" maxOccurs="unbounded" />
</xsd:sequence>
<xsd:attribute name="resource" type="xsd:string" use="required" />
<xsd:attribute name="type" type="xsd:string" />
<xsd:attribute name="exclude" type="xsd:string" />
<xsd:attribute name="prefix" type="xsd:string" />
<xsd:attribute name="name-prefix" type="xsd:string" />
<xsd:attribute name="host" type="xsd:string" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ public function testLoadAddsTheGlobResourceToTheContainer()

class GlobFileLoaderWithoutImport extends GlobFileLoader
{
public function import($resource, $type = null, $ignoreErrors = false, $sourceResource = null)
public function import($resource, $type = null, $ignoreErrors = false, $sourceResource = null, $exclude = null)
{
return new RouteCollection();
}
Expand Down