Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
98e109c to429163fCompare| 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 ($pathsas$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 noarray() !== ... 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 theglob should cover all current use cases.
| } | ||
| 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 thefile_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 to1c86fb4Compare| } | ||
| 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 theglob could return more than one result, so the the check needed to be added
| return$name; | ||
| if (true ===$first) { | ||
| returnarray_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 thinkreturn 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
| $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); |
nicolas-grekasJan 15, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 ($filesas$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
| $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.
| parent::__construct($locator); | ||
| } | ||
| protectedfunctionisAbsolutePath($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
nicolas-grekas commentedJan 16, 2017
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 tof14336cCompare| foreach ($filesas$file) { | ||
| if (is_dir($file)) { | ||
| $this->container->addResource(newDirectoryResource($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?
pierredup commentedJan 17, 2017
@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 tof5fc458Compare| foreach ($filesas$file) { | ||
| if (is_dir($file)) { | ||
| $this->container->addResource(newDirectoryResource($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 tof5cd0deComparefabpot commentedFeb 12, 2017
Coding standard should be fixed (cf. fabbot) |
nicolas-grekas commentedFeb 12, 2017
I suggest waiting for#21289 before working on this PR - it will need a rebase and share code (new |
nicolas-grekas commentedFeb 13, 2017
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.) |
nicolas-grekas commentedFeb 13, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thinking again about this one, I'd now say it should not handle directories at all - only files (meaning $recursive is required :) ) |
621d473 toe8bed11Comparepierredup commentedFeb 13, 2017
Rebased and updated to use the new glob method.
Does that mean I also don't need to change the |
nicolas-grekas commentedFeb 13, 2017
I think that part should be removed yes: just skip directories. |
1afc9f1 to21dcc1fComparepierredup commentedFeb 13, 2017
It seems like directory loading can't be skipped, it makes the current tests fail. So I think either |
nicolas-grekas commentedFeb 13, 2017
This should do it, WDYT? diff --git a/src/Symfony/Component/DependencyInjection/Loader/FileLoader.php b/src/Symfony/Component/DependencyInjection/Loader/FileLoader.phpindex 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; } } |
| try { | ||
| foreach ($this->glob($resource,false)as$path =>$info) { | ||
| parent::import($path,$type,$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.
Should be named $e for consistency with the code base
pierredup commentedFeb 14, 2017
Changes made and tests passing again |
nicolas-grekas left a comment
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.
👍
fabpot commentedFeb 14, 2017
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.ymlimports: - { 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 yetIn#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<?phpnamespace 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
Uh oh!
There was an error while loading.Please reload this page.
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: