-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[DependencyInjection] [Routing] [Config] Recursive directory loading #11059
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
class DirectoryLoader extends FileLoader | ||
{ | ||
/** | ||
* @param mixed $file The 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.
should be @param string ?
Seems like the test failures are not related to this PR. |
The doc PR is great but I fail to see how this is related to this PR. I'm 👎 for this PR as it makes the loading of files non-deterministic (or put another way, random). As the order of file loading is important, automatically loading a directory is not possible/desirable. |
I must disagree, I am far from alone asking about directory loading, this PR merely adds support for it. It does not force you to use it and I did not even change anything about Symfony itself. Loading configuration in this fashion is very useful when you have scripts that dump config file in a folder. Multiplying the configuration files also helps maintaining grouped configurations and reduces the risk of merge conflicts. As an example, a small project I am currently working on has 30 config files, 11 parameter files, 9 routing files and so far, everyone loves the new structure. As for your point about non-deterministic, this is false, As for the “relation” of the linked doc PR, it is because there have been discussions around directory loading while talking about it. It also seems that if my PR was accepted, this is exactly the section of the Cookbook in which we would talk about it. |
@fabpot For DI files, the order may not be significant depending of how you organize your files. I know that some people are splitting the semantic configuration into a separate file per bundle. In this case, the order of these files is not important as they don't override each other. This would make the import much easier to do in such case. |
and regarding the doc PR, it was documenting the usage of such a loader at some point, before figuring out that it is not available currently in core. |
Routing currently uses |
Regarding the "non-deterministic loading problem", I can't really understand the differences with the current situation. This works today and loads routes from an infinite number of controller files whose order you cannot control:
The proposed solution works exactly the same, with the added advantage that you can easily prefix numbers to files and directories (as suggested by @stof and as done for the Doctrine fixture files) to control the load order (you cannot do that with controller classes):
|
Should this be closed given that #11045 was closed? |
It seems to me that the merging of this PR is being blocked for obscure reasons and no good chance have been given to it. Here is an account of all the arguments I can recall: Pros:
Cons:
@weaverryan, Do you think this would be of any benefit? (I’m talking about DX here). |
@lavoiesl you've certainly listed a lot of good reasons to promote this PR. I wish you can finally make it! |
This could indeed help people. @lavoiesl could you rebase your branch on top of the 2.7 branch (and resubmit the PR to the 2.7 branch) to give it a chance to make it in ? |
Can someone give me a hand, the Travis build is failing on |
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. The type declaration requires a [patch in AsseticBundle](symfony/assetic-bundle@f76291c) for it to be used on `framework.router.resource`. The patch is already merged in master, but not in a release yet.
@stof You were right for |
…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
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
It also works for routing configuration:
Advantages
AnnotationDirectoryLoader
, so it does not duplicate the logic.Conflicts
For now, the Routing DirectoryLoader requires the type
directory
to be specified so it does not conflict withAnnotationDirectoryLoader
. However, this could be refactored.The type declaration requires a patch in AsseticBundle for it to be used onReleased in 2.5.0. Changing the main router would be like this:framework.router.resource
. The patch is already merged in master, but not in a release yet.