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.

Could the solution suggestion be flushed out a bit?

smustgrave → closed merge request !11120

smustgrave → closed merge request !11111

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.

Just going to add my $0.02 here as the OP... I pulled in the patch for MR 11141, and it's fixing the original problem I had. So I definitely 2nd this for RTBC status to get some core maintainer eyes on this issue.

I definitely don't think we need to have the layout_builder use case as a test for this, that was just the best way that I could come up with to demonstrate exactly how this bug was affecting my project. Just want to give a huge thanks for @godotislate & @smustgrave for kind of taking this bug report and running with it.

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!

catch → closed merge request !11141