Removing field from LB content type edits associated roles

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.

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
3. 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.

Could the solution suggestion be flushed out a bit?

Proposed resolution updated.

Closed the older MRs to make it more clear

Test-only feature

1) Drupal\KernelTests\Core\Config\ConfigDependencyTest::testDependencyOrder
Failed asserting that false is true.
/builds/issue/drupal-3504368/core/tests/Drupal/KernelTests/Core/Config/ConfigDependencyTest.php:694
FAILURES!
Tests: 10, Assertions: 213, Failures: 1.

If showing coverage
Don't see any open threads.

Believe this could be ready but not as familiar so will give +1 but leave in review for others to also chime in.

Going to take a swing and go ahead and mark it. If too soon apologize!

Note that in MR 11141, the test does not address Layout Builder permissions and field configuration specifically, but does generic config entity dependency order in a similar fashion. I had difficulty recreating the previous test MR that was against 10.4.x to 11.x, and a kernel test is more efficient than a functional test.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rebased and removed the unused variable flagged by the nr-bot.

Rebase seems fine.

One question on the MR - either an easy fix or an easy 'no it's not possible'.

Updated per MR review.

Feedback appears to be addressed for this one.

Sorry this took a while to get back to, just sheer volume in the RTBC queue at the moment ,meaning I didn't spot it was back at RTBC.

Committed/pushed to 11.x, 11.1.x, and 10.5.x thanks!