Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has acontribution guide which I suggest you to read.

In short:

• Always add tests
• Keep backward compatibility (seehttps://symfony.com/bc).
• Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://symfony.com/releases)
• Features and deprecations must be submitted against the 5.x branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@derrabus I would gladly do so but haven't found any tests related to theContainerConfigurator, could you point me out where to find them?

The more I read the original issue, the more I think that bundles are the appropriate way to create modules.

In the linked issue, you state that you want defaults: this is done by instantiating a PhpFileLoader in a bundle's extension load() method (see FrameworkExtension for an example).

You also write that "the bundle approach would not work for projects that use the DI component as a standalone library outside of Symfony.", but you examples all use the HttpKernel component, which is the one that provides the bundle system. It can be used standalone.

I made this proposal to add this method but having taken time to look closer at the code, I think that ContainerConfigurator should keep its focus on being another format to define DI config, and that it should be very close to what yaml/xml/etc formats provide.

For modules, I don't see why bundles wouldn't be a fit, even in your case.

@nicolas-grekas Doing the same in the bundle system requires quite a lot of boilerplate code which gives a tedious DX.
Bundles don't have direct access to theContainerConfigurator, and as you said, in order to be able to use it, one needs
to create an extension for the bundle, then load the configuration in yet a third file using thePhpFileLoader.

In essence:

1. Create a Bundle Class and link the extension class
2. Create an Extension Class and implement loading using thePhpFileLoader
3. Create a PHP file for the configuration.
4. Register the bundle in theconfig/bundles.php file.

All these steps are only for wiring things up so one can end up configuring the container.

It also means that part of the work of a bundle on the container is inside theservices.php file for the service definitions
and another part is either in the bundle or the extension for compiler passes.

My experience might be very limited compared to yours, but usually my use of compiler passes and the way the services are defined often change together, so it makes sense to have them close.

The bundle system makes a lot of sense for reusable code across multiple projects by forcing a standard approach that is easy to comprehend, but not so much for code that is specific to a single project.

The reason I wanted to go with a custom "Module" system instead of bundles was that it is quite a lot of work
for a simple task, i.e. Define the dependencies of an isolated part of the application.

What I had come up with was the following (once I had type hinted the kernel to use theContainerConfigurator):

1. Create aModuleConfigurator class with a methodconfigureContainer(ContainerConfigurator $container)
2. In the kernel, instantiate a ModuleConfigurator class in the configure method and simply:$module->configureContainer($container).

This ends up being a lot simpler and keeps all things container-related grouped together in a single bootstrapping class.

I made this proposal to add this method but having taken time to look closer at the code, I think that ContainerConfigurator should keep its focus on being another format to define DI config and that it should be very close to what yaml/xml/etc formats provide

I fail to see how this would break this contract of being very close to the other formats. The container is for specifying dependencies in a unified way, yet, some of its dependencies have to be set up differently (compiler passes) and in another location. Aren't compiler passes still a form of configuration of the container?

#40600 will allow getting the container by type-hinting it, wouldn't this solve your usecase?

When using the PHP-DSL, the ContainerBuilder is passed as a 2nd argument to the configuration closure:

return function (ContainerConfigurator $cc, ContainerBuilder $cb) {

Doesn't that solve your use case?

@nicolas-grekas Is is it available directly from the Kernel? In my case, I am loading configuration classes from there rather than using closures.

friendly ping@nicolas-grekas , thanks

Is is it available directly from the Kernel? In my case, I am loading configuration classes from there rather than using closures.

I don't think so from the kernel:configureContainer should accept either aContainerConfigurator or aContainerBuilder, not both. We could allow passing both objects. PR welcome if you wan to give it a try.

That being said, I had a look athttps://github.com/Morebec/orkestra-symfony-bundle/ and I think there might be something to improve in the design there: a bundle should not ship a kernel.

I get that's the way you managed to hook your module system, but I'd suggest looking for another approach instead. I can think of two and can't really decide which one would work or fit in the end:

• load modules in someconfig/packages/orkestra.php file which could be provided by a recipe, there you have the ContainerBuilder
• load modules in the bundle, aka in OrkestraSymfonyExtension, where you have acces to anotherContainerBuilder.

An alternative that requires less changes on your side might be to call$loader->load(function (ContainerBuilder $container) {... in yourconfigureContainer() method, which is already passed with a loader as second argument. SeeMicroKernelTrait for inspiration.

I'm going to close here because I'm not convinced that the described use case fits the purpose ofContainerConfigurator, and because I think the module system could be integrated in a more conventional way.

For the record, I'm proposing to add a 3rdContainerBuilder argument toconfigureContainer in#42991, that should help with your use case.