Skip to content
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

added support for glob loaders in Config #21635

Merged
merged 1 commit into from Feb 18, 2017

Conversation

Projects
None yet
5 participants
@fabpot
Copy link
Member

commented Feb 16, 2017

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

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');
    }
}
$ret[] = $this->doImport($resource, $type, $ignoreErrors, $sourceResource);
}

return $ct > 1 ? $ret : $ret[0] ?? null;

This comment has been minimized.

Copy link
@fabpot

fabpot Feb 16, 2017

Author Member

This is ugly as load and import are not supposed to return anything, but the routing loaders do expect some return value (something that should probably be fixed at some point).

This comment has been minimized.

Copy link
@chalasr

chalasr Feb 16, 2017

Member

even if already ugly enough as is, do we really want this to require php 7+ for such syntactic sugar ???

This comment has been minimized.

Copy link
@fabpot

fabpot Feb 16, 2017

Author Member

fixed

@fabpot fabpot force-pushed the fabpot:glob-loader branch from fbbcf9f to 1ebbcf7 Feb 16, 2017

@@ -88,8 +68,12 @@ private function findClasses($namespace, $resource)
{
$classes = array();
$extRegexp = defined('HHVM_VERSION') ? '/\\.(?:php|hh)$/' : '/\\.php$/';
$prefixLen = null;
foreach ($this->glob($resource, true, $prefix) as $path => $info) {

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Feb 16, 2017

Member

if you call glob outside of the loop, you can compute prefixLen outside of the foreach

This comment has been minimized.

Copy link
@fabpot

fabpot Feb 16, 2017

Author Member

that does not work

$ret[] = $this->doImport($resource, $type, $ignoreErrors, $sourceResource);
}

return $ct > 1 ? $ret : isset($ret[0]) ? $ret[0] : null;

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Feb 16, 2017

Member

drop $ct then:
isset($ret[1]) ? $ret : ($ret ? $ret[0] : null) or
$ret ? (isset($ret[1]) ? $ret : $ret[0]) : null ?

This comment has been minimized.

Copy link
@fabpot

fabpot Feb 16, 2017

Author Member

which would be less readable

{
if (strlen($resource) === $i = strcspn($resource, '*?{[')) {
if (!$recursive) {
yield $resource => new \SplFileInfo($resource);

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Feb 16, 2017

Member

just in case, $prefix should be set to null here

This comment has been minimized.

Copy link
@fabpot

fabpot Feb 16, 2017

Author Member

done

*/
public function supports($resource, $type = null)
{
return 'glob' === $type;

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Feb 16, 2017

Member

add a check on $resource, like false === strpbrk($resource, '*?{[')?

This comment has been minimized.

Copy link
@fabpot

fabpot Feb 16, 2017

Author Member

Not needed as we force $type to be glob

This comment has been minimized.

Copy link
@lunetics

lunetics Mar 26, 2017

@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?

@@ -61,21 +61,28 @@ public function __construct(LoaderInterface $loader = null)
*/
public function import($resource, $prefix = '/', $type = null)

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Feb 16, 2017

Member

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?

This comment has been minimized.

Copy link
@fabpot

fabpot Feb 16, 2017

Author Member

Not about the "leak", as routing resources are tracked in a different file, so we cannot reused the same logic.

This comment has been minimized.

Copy link
@fabpot

fabpot Feb 16, 2017

Author Member

By the way, we have the same issue with the container.

This comment has been minimized.

Copy link
@fabpot

fabpot Feb 16, 2017

Author Member

I think there is nothing to fix here. Calling import 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.

@fabpot fabpot force-pushed the fabpot:glob-loader branch 2 times, most recently from a183027 to ce72d21 Feb 16, 2017

@nicolas-grekas nicolas-grekas added this to the 3.3 milestone Feb 18, 2017

@nicolas-grekas
Copy link
Member

left a comment

👍 after rebase

@fabpot fabpot force-pushed the fabpot:glob-loader branch from ce72d21 to 025585d Feb 18, 2017

@fabpot fabpot merged commit 025585d into symfony:master Feb 18, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Feb 18, 2017

feature #21635 added support for glob loaders in Config (fabpot)
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 deleted the fabpot:glob-loader branch Apr 7, 2017

@fabpot fabpot referenced this pull request May 1, 2017

Merged

Release v3.3.0-BETA1 #22603

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.