Hey!

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

Symfony has a contribution guide which I suggest you to read.

In short:

• Always add tests
• Keep backward compatibility (see https://symfony.com/bc).
• Bug fixes must be submitted against the lowest maintained branch where they apply (see https://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 the ContainerConfigurator, 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 the ContainerConfigurator, 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 the PhpFileLoader.

In essence:

1. Create a Bundle Class and link the extension class
2. Create an Extension Class and implement loading using the PhpFileLoader
3. Create a PHP file for the configuration.
4. Register the bundle in the config/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 the services.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 the ContainerConfigurator):

1. Create a ModuleConfigurator class with a method configureContainer(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 a ContainerConfigurator or a ContainerBuilder, 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 at https://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 some config/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 another ContainerBuilder.

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

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

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