search

contao / manager-plugin

Contao Manager Plugin
GNU Lesser General Public License v3.0
4 stars 9 forks source link

Fix issue that loading order permutates per system #27

Closed discordier closed 4 years ago

discordier commented 4 years ago

As discussed on Mumble on 2020-07-02, we want reproducible builds (as everyone else). However, Contao creates a bundles.map which can vary per host, directory, etc. despite using the very same composer.lock for input. This fixes that behaviour by sorting the loading order keys and therefore have an deterministic output of the resolved dependencies.

discordier commented 4 years ago

While working on this, I just discovered some more problems related to non deterministic states and even a bug handling of loadAfter. I will update this PR and add the relevant tests.

Thanks to @ausi for converting it to draft and the fruitful discussion.

m-vo commented 4 years ago

Just for the record, the issue we discussed was this:

Two bundles Foo + Bar both ship a dependency Lib and each create a bundle config for it:

// Foo's Plugin#getBundles() returns
[
  BundleConfig::create(FooBundle::class),
  BundleConfig::create(LibBundle::class)->setLoadAfter([FooBundle::class])
]

// Bar's Plugin#getBundles() returns
[
  BundleConfig::create(BarBundle::class),
  BundleConfig::create(LibBundle::class)->setLoadAfter([BarBundle::class])
]

Is Lib now still guaranteed to be loaded after Foo and Bar? (i.e.: are the bundle config's constraints merged?)

Toflar commented 4 years ago

Thanks for picking this up @discordier <3 Reproducible builds FTW!

discordier commented 4 years ago

I have updated my implementation and also added a FIXME we might want to discuss.

discordier commented 4 years ago

~~The coding standard issue in here is not trivial to resolve, as it causes test failure then. However, it is correct that the order might change again, I will update this. We can not switch to identity matching via same though, as the config itself will become a new, merged, config. I refrained from using each of the original configs here, as they could, in theory, be used elsewhere and therefore manipulating them will open a new can of worms there then.~~

Implemented a foreach in dfb04e7

discordier commented 4 years ago

While playing with shuffling the bundle adding order, @baumannsven and I discovered that apparently even the manager-bundle does not provide sufficient loadAfter definitions and depends on the adding order of bundles. I have not yet tracked down WHY the loading order of extensions of plain symfony bundles suddenly matters when loaded via manager-bundle versus a plain symfony application but IMO the loadAfter rules must be added there as well or the behaviour of the extension loading must be adapted so that the order does NOT matter anymore. Test case was:

  1. swap ksort($loadingOrder) with krsort($loadingOrder).
  2. call composer install again.
  3. suddenly %kernel.secret% (from FrameworkBundle) is not resolvable anymore.

I fear I'm up to open one can of worms after another here... did not think that such a small finding like the reproducable builds could generate such impact.

Currently we are shuffling the configs in the Plugin of the contao-core bundle.

discordier commented 4 years ago

The results of our tests became contao/contao#1903, we were not able to find more issues. I wonder if we now need to place a conflict here in the manager-plugin.

leofeyer commented 4 years ago

What is the status of this issue?

discordier commented 4 years ago

Waiting for decision(s) on:

discordier commented 4 years ago

As discussed on Mumble on 2020-07-30:

discordier commented 4 years ago

All done from my side, ready for review! :tada:

leofeyer commented 4 years ago

Thank you @discordier.