Skip to content

[DependencyInjection] Use glob pattern to load config files #21270

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 1 commit into from
Feb 14, 2017

Conversation

pierredup
Copy link
Contributor

@pierredup pierredup commented Jan 13, 2017

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

This relates to #21173, but I'm not sure if it completely fixes the issue.

This allows to use a glob pattern to load config files, which makes the following possible:

# config.yml
imports:
    - { resource: "*.yml" }
    - { resource: "folder/*.yml" }
    - { resource: "/etc/myapp/*.{yml,xml}" }

It can also be used in a container extension, if a bundle uses a lot of configs:

$loader = new Loader\YamlFileLoader($container, new FileLocator(__DIR__ . '/../Resources/config'));
$loader->load('*.yml');
$loader->load('routing/*');

@@ -42,11 +42,15 @@ public function locate($name, $currentPath = null, $first = true)
}

if ($this->isAbsolutePath($name)) {
if (!file_exists($name)) {
if (array() === $files = glob($name, GLOB_BRACE)) {
Copy link
Member

Choose a reason for hiding this comment

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

missing check for GLOB_BRACE, because it's is not always defined (see php.net/glob )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check added

};

foreach ($paths as $path) {
if (array() !== $files = glob($path.DIRECTORY_SEPARATOR.$name)) {
Copy link
Member

Choose a reason for hiding this comment

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

no BLOG_BRACE here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GLOB_BRACE added with a defined check

if (array() !== $files = glob($path.DIRECTORY_SEPARATOR.$name)) {
array_walk($files, $load);
} else {
$load($path.DIRECTORY_SEPARATOR.$name);
Copy link
Member

Choose a reason for hiding this comment

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

Does this actually do something? Looks like "glob" failed to find any file, so why would "file_exists" (in $load) return something else that "false"? If I'm right, then I'd prefer this array_walk + $load to be replaced by a simple foreach (with no array() !== ... check). But I may miss something, that's why I'm asking :)

Copy link
Member

Choose a reason for hiding this comment

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

(if it's legit, we should be sure it has a test case btw)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that this doesn't do anything.
Originally I wanted to preserve any existing functionality, but the glob should cover all current use cases.

throw new FileLocatorFileNotFoundException(sprintf('The file "%s" does not exist.', $name));
}

return $name;
if ($first) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wdyt about adding || 1 === count($files) to keep the return the same as now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I think we should keep the original behavior as it was (just invert the file_exists check), then add the glob as an extra step.
Otherwise if you do something like /etc/myapp/*.yml, you would expect an array, so it might be weird to get a string if the glob only matches a single file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes made

if (true === $first) {
return $file;
}
foreach (glob($path.DIRECTORY_SEPARATOR.$name, defined('GLOB_BRACE') ? GLOB_BRACE : 0) as $file) {
Copy link
Contributor

Choose a reason for hiding this comment

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

array_merge?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@pierredup pierredup force-pushed the config-glob branch 2 times, most recently from 80bd975 to 1c86fb4 Compare January 15, 2017 09:56
throw new FileLocatorFileNotFoundException(sprintf('The file "%s" does not exist.', $name));
}

return $name;
if (true === $first) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be qualified a bugfix for 2.7 i guess, $first seems to be ignored previously 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would always have returned only one result, so the check for $first wasn't necessary. But the glob could return more than one result, so the the check needed to be added

throw new FileLocatorFileNotFoundException(sprintf('The file "%s" does not exist.', $name));
}

return $name;
if (true === $first) {
return array_shift($files);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think return reset($files) is a faster operation..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

array_shift is used more commonly across the rest of the code base when doing a return. I don't think it makes much of a difference either way

}
$filepaths[] = $file;
}
$filepaths = array_merge($filepaths, glob($path.DIRECTORY_SEPARATOR.$name, defined('GLOB_BRACE') ? GLOB_BRACE : 0));
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer a quick return if $first=true and the first file is found.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Early return added

$this->import($resource, isset($import['type']) ? $import['type'] : null, isset($import['ignore_errors']) ? (bool) $import['ignore_errors'] : false, $file);
}
} else {
$this->import($import['resource'], isset($import['type']) ? $import['type'] : null, isset($import['ignore_errors']) ? (bool) $import['ignore_errors'] : false, $file);
Copy link
Member

@nicolas-grekas nicolas-grekas Jan 15, 2017

Choose a reason for hiding this comment

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

same question as FileLocator: is this "else" dead code or live code (same on the XmlFileLoader?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to rethink this part a bit, because without this, you don't get the exception if a file doesn't exist


if (array() !== $files = glob($resourceFile, defined('GLOB_BRACE') ? GLOB_BRACE : 0)) {
foreach ($files as $resource) {
$this->import($resource, XmlUtils::phpize($import->getAttribute('type')) ?: null, (bool) XmlUtils::phpize($import->getAttribute('ignore-errors')), $file);
Copy link
Contributor

Choose a reason for hiding this comment

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

The value of 'type' and 'ignore-errors' will not change within this loop, so it's better to resolve them before the loop, and then reuse the value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Variables added for the two values

@@ -106,7 +106,21 @@ private function parseImports(\DOMDocument $xml, $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);

$resourceFile = $import->getAttribute('resource');
Copy link
Member

Choose a reason for hiding this comment

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

Two notes here:

  • tests still pass if I revert this change on XmlFileLoader on my setup
  • I think this logic should be handled at the FileLoader level: I don't see why both Xml&Yaml loaders should care about that. They should be untouched to me.

@@ -34,4 +34,15 @@ public function __construct(ContainerBuilder $container, FileLocatorInterface $l

parent::__construct($locator);
}

protected function isAbsolutePath($file)
Copy link
Member

Choose a reason for hiding this comment

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

this looks like leaking logic from the locator, looks suspicious to me

@nicolas-grekas
Copy link
Member

While working on using glob for another use case in #21289, I found some similarities with this PR.
Here is what I did there. It looks like this PR could do almost exactly the same:

  • let the Config component untouched
  • move the glob call to FileLoader in the DI component

Before calling glob, the "resource" should be parsed for glob-specific characters. The constant part of the "resource" should be the part that is given the locator. Then, FileLoader could use glob on the result, with the glob tail appended.

In case this is not clear enough, this logic is implemented here:
https://github.com/symfony/symfony/pull/21289/files#diff-ad1ed76aba6a80df5a48dfa4585adcf3R61

@nicolas-grekas nicolas-grekas added this to the 3.3 milestone Jan 16, 2017
@pierredup pierredup force-pushed the config-glob branch 2 times, most recently from 3495bb1 to f14336c Compare January 17, 2017 13:56
foreach ($files as $file) {
if (is_dir($file)) {
$this->container->addResource(new DirectoryResource($file, '/^$/'));
parent::import($file, 'directory', $ignoreErrors, $sourceResource);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Directories should be handle by the DirectoryLoader class, otherwise we get an exception here.
Maybe we can deprecate the DirectoryLoader class in favor of using a glob?

@pierredup
Copy link
Contributor Author

@nicolas-grekas I have made the suggested changes, please have a look if that is what you had in mind.

For me the downside of keeping this in the DI component and out of the Config component, is that you loose the ability to load multiple configs in a container extension, E.G

$loader = new Loader\YamlFileLoader($container, new FileLocator(__DIR__ . '/../Resources/config'));
$loader->load('*.yml');

@pierredup pierredup force-pushed the config-glob branch 2 times, most recently from ed9f714 to f5fc458 Compare January 17, 2017 14:14
foreach ($files as $file) {
if (is_dir($file)) {
$this->container->addResource(new DirectoryResource($file, '/^$/'));
parent::import($file, 'directory', $ignoreErrors, $sourceResource);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Directories should be handle by the DirectoryLoader class, otherwise we get an exception here.
Maybe we can deprecate the DirectoryLoader class in favor of using a glob?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's ok as is, no need to deprecate yet, that wouldn't provide much benefit, while forcing everyone to upgrade, isn't it?

@pierredup pierredup force-pushed the config-glob branch 2 times, most recently from 6d58056 to f5cd0de Compare January 17, 2017 14:40
@fabpot
Copy link
Member

fabpot commented Feb 12, 2017

Coding standard should be fixed (cf. fabbot)

@nicolas-grekas
Copy link
Member

I suggest waiting for #21289 before working on this PR - it will need a rebase and share code (new glob method)

@nicolas-grekas
Copy link
Member

rebase unlocked, you can now use the new "glob" function:
https://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/Loader/FileLoader.php#L95

If you happen to chosse to use "true" as second argument, then I invite you to remove this argument altogether because then it's the only value used in the code base (I tend to think we should do that.)

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Feb 13, 2017

Thinking again about this one, I'd now say it should not handle directories at all - only files (meaning $recursive is required :) )

@pierredup pierredup force-pushed the config-glob branch 5 times, most recently from 621d473 to e8bed11 Compare February 13, 2017 19:32
@pierredup
Copy link
Contributor Author

Rebased and updated to use the new glob method.

I'd now say it should not handle directories at all - only files

Does that mean I also don't need to change the $type to directory when a path is a directory? Or is that part still fine?

@nicolas-grekas
Copy link
Member

I think that part should be removed yes: just skip directories.

@pierredup pierredup force-pushed the config-glob branch 2 times, most recently from 1afc9f1 to 21dcc1f Compare February 13, 2017 20:15
@pierredup
Copy link
Contributor Author

It seems like directory loading can't be skipped, it makes the current tests fail. So I think either $recursive then needs to be true, or we need to pass directory as the type if the path is a directory

@nicolas-grekas
Copy link
Member

This should do it, WDYT?

diff --git a/src/Symfony/Component/DependencyInjection/Loader/FileLoader.php b/src/Symfony/Component/DependencyInjection/Loader/FileLoader.php
index 794e242..41c9da6 100644
--- a/src/Symfony/Component/DependencyInjection/Loader/FileLoader.php
+++ b/src/Symfony/Component/DependencyInjection/Loader/FileLoader.php
@@ -90,7 +90,7 @@ abstract class FileLoader extends BaseFileLoader
         $extRegexp = defined('HHVM_VERSION') ? '/\\.(?:php|hh)$/' : '/\\.php$/';
 
         foreach ($this->glob($resource, true, $prefixLen) as $path => $info) {
-            if (!preg_match($extRegexp, $path, $m) || !$info->isFile() || !$info->isReadable()) {
+            if (!preg_match($extRegexp, $path, $m) || !$info->isReadable()) {
                 continue;
             }
             $class = $namespace.ltrim(str_replace('/', '\\', substr($path, $prefixLen, -strlen($m[0]))), '\\');
@@ -112,6 +112,11 @@ abstract class FileLoader extends BaseFileLoader
     private function glob($resource, $recursive, &$prefixLen = null)
     {
         if (strlen($resource) === $i = strcspn($resource, '*?{[')) {
+            if (!$recursive) {
+                yield $resource => new \SplFileInfo($resource);
+
+                return;
+            }
             $resourcePrefix = $resource;
             $resource = '';
         } elseif (0 === $i) {
@@ -134,9 +139,11 @@ abstract class FileLoader extends BaseFileLoader
                 if ($recursive && is_dir($path)) {
                     $flags = \FilesystemIterator::SKIP_DOTS | \FilesystemIterator::FOLLOW_SYMLINKS;
                     foreach (new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($path, $flags)) as $path => $info) {
-                        yield $path => $info;
+                        if ($info->isFile()) {
+                            yield $path => $info;
+                        }
                     }
-                } else {
+                } elseif (is_file($path)) {
                     yield $path => new \SplFileInfo($path);
                 }
             }
@@ -155,7 +162,7 @@ abstract class FileLoader extends BaseFileLoader
         }
 
         foreach ($finder->followLinks()->in($resourcePrefix) as $path => $info) {
-            if (preg_match($regex, substr($path, $prefixLen))) {
+            if (preg_match($regex, substr($path, $prefixLen)) && $info->isFile()) {
                 yield $path => $info;
             }
         }

foreach ($this->glob($resource, false) as $path => $info) {
parent::import($path, $type, $ignoreErrors, $sourceResource);
}
} catch (FileLocatorFileNotFoundException $exception) {
Copy link
Member

Choose a reason for hiding this comment

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

Should be named $e for consistency with the code base

@pierredup pierredup changed the title [Config] [DependencyInjection] Use glob pattern to load config files [DependencyInjection] Use glob pattern to load config files Feb 14, 2017
@pierredup
Copy link
Contributor Author

Changes made and tests passing again

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

👍

@fabpot
Copy link
Member

fabpot commented Feb 14, 2017

Thank you @pierredup.

@fabpot fabpot merged commit 519180e into symfony:master Feb 14, 2017
fabpot added a commit that referenced this pull request Feb 14, 2017
…files (pierredup)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[DependencyInjection] Use glob pattern to load config files

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

This relates to #21173, but I'm not sure if it completely fixes the issue.

This allows to use a glob pattern to load config files, which makes the following possible:

```
# config.yml
imports:
    - { resource: "*.yml" }
    - { resource: "folder/*.yml" }
    - { resource: "/etc/myapp/*.{yml,xml}" }
```

It can also be used in a container extension, if a bundle uses a lot of configs:

```
$loader = new Loader\YamlFileLoader($container, new FileLocator(__DIR__ . '/../Resources/config'));
$loader->load('*.yml');
$loader->load('routing/*');
```

Commits
-------

519180e Use glob pattern to load config file
@pierredup pierredup deleted the config-glob branch February 14, 2017 18:58
fabpot added a commit that referenced this pull request Feb 18, 2017
This PR was merged into the 3.3-dev branch.

Discussion
----------

added support for glob loaders in Config

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | not yet

In #21270, we added the possibility to use glob patterns to import (not load) config files, but it was restricted to the container. The same feature could be useful (and I actually have a use case) for the routing.

So, this PR moves the logic to the Config component. It also adds a new `GlobFileLoader` class that allows to load glob patterns (not just import them as in #21270).

Last, but not least, the new glob file loader is registered in both the routing and the container default loaders.

Here is a simple, but powerful example, using the Symfony micro kernel (actually, this is a snippet from the Kernel used in Symfony Flex :)):

```php
<?php

namespace Symfony\Flex;

use Symfony\Component\HttpKernel\Kernel as BaseKernel;

class Kernel extends BaseKernel
{
    use MicroKernelTrait;

    const CONFIG_EXTS = '.{php,xml,yaml,yml}';

    // ...

    protected function configureContainer(ContainerBuilder $container, LoaderInterface $loader)
    {
        $confDir = dirname($this->getRootDir()).'/etc';
        $loader->import($confDir.'/packages/*'.self::CONFIG_EXTS, 'glob');
        $loader->import($confDir.'/packages/'.$this->getEnvironment().'/**/*'.self::CONFIG_EXTS, 'glob');
        $loader->import($confDir.'/container'.self::CONFIG_EXTS, 'glob');
    }

    protected function configureRoutes(RouteCollectionBuilder $routes)
    {
        $confDir = dirname($this->getRootDir()).'/etc';
        $routes->import($confDir.'/routing/*'.self::CONFIG_EXTS, '/', 'glob');
        $routes->import($confDir.'/routing/'.$this->getEnvironment().'/**/*'.self::CONFIG_EXTS, '/', 'glob');
        $routes->import($confDir.'/routing'.self::CONFIG_EXTS, '/', 'glob');
    }
}
```

Commits
-------

025585d added support for glob loaders in Config
@fabpot fabpot mentioned this pull request May 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants