Config installer doesn't install possible installable config

We need to do some issue archeology to work out why a test for this specific case was added to Drupal\Tests\config\Functional\ConfigOtherModuleTest

So the ConfigInstaller used to do this until #2623940: ConfigInstaller tries to install optional config with missing dependencies - reading that issue points towards a better fix. But obviously we need to read it though and determine if we're now breaking something.

No interdiff cause new approach.

@Berdir pointed out that #8 might cause deleted configuration to suddenly reappear. That would be a problem. We need to add a test for that. #2 might not suffered from this problem. Since we're limiting it only to things with direct dependencies on the new configuration that is created.

Here's an approach that might satisfy both cases. We need more test coverage though.

I have tested this additionally. It looks good.

Small nitpick from patch.

+++ b/core/modules/config/tests/src/Functional/ConfigOtherModuleTest.php
@@ -64,10 +64,9 @@ public function testInstallOtherModuleFirst() {
+    $this->assertTrue(entity_load('config_test', 'other_module_test_optional_entity_unmet', TRUE), 'The optional configuration config_test.dynamic.other_module_test_optional_entity_unmet whose dependencies are met is not created.');

Assertion message should be adjusted.

I'm not sure are following cases covered in some of the tests, but they should be:

1. Import single configuration => optional configuration should not be imported
2. Import full site configuration, where module that triggers import of optional configuration is enabled => optional configuration should not be imported

Since I have fiddled a bit with tests to try single import, I have provided a patch here with mentioned changes. If it's of some use. I'm not sure if that test should be part of the optional configuration test or in import/export tests.

My uploads are strange in the last comment (#13) - here are correct ones.

Single import and full site import explicit don't look in a module's optional configuration so that's okay. What we do need to test is that deleted configuration is not suddenly reimported because of this.

I've basically reverted #14 since this doesn't add meaningful test coverage. The single import export form doesn't ever touch optional configuration or install anything other than the config object you provide it.

I've have however extended the test coverage to ensure that you can delete a configuration entity provided by a module and then install a module and ensure it does not magically come back again. New optional configuration is only installed if it could not possibly have existed before. I.e. the module being installed has to be in the dependency chain.

Thanks for testing @chr.fritsch nice find. First I'm going to expand the testing to have dependencies of dependencies as looking at https://github.com/BurdaMagazinOrg/thunder-distribution/pull/542 that's the problem.

Thanks thunder tests! So what's happening here is that we need the full configuration graph in order to determine proper dependencies.

Here's a 8.5.x patch for Thunder testing.

Here's a test for why we need the full dependency graph and a fix for ViewsKernelTestBase tests. This is needed because of the way it installs configuration out of order and so when we do $this->installConfig(['system']); we end up installing some optional views because they depend on system. This populates views data and the overrides from each test didn't take affect. Fun.

1. +++ b/core/modules/config/tests/src/Functional/ConfigOtherModuleTest.php
   @@ -62,12 +62,24 @@ public function testInstallOtherModuleFirst() {
        $this->assertNull(entity_load('config_test', 'other_module_test_optional_entity_unmet', TRUE), 'The optional configuration config_test.dynamic.other_module_test_optional_entity_unmet whose dependencies are not met is not created.');
        $this->installModule('config_test_language');
   +    $this->assertNull(entity_load('config_test', 'other_module_test_optional_entity_unmet2', TRUE), 'The optional configuration config_test.dynamic.other_module_test_optional_entity_unmet2 whose dependencies are not met is not created.');
   

   I thought we had actually replaced all entity_load() calls, but apparently #2723593: Properly deprecate entity_load() and friends never got in.

   Should we at least avoid it in the new calls and use ConfigTest::load()?

2. +++ b/core/modules/config/tests/src/Functional/ConfigOtherModuleTest.php
   @@ -62,12 +62,24 @@ public function testInstallOtherModuleFirst() {
   +    // The following configuration entity's are now met even though it has no
   +    // direct dependency on the module and it's dependency is not provided by
   +    // the being installed. The dependency already exists in configuration.
   +    $this->assertTrue(entity_load('config_test', 'other_module_test_optional_entity_unmet2', TRUE), 'The optional configuration config_test.dynamic.other_module_test_optional_entity_unmet2 whose dependencies are met is now created.');
   

   word missing? the ... module? ... being installed?

On the plus side, I successfully tested this with two different complex distributions and only had a single problem in a kernel test that was caused by an incorrectly named config file, created #2972003: Verify that the file name matches the ID when installing configuration.

1. Can we file a follow-up for that? We have to reset the cache every time we load just to be sure we're using up-to-date info.
2. Fixed.

1. Fine, lets reroll the other issue when this is in. It will conflict anyway. That said, config entities do currently not use the static cache by default, they have to opt-in, and only role is doing that currently. So that cache reset isn't doing anything at the moment, unless we'd opt-in to static caching.

Testbot Snafu.

1. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
   @@ -204,16 +203,19 @@ public function installOptionalConfig(StorageInterface $storage = NULL, $depende
   +      // on the thing being installed.
   

   Can we be more specific than thing?

2. +++ b/core/modules/config/tests/src/Functional/ConfigOtherModuleTest.php
   @@ -62,12 +62,25 @@ public function testInstallOtherModuleFirst() {
   +
   +    // The following configuration entity's are now met even though it has no
   +    // direct dependency on the module.
   

   Should this be configuration entity's dependencies?

@catch thanks for the review.

Re #1. Turned the sentence around so we didn't have to work out what thing is - it is an extension - either a module or a theme.
Re #2. Yep you're right. I tried to add more detail to help people / myself when I look at this in the future.

As all of the fixes are comments / test assertion text only setting back to rtbc.

This is theoretically just a bug fix, but in practice it could mean unexpected new configuration showing up in profiles and sites under development which feels behaviour-change-ish, so leaving in 8.6.x only for now. Would think about backporting but would we need a change notice for that? (Genuinely not sure).

Note that this caused a change in kernel tests where if one specified a module's config to be installed, any optional config for other enabled modules would be installed. This is demonstrated in the Message Digest module's recent fixes: #2981675: Tests are failing on 8.6. The workaround seems fine in that case, but perhaps a change notice is needed? It obviously didn't impact any core tests though.

@jhedstrom thanks for the report of the issue. Do you feel a CR would have helped you track down the fix? If so it'd be great to add one. I'm not sure of what to write that would be like a change record and inform the people that need to know.

As an aside I feel that KernelTestBase and the way it ignores configuration and module dependencies on configuration install is not really that great.

Do you feel a CR would have helped you track down the fix?

Honestly, probably not. I wouldn't have known what to search for until I found this issue I think.

As an aside I feel that KernelTestBase and the way it ignores configuration and module dependencies on configuration install is not really that great.

It's a bit of a mixed bag. It's great to not have to install every single dependency if the tested code doesn't need it, but it does lead to other issues. I think this new approach of installing installable config makes sense, without tipping too far the other way to requiring every dependency to be installed for all kernel tests.

This issue is missing a change record. Can we add one that can be linked in the 8.6.0 release notes? Thanks!

Added a change record.

Thanks for the change record. The CR says "This should not require any action from module authors or site administrators." But as @catch pointed out in #36, "in practice it could mean unexpected new configuration showing up in profiles and sites under development".

Under what circumstances would new configuration show up? And what if someone wanted to trigger this? I assume the only way to trigger a recursive installation of config would be to uninstall and re-install the module? Is that correct? (Asking for documentation and tutorial updating's sake.)