-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
Conversation
98e109c
to
429163f
Compare
@@ -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)) { |
There was a problem hiding this comment.
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 )
There was a problem hiding this comment.
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)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no BLOG_BRACE here?
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
array_merge
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
80bd975
to
1c86fb4
Compare
throw new FileLocatorFileNotFoundException(sprintf('The file "%s" does not exist.', $name)); | ||
} | ||
|
||
return $name; | ||
if (true === $first) { |
There was a problem hiding this comment.
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 😕
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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..
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Early return added
1c86fb4
to
a8dbb2a
Compare
$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); |
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
a8dbb2a
to
1a5955b
Compare
@@ -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'); |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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
While working on using glob for another use case in #21289, I found some similarities with this PR.
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: |
3495bb1
to
f14336c
Compare
foreach ($files as $file) { | ||
if (is_dir($file)) { | ||
$this->container->addResource(new DirectoryResource($file, '/^$/')); | ||
parent::import($file, 'directory', $ignoreErrors, $sourceResource); |
There was a problem hiding this comment.
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?
@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
|
ed9f714
to
f5fc458
Compare
foreach ($files as $file) { | ||
if (is_dir($file)) { | ||
$this->container->addResource(new DirectoryResource($file, '/^$/')); | ||
parent::import($file, 'directory', $ignoreErrors, $sourceResource); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
6d58056
to
f5cd0de
Compare
Coding standard should be fixed (cf. fabbot) |
I suggest waiting for #21289 before working on this PR - it will need a rebase and share code (new |
rebase unlocked, you can now use the new "glob" function: 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.) |
Thinking again about this one, I'd now say it should not handle directories at all - only files (meaning $recursive is required :) ) |
621d473
to
e8bed11
Compare
Rebased and updated to use the new glob method.
Does that mean I also don't need to change the |
I think that part should be removed yes: just skip directories. |
1afc9f1
to
21dcc1f
Compare
It seems like directory loading can't be skipped, it makes the current tests fail. So I think either |
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) { |
There was a problem hiding this comment.
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
21dcc1f
to
c19f8f0
Compare
c19f8f0
to
519180e
Compare
Changes made and tests passing again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Thank you @pierredup. |
…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
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
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:
It can also be used in a container extension, if a bundle uses a lot of configs: