Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
added support for glob loaders in Config#21635
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
| ++$ct; | ||
| $ret[] =$this->doImport($resource,$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.
This is ugly asload andimport are not supposed to return anything, but the routing loaders do expect some return value (something that should probably be fixed at some point).
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.
even if already ugly enough as is, do we really want this to require php 7+ for such syntactic sugar???
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.
fixed
| $classes =array(); | ||
| $extRegexp =defined('HHVM_VERSION') ?'/\\.(?:php|hh)$/' :'/\\.php$/'; | ||
| $prefixLen =null; | ||
| foreach ($this->glob($resource,true,$prefix)as$path =>$info) { |
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 you call glob outside of the loop, you can compute prefixLen outside of the foreach
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.
that does not work
| ++$ct; | ||
| $ret[] =$this->doImport($resource,$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.
drop $ct then:isset($ret[1]) ? $ret : ($ret ? $ret[0] : null) or$ret ? (isset($ret[1]) ? $ret : $ret[0]) : null ?
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.
which would be less readable
| protectedfunctionglob($resource,$recursive, &$prefix =null,$ignoreErrors =false) | ||
| { | ||
| if (strlen($resource) ===$i =strcspn($resource,'*?{[')) { | ||
| if (!$recursive) { |
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.
just in case, $prefix should be set to null 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.
done
| */ | ||
| publicfunctionsupports($resource,$type =null) | ||
| { | ||
| return'glob' ===$type; |
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.
add a check on $resource, likefalse === strpbrk($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.
Not needed as we force$type to beglob
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.
@nicolas-grekas could the check be done at some other place, see#22160
Actually just a check if a glob pattern is used but not the correct type (type: glob) is configured to return a meaningful errormessage?
| * | ||
| * @throws FileLoaderLoadException | ||
| */ | ||
| publicfunctionimport($resource,$prefix ='/',$type =null) |
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.
in case of a glob, this is missing tracking of the directory prefixing the pattern - see "$this->container->fileExists($resourcePrefix, '/^$/');" in DI.
Should we do it inline here? Would be like a leak but do we have a better choice?
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.
Not about the "leak", as routing resources are tracked in a different file, so we cannot reused the same logic.
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.
By the way, we have the same issue with the container.
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 there is nothing to fix here. Callingimport does not imply that you were using a glob. However, the previous implementation made this assumption and always added the direction as a resource, even for non-globs, I don't think this is something we want.
a183027 toce72d21Compare
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.
👍 after rebase
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
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
GlobFileLoaderclass 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 :)):