[DependencyInjection] [Routing] [Config] Recursive directory loading #13246

lavoiesl · 2015-01-04T17:00:28Z

As asked by @stof, this PR is a remake of #11059 on the 2.7 branch.

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#11045, #11059
License	MIT
Doc PR	TODO

See those about creating a cookbook for better file organization:
Issue: symfony/symfony-docs#3680
PR: symfony/symfony-docs#3885

This PR allows to specify a directory to load and it will recursively load all files from it, using the LoaderResolver.

Example

# app/config/config.yml
imports:
    - { resource: 'dev/' }

└── app
   ├── config
   │  ├── common
   │  │  └── framework.yml
   │  │  └── assetic.yml
   │  └── config.yml

It also works for routing configuration:

_common:
    resource: "common/"
    type: directory

Advantages

Allows easy separation of configuration files per bundle.
This could eventually allow a bundle installation tool that would automatically install needed configuration files.
Could superseed the AnnotationDirectoryLoader, so it does not duplicate the logic.
Is entirely optional and introduces no BC break.
Files are loaded lexically with scandir, allowing someone to control the load order through a number prefix.
Even if the initial configuration load may be a little bit longer, it does not impact the compiled configuration (production).

Graceful degradation

In FrameworkBundle and HttpKernel, the DirectoryLoader is only added if available. This means you can have FrameworkBundle 2.7 with Routing and DependencyInjection 2.6 and everything will work, except the directory loading. As asked by @stof, the version requirements are now 2.7 for both.

Conflicts

For now, the Routing DirectoryLoader requires the type directory to be specified so it does not conflict with AnnotationDirectoryLoader. However, this could be refactored.

~~The type declaration requires a patch in AsseticBundle for it to be used on framework.router.resource. The patch is already merged in master, but not in a release yet.~~ Released in 2.5.0. Changing the main router would be like this:

framework:
    router:
        resource: "%kernel.root_dir%/config/routing/common/"
        type: directory

lavoiesl · 2015-01-04T17:01:29Z

As of now, some Travis tests fail for a reason I can’t identify. The tests run fine on my computer.

nicolas-grekas · 2015-01-04T17:04:23Z

To reproduce on your local computer:
cd src/Symfony/Component/...
composer up for "high" tests or composer up --prefer-lowest --prefer-stable "low" ones
phpunit

lavoiesl · 2015-01-04T17:07:00Z

@nicolas-grekas And do you know what could be causing the issue? AFAIK, the class is in the proper place. Isn’t the autoloader supposed to do its job?

lavoiesl · 2015-01-04T17:10:42Z

Oh, nevermind, it is actually pretty simple: it downloads the symfony/dependencyinjection which hasn’t the class yet. So this is expected to fail and will be resolved when merged.

nicolas-grekas · 2015-01-04T17:31:25Z

You're right for "high" tests. But merging won't solve "low" tests unless you state that ~2.7 is the minimum allowed version (in components that relies on your newly added class). One or more composer.json need an update

aitboudad · 2015-01-04T19:29:55Z

src/Symfony/Component/DependencyInjection/Loader/DirectoryLoader.php

+     */
+    public function supports($resource, $type = null)
+    {
+        return 'directory' === $type || (!$type && preg_match('/\/$/', $resource) === 1); // ends with a slash


I'd prefer to check is_dir instead of is ended with a slash

After extensive testing, I had to remove is_dir checks in both loaders because ->supports() is not aware of the current location. Therefore, trailing slash is mandatory. I also updated the tests to reflect this.

aitboudad · 2015-01-04T22:18:32Z

👍

lavoiesl · 2015-01-04T22:27:10Z

The errors remaining in Travis are not related to this PR.

@nicolas-grekas I added graceful degradation instead of bumping the requirements. See the updated description.

@stof If nobody else has issues with this, I believe this is ready now, thanks for your input.

lavoiesl · 2015-01-04T22:59:13Z

src/Symfony/Component/HttpKernel/Kernel.php

+            $loaders[] = new DirectoryLoader($container, $locator);
+        }
+
+        $resolver = new LoaderResolver($loaders);


I am wondering why the default set of loaders is written in HttpKernel, shouldn’t there be something like Symfony\Component\DependencyInjection\Loader\LoaderResolver that would contain the set of default loaders? This is out of scope of this PR though.

