Load legacy module ini files only once

[discordier]

Fixes #30, replaces #32

Before the legacy module ini files were loaded and returned multiple times resulting in the config resolver trying to merge legacy configs which naturally does not work (and should not have happened at all in the first place).

Now the ini files are loaded only once and therefore not merged anymore.

[discordier]

Please adopt the unit test from https://github.com/contao/manager-plugin/pull/32/files#diff-06bee21664507bcc5932773f5f8e0c2a and add array_unique() in the mergeConfig() method.

Done

[discordier]

I don't think this will work in all cases. If my bundle has a dependency on a Contao 3 extension (e.g. "haste", I would simply create a new ModuleConfig in my plugin. The IniParser does not necessarily handle that.

This case would not be handled, correct. But I simply fail to see why on earth you would ship a module within your bundle. In your bundle, you pass ->setLoadAfter(['haste']) and are set.

Modules are defined to be located within system/modules and hence will get loaded from there - nothing else is possible for legacy extensions. I see absolutely no legitimate use case to create an additional ModuleConfig in your bundle. The only reason to do so might be to say "I want to be loaded BEFORE module haste" which the system explicitly does not allow at all.

For BundleConfigs this is different as multiple bundles might ship a configuration for, let's say, the AssetBundle or the like and therefore these get merged.

To put long story short, ModuleConfigs are not merged because they simply may not be created multiple times as the only lgeitimate way to create them is by traversing system/modules and trying to load each.

[discordier]

Any feedback @leofeyer, @aschempp?

[leofeyer]

I agree with what you have said about not merging module configs. 👍🏻 We need @aschempp's approval before I can merge, though.

[aschempp]

You're right it shouldn't be done. But is there a reason we can't handle that case?

[discordier]

You're right it shouldn't be done. But is there a reason we can't handle that case?

You mean aside from:

• introducing quite a bunch of code to support an usecase in legacy extensions that was not possible before
• hence qualifies as new feature of legacy code
• should never be done
• and is discouraged?

No. :smile_cat:

However, I do not have the time to rework the whole system to enhance functionality in that way as if we support that bundles also provide modules, other things will most likely break. One side effect that might arise from this is we then allow a bundle to enable a legacy module even if there is a .skip file present (as otherwise the module would already been have picked up by the ini parser) and hence override administrator decision to not load the module. etc.

If you provide a ModuleConfig without having the module installed in system/modules, other things might break as well as the config files are not present, however the bundle told the system to load the module.

If you can present a legitimate use case (that was working in Contao 3.5, as that is the whole purpose of the ModuleConfig class and that is not working anymore due to this), I am open to revisit this again. For the moment, I prefer to keep things as simple as possible and hence keep the legacy modules working as before but deprecated at the same time and not introducing more features for them.

[leofeyer]

Thank you @discordier.