In implementing proper dependency removal in the Search API and writing tests for it, I came across a strange behavior in the config dependency logic which I think constitutes a bug, since I can't imagine this would be desired behavior. (If it cannot be fixed, it should at least be clearly documented.)

The setup is as follows: Let's assume we have three config entities A, B and C. B has a dependency on A, and C has a dependency on B.

Now, when I delete entity A, the system determines that B and C will be affected, and the onDependencyRemoval() method of both B and C will be called, for B with A as the removed dependendcy, and for C with B.
However, if B succeeds in removing its dependency on A, it will not be deleted, thus rendering C's actions in reaction to the removal of B irrelevant. Contrary to this logic, though, the changes made by C will be saved anyways. (If C doesn't make changes, though, it will not be deleted – the current code correctly catches this case, it seems, maybe inadvertently, by setting $return['delete'] only after all onDependencyRemoval() methdos have been called.)

To summarize the order of events, maybe more clearly:

  1. A is deleted.
  2. ConfigManager::getConfigEntitiesToChangeOnDependencyRemoval() determines that B and C will be affected (C only because B might be deleted).
  3. C::onDependencyRemoval is called. (This is the order for me, but that might depend on the details. I think the bug will occur in any case, though.) It changes the entity and returns TRUE.
  4. ConfigManager::getConfigEntitiesToChangeOnDependencyRemoval() therefore saves it in $return['update'].
  5. B::onDependencyRemoval is called. It changes the entity and returns TRUE.
  6. ConfigManager::getConfigEntitiesToChangeOnDependencyRemoval() therefore saves it in $return['update'].
  7. Neither B nor C will be deleted, but both will be saved with the made changes.

There is actually code in ConfigManager::getConfigEntitiesToChangeOnDependencyRemoval() to take the constantly changing dependencies into account, but it doesn't work completely correctly. It seems to me as if the code assumes that the list of dependents (as returned by findConfigEntityDependentsAsEntities()) is ordered with "direct" dependents first and their dependents afterwards, but this is not actually the case. Maybe ensuring that order will be the best way to fix this issue, or maybe there is an even easier way.

CommentFileSizeAuthor
#12 2642374-12.patch6.45 KBalexpott
#12 2642374.12.test-only.patch2.7 KBalexpott
#11 2642374-11.patch3.75 KBalexpott
#10 goto.png25.6 KBDamienMcKenna
#9 config-manager-nested-2642374-9-goto.patch1.9 KBBerdir
#9 config-manager-nested-2642374-9.patch3.78 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drunken monkey created an issue. See original summary.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Priority: Major » Critical

Thanks @drunken monkey for the excellent issue report. @alexpott, @catch, and I all thought this issue might in fact be critical.

chx’s picture

chx CreditAttribution: chx as a volunteer commented

So the way I would do this is: when a config object gets deleted then its direct dependents gets collected and one by one getConfigEntitiesToChangeOnDependencyRemoval is called. If one of them decides to delete then the process from above repeats. If it doesn't delete then nothing will happen. Ie C never happens because C doesn't directly depend on A just B which now doesn't depend on A. End.

tl;dr: BFS instead of DFS.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Assigned: Unassigned »

i went to write a test for this, but it seems there already is one.

ConfigDependencyTest explicitly contains code to test transitive dependencies, and it passes.

@drunken monkey - can you share the Search API test code that triggers the issue?

i'll post a patch shortly with a simplified A, B, C test that seeks to reproduce the original issue so we have a simplified testing ground to reproduce this.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey for Acquia commented

@drunken monkey - can you share the Search API test code that triggers the issue?

It's in \Drupal\Tests\search_api\Kernel\DependencyRemovalTest::testBackendDependency(). There is a commented-out assertion at the bottom of the method along with a comment referencing this issue – if you re-add the assertion, the test fails.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

Is this maybe related to the same problem that we had in #2541206: Consider field storage dependency removal on Index?

There we had a problem that the function changed global state. And because it is called twice on form submission (once to re-build the form again, and then once on submit), it didn't work anymore on the submit call as it thought that it already got rid of the changes.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented

Huh. Good idea, you had me worried a bit there. But if I reset the storage cache and reload the index the result is the same, the test fails. I guess that's what you meant?
I think I also debugged this problem quite thoroughly back when I discovered it, and it also appears "in the wild", not just in tests – and the config manager acts as outlined in the issue summary.

Looking into the code again now I even see this code in \Drupal\Core\Config\Entity\ConfigDependencyManager::getDependentEntities():

    // Sort dependencies in the reverse order of the graph. So the least
    // dependent is at the top. For example, this ensures that fields are
    // always after field storages. This is because field storages need to be
    // created before a field.
    return array_reverse(array_intersect_key($this->graph, $dependencies));

If I understand this correctly, it seems this will order the dependencies on purpose the way that will break \Drupal\Core\Config\ConfigManager::getConfigEntitiesToChangeOnDependencyRemoval(). So maybe just do an array_reverse() there, too?
Anyways, first we probably really need a test case for this in Core to demonstrate it's wrong now and to verify that this would indeed fix it. (Just checked with the Search API test, though, and it seems: no, it doesn't. But didn't debug thoroughly this time.)

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

i don't think it's about resetting the storage, config entities don't use the static cache anyway by default.

The exapt change and explanation is in #18 there.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Active » Needs review
Issue tags: +Needs tests
FileSize
config-manager-nested-2642374-9.patch3.78 KB
config-manager-nested-2642374-9-goto.patch1.9 KB

Yeah, ignore my comments above, that's a different scenario.

I agree, there is indeed something missing. I think we need to do two changes:

1. Process them in reverse, as you already suggested.
2. That's not enough because we still have all dependencies in the initial loop. So what I'm doing is wrapping the foreach inside a do-while (TRUE). Then I can restart the foreach if something changed. Thinking about it, this might actually be a good use case for a goto statement. Never used that before in PHP. So, attaching a second patch that uses a goto.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
FileSize
goto.png25.6 KB

goto

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
2642374-11.patch3.75 KB

I've spent sometime staring at @Berdir's patches and I think we can do this without a goto or extra nesting of complexity.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
2642374.12.test-only.patch2.7 KB
2642374-12.patch6.45 KB

Drupal\Tests\search_api\Kernel\DependencyRemovalTest passes with #11. The test only patch is the interdiff :)

The last submitted patch, 12: 2642374.12.test-only.patch, failed testing.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Assigned: » Unassigned

nice patch.

not confident enough about the code to mark RTBC.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Awesome patch, greatly explained comments and test

+++ b/core/lib/Drupal/Core/Config/ConfigManager.php
@@ -302,7 +302,7 @@ public function getConfigEntitiesToChangeOnDependencyRemoval($type, array $names
-    $update_uuids = [];
+    $delete_uuids = $update_uuids = [];

the only nit, tb fixed on commit

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

@andypost what is wrong there? Both variables need to be initialised to an empty array.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

@alexpott I suppose they should be separate lines. But checked coding standards and found no mentions.

  • catch committed 7d029cb on 8.1.x 
    Issue #2642374 by alexpott, Berdir, drunken monkey, beejeebus:...

  • catch committed 63a1cfa on 8.0.x 
    Issue #2642374 by alexpott, Berdir, drunken monkey, beejeebus:...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Reviewed this a couple of times and can't find anything wrong with it, although also not 100% confident I'd spot something wrong with it if there is.

Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.