stof · 2015-01-18T15:56:08Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/RoutingResolverPass.php

@@ -30,6 +30,12 @@ public function process(ContainerBuilder $container)

        $definition = $container->getDefinition('routing.resolver');

+        // routing.loader.directory only appears in Routing 2.7


I would rather bump the requirement, so that people using FrameworkBundle 2.7 can be sure about which loaders are supported

Issue #11045 For now, the Routing DirectoryLoader requires the type `directory` to be specified so it does not conflict with `AnnotationDirectoryLoader`. However, this could be refactored.

lavoiesl · 2015-01-18T16:24:34Z

Errors in Travis not related to my PR.

webda2l · 2015-04-16T08:42:43Z

Too late for 2.7? Will be a nice addition.

nicolas-grekas · 2015-05-20T01:19:23Z

Cherry-picked in #14700 thank you @lavoiesl!

…ectory loading (lavoiesl, nicolas-grekas) This PR was merged into the 2.8 branch. Discussion ---------- [DependencyInjection] [Routing] [Config] Recursive directory loading | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #13246, #11045, #11059 | License | MIT | Doc PR | - Commits ------- 60b1c5b Added CHANGELOG entries, cleanups 73f0ee2 [DI][Routing] recursive directory loading

aitboudad reviewed Jan 4, 2015
View reviewed changes

lavoiesl reviewed Jan 4, 2015
View reviewed changes

fabpot force-pushed the 2.7 branch from 77b6cca to dcfc338 Compare January 8, 2015 13:06

fabpot added DependencyInjection Routing Config labels Jan 16, 2015

stof reviewed Jan 18, 2015
View reviewed changes

lavoiesl and others added 2 commits January 18, 2015 11:16

[DI][Routing] recursive directory loading

1259a93

Issue #11045 For now, the Routing DirectoryLoader requires the type `directory` to be specified so it does not conflict with `AnnotationDirectoryLoader`. However, this could be refactored.

do not add DirectoryLoader if not available

51677b1

lavoiesl added 2 commits January 18, 2015 11:20

code style fixes

2be2d56

Revert f7f5dd7: bump version requirements instead

3ff25ac

nicolas-grekas mentioned this pull request May 20, 2015

[DependencyInjection] [Routing] [Config] Recursive directory loading #14700

Merged

nicolas-grekas closed this May 20, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DependencyInjection] [Routing] [Config] Recursive directory loading #13246

[DependencyInjection] [Routing] [Config] Recursive directory loading #13246

lavoiesl commented Jan 4, 2015

lavoiesl commented Jan 4, 2015

nicolas-grekas commented Jan 4, 2015

lavoiesl commented Jan 4, 2015

lavoiesl commented Jan 4, 2015

nicolas-grekas commented Jan 4, 2015

aitboudad Jan 4, 2015

lavoiesl Jan 4, 2015

aitboudad commented Jan 4, 2015

lavoiesl commented Jan 4, 2015

lavoiesl Jan 4, 2015

stof Jan 18, 2015

lavoiesl commented Jan 18, 2015

webda2l commented Apr 16, 2015

nicolas-grekas commented May 20, 2015

		@@ -30,6 +30,12 @@ public function process(ContainerBuilder $container)

		$definition = $container->getDefinition('routing.resolver');

		// routing.loader.directory only appears in Routing 2.7

[DependencyInjection] [Routing] [Config] Recursive directory loading #13246

[DependencyInjection] [Routing] [Config] Recursive directory loading #13246

Conversation

lavoiesl commented Jan 4, 2015

Example

Advantages

Graceful degradation

Conflicts

lavoiesl commented Jan 4, 2015

nicolas-grekas commented Jan 4, 2015

lavoiesl commented Jan 4, 2015

lavoiesl commented Jan 4, 2015

nicolas-grekas commented Jan 4, 2015

aitboudad Jan 4, 2015

Choose a reason for hiding this comment

lavoiesl Jan 4, 2015

Choose a reason for hiding this comment

aitboudad commented Jan 4, 2015

lavoiesl commented Jan 4, 2015

lavoiesl Jan 4, 2015

Choose a reason for hiding this comment

stof Jan 18, 2015

Choose a reason for hiding this comment

lavoiesl commented Jan 18, 2015

webda2l commented Apr 16, 2015

nicolas-grekas commented May 20, 2015