Skip to content
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

Load the security bundle after the framework bundle #1903

Conversation

baumannsven
Copy link
Contributor

In the extension of the security bundle, the parameter %kernel.secret% is used. This provides the framework bundle. If it is not set, an error will occur after running composer (install|update).

This has changed in version 4.4.1 of the security bundle.
https://github.com/symfony/security-bundle/blob/v4.4.1/DependencyInjection/SecurityExtension.php#L66

The Contao 4.4 LTS is not affected by this.

The error can be reproduced in the method buildLoadingOrder of the ConfigResolver which shuffle the loading order.
https://github.com/contao/manager-plugin/blob/442c3b1360b2caa784cd38aa8b2997545f20f3e1/src/Bundle/Config/ConfigResolver.php#L95

uksort($loadingOrder, function() { return rand() > rand() ? 1 : -1; });

return $loadingOrder;

And additionally in the manager bundle plugin the order of the security bundle and framework bundle is changed.

BundleConfig::create(FrameworkBundle::class),
BundleConfig::create(SecurityBundle::class),

@baumannsven baumannsven changed the base branch from master to 4.9 July 4, 2020 12:55
@leofeyer leofeyer added the bug label Jul 4, 2020
@leofeyer leofeyer added this to the 4.9 milestone Jul 4, 2020
@leofeyer
Copy link
Member

leofeyer commented Jul 4, 2020

Shouldn't we make sure to always load the Symfony framework bundle before all other Symfony bundles? Just like we always load the Contao manager bundle first:

https://github.com/contao/manager-plugin/blob/ad489738c9d5e3079286e5e582efffb4bcb8c265/src/Composer/ManagerPluginInstaller.php#L262-L266

Maybe there are other Symfony bundles that react differently depending on whether the framework bundle is installed or not, too.

@baumannsven
Copy link
Contributor Author

Therefore the bundles, which refer to what framework bundle should be marked with the set load after. I check this again.

@baumannsven
Copy link
Contributor Author

I have now placed the framework bundle at the end in the manager bundle plugin. To test if the other bundles which are defined before don't cause an error. So everything should work with this pull request.

@leofeyer
Copy link
Member

leofeyer commented Jul 8, 2020

Thank you @baumannsven.

@leofeyer leofeyer changed the title The security bundle may only be loaded after the framework bundle Load the security bundle after the framework bundle Jul 9, 2020
discordier added a commit to discordier/manager-plugin that referenced this pull request Aug 15, 2020
We now conflict with all previous 4.9 manager-bundle versions, as these
do not contain the fixes from contao/contao#1903 which are needed to not
break things for existing installations.
leofeyer pushed a commit to contao/manager-plugin that referenced this pull request Aug 18, 2020
Description
-----------

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.

Commits
-------

91d1173 Fix issue that loading order permutates per system

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.
e510734 Merge configurations when bundles are provided multiple times

When two extensions provide the same bundle to the system and both
provide a config for `loadAfter` and `replace`, the values must get
merged to a common array.
When at least one of each sets `loadInProduction` or
`loadInDevelopment`, the result must also get loaded then and not only
the one coming arbitrary "last".

Variance in config types is explicitely not handled so far and marked as
FIXME for the moment as I have no clue how to handle these and the use
case seems too narrow to handle them explicitely.
bdc7339 Handle `setReplace` and  `setLoadAfter` sorted

To have predictable output, the results must be sorted, for
functionality the order does not matter so this is fine.
d9788e1 Fix coding standard issues
dfb04e7 Fix for assertEquals => assertSame in test

Now we have `assertSame` back at the expense of needing a loop.
However, the result is now guaranteed to exactly match the expected
output in order and contents.
8d8207b Yet another coding standard issue
172bedf Runtests five times with mt_srand(0) til mt_srand(4)

We now run the tests five times with different random seeds to ensure
the random sorting is predictable as well.
This is a compromise between having "random" input values and
predictable test output.
587f945 Drop FIXME

After discussion we decided to keep the exception for the moment and
revisit this as soon as some one encounteres a problem with it.
5561c93 Add conflict with Contao <4.9.4

We now conflict with all previous 4.9 manager-bundle versions, as these
do not contain the fixes from contao/contao#1903 which are needed to not
break things for existing installations.
96fc04c Replace `ksort` with `md5` comparison
4eb0b15 CI
@baumannsven baumannsven deleted the bugfix/missing_load_security_after_framework branch March 9, 2022 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants