Removing field from LB content type edits associated roles.

Ugh. Pointed out by @godotislate on Slack that my permission name is wrong in my test. I need to revisit and see how I can reproduce my problem with a test. =/

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies → .

Alright, I think I have figured out a way to demonstrate this...

I have put up two scenarios. In both branches, I have a content type w/ layout builder, a user role, and a field that references a media item. The scenario that I'm trying to prove is that when I delete a field, my user role is getting it's layout builder permission stripped from the role. I have one branch where this scenario passes, and in that case, the module that provides the media source plugin is named "a". In the same exact scenario, I can cause a failure by naming my module "z" instead. I think it's not likely to be controversial to say that the name of the module should not have this much affect on how these dependencies are getting processed...?

Now that I'm a little more confident that this is a core bug, I want to flirt with marking this as "MAJOR" b/c it really is unsuspected data loss that's occurring. I have to imagine that if there's a problem with how the dependency graph is built, that this could affect other unknown areas as well...?

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies.

Sorry, I was just trying to show a failing test where I know it's failing (10.4.x). I'm guessing this is also present in 11.x, but haven't made it that far yet.

Investigated this a little, and one thing that's happening is that when the config dependencies are being determined, in the erroneous state, the order of the dependencies returned from Drupal\Core\Config\ConfigManager::findConfigEntityDependencies() looks some thing like this:

1. default test node entity view display
2. test role
3. full test node entity view display

And when it behaves correctly, the order is like this:

1. test role
2. default test node entity view display
3. full test node entity view display

I haven't determined yet why this is happening, but for this specific case, the different ordering should be fine. The test role permission only has a dependency on the full entity display, and not the default one. So when ConfigManager::getConfigEntitiesToChangeOnDependencyRemoval acts on the list of of dependencies, it starts with the end of the list, and as long as the full entity display is processed before the role, everything should be fine.

The problem is that the list from findConfigEntityDependencies() is returned to findConfigEntityDependenciesAsEntities(), where entities are loaded from the list of config names. For efficiency, entities of the same type are loaded in one loadMultiple() operation instead of separate loads, and the order of the list returned becomes:

1. default test node entity view display
2. full test node entity view display
4. test role

With this ordering, the role is processed before the full entity view display and is erroneously handled to remove the permission.

I have put up MR 11141 with a fix for findConfigEntityDependenciesAsEntities(), to make sure the loaded entities stay in the order of the dependency config names. The tests in MR 11120 need to be updated for 11.x, but there were some issues with the config provided by the test modules that needs resolving, then the test can be added to the MR with the fix. Leaving this at Needs Work.

One thing I have discovered is that Drupal\Core\Config\ConfigManager::findConfigEntityDependencies() with the same parameters is not idempotent for the order of the dependencies.

This occurs because when the ConfigDependencyManager is instantiated in getConfigDependencyManager(), the order of config loaded the config factory can vary depending on how much config objects are loaded from the static cache vs database storage. On the first call to ConfigFactory::loadMultiple(), it's possible that only a partial list of config objects were loaded into the static cache, with the remainder needing to be loaded from database storage. On the subsequent calls, all the config objects may already be loaded in the static cache, so the returned order can be different from the first call.

The order the config data is loaded set at ConfigDependencyManager instantiations ultimately affects the sorted order of the dependencies. Trying to fix this by sorting the list of config objects does seem to make findConfigEntityDependencies() idempotent, but this breaks an existing test at Drupal\KernelTests\Core\Config\ConfigDependencyTest::testConfigEntityUninstallComplex().

I'm not sure the results need to be idempotent, either. As long as the order returned by findConfigEntityDependencies() has the correct relative order between dependencies, the overall order probably doesn't need to be consisitent. Meaning, if list of returned dependencies includes a config object (config 1) that depends on another (config 2) within the list, then as long as config 1 comes before config 2 in the list, this should be fine, for all listed dependencies. Investigating further whether true idempotency is required might be out of scope for this issue.

I've added a Kernel test to MR 11141 that creates a similar scenario with permissions being removed incorrectly, that demonstrates how the relative order can be broken per comment #9 when config entities are loaded.

Producing in browser reproduction steps has proven difficult, so the issue mgith be best be observed by the failing test-only job.