Problem/Motivation

Uninstalling modules can result in configuration entities being deleted that should not be.

Steps to reproduce:

  1. Install standard install profile
  2. Create a content type
  3. Uninstall the Menu UI module
  4. See that the body field on the new content type will be deleted!!!!

Proposed resolution

Fix config entity dependency graph to sort by number of vertices from the thing being removed.

The way the current directed acyclic graph is built does not have enough information to produce a consistent topographical sort. We need to add the module and theme information to the graph and change the direction to make the sort consistent.

HEAD behaviour

PATCH behaviour

Remaining tasks

User interface changes

None

API changes

?

Data model changes

?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
3.68 KB

The graph created by the ConfigDependency was the wrong way around - so that the topographical sort didn't quite work as expected.

mr.baileys’s picture

Just tried to install 8.2.x with this patch applied, and am getting

Fatal error: Call to a member function getConfigDependencyName() on null in /d8/core/modules/editor/src/Entity/Editor.php on line 100
Call Stack
#	Time	Memory	Function	Location
1	0.0006	246760	{main}( )	.../install.php:0
2	0.0066	1596040	install_drupal( )	.../install.php:39
3	1.0826	26161024	install_run_tasks( )	.../install.core.inc:115
4	1.1834	30078224	install_run_task( )	.../install.core.inc:536
5	1.1834	30078344	install_install_profile( )	.../install.core.inc:658
6	1.1834	30079952	Drupal\Core\ProxyClass\Extension\ModuleInstaller->install( )	.../install.core.inc:1553
7	1.1858	30576400	Drupal\Core\Extension\ModuleInstaller->install( )	.../ModuleInstaller.php:83
8	2.1768	44876448	Drupal\Core\ProxyClass\Config\ConfigInstaller->installDefaultConfig( )	.../ModuleInstaller.php:248
9	2.1769	44878216	Drupal\Core\Config\ConfigInstaller->installDefaultConfig( )	.../ConfigInstaller.php:75
10	2.3294	45299664	Drupal\Core\Config\ConfigInstaller->createConfiguration( )	.../ConfigInstaller.php:125
11	2.3576	46178280	Drupal\Core\Config\Entity\ConfigEntityBase->save( )	.../ConfigInstaller.php:321
12	2.3576	46178280	Drupal\Core\Entity\Entity->save( )	.../ConfigEntityBase.php:637
13	2.3576	46178312	Drupal\Core\Config\Entity\ConfigEntityStorage->save( )	.../Entity.php:358
14	2.3576	46178344	Drupal\Core\Entity\EntityStorageBase->save( )	.../ConfigEntityStorage.php:259
15	2.3576	46178424	Drupal\Core\Entity\EntityStorageBase->doPreSave( )	.../EntityStorageBase.php:389
16	2.3586	46178904	Drupal\Core\Config\Entity\ConfigEntityBase->preSave( )	.../EntityStorageBase.php:434
17	2.3591	46183832	Drupal\editor\Entity\Editor->calculateDependencies( )	.../ConfigEntityBase.php:346

Status: Needs review » Needs work

The last submitted patch, 4: 2754477-4.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
9.78 KB
8.72 KB
alexpott’s picture

Here's a test only to show that the current test only passes because the entity IDs created in a alphabetic sorting order :)

I think all we need is a web test of the steps-to-reproduce and a better issue summary to explain how the fix is done.

The last submitted patch, 7: 2754477-7.patch, failed testing.

The last submitted patch, 8: 2754477-test-only-7.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 8: 2754477-7.patch, failed testing.

alexpott’s picture

Fixing the tests and improves ConfigDependencyTest to ensure alphabetical order has not impact.

Status: Needs review » Needs work

The last submitted patch, 12: 2754477-12.patch, failed testing.

alexpott’s picture

Version: 8.1.x-dev » 8.2.x-dev

#2692247: Pre-existing 'forum' content type prevents forum module installation means we need to do this on 8.2.x first as core.entity_form_display.taxonomy_term.forums.default.yml moved.

alexpott’s picture

Status: Needs work » Needs review

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

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

mr.baileys’s picture

Applied the patch and went through the STR in #2751495-7: Uninstalling Scheduler 8.x-1.0-alpha1 deletes all fields on content types, scheduler is now properly uninstalled without removing fields and other configuration from the article content type.

alexpott’s picture

Issue summary: View changes
alexpott’s picture

alexpott’s picture

+++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigDependencyTest.php
@@ -221,14 +222,34 @@ public function testConfigEntityUninstall() {
+  /**
+   * Data provider for self::testConfigEntityUninstallComplex().
+   */
+  public function providerConfigEntityUninstallComplex() {
+    // Ensure that alphabetical order has no influence on dependency fixing and
+    // removal.
+    return [
+      [['a', 'b', 'c', 'd']],
+      [['d', 'c', 'b', 'a']],
+      [['c', 'd', 'a', 'b']],
+    ];
+  }

So this variation of sorting covers the Menu UI uninstall case so I think adding a specific test for that is unnecessary.

alexpott’s picture

alexpott’s picture

@swentel pointed out that the test talks about Entity A, B, C and D - this was because I renamed them in one version of the patch. However I realised that we wanted to test several different alphabetical orders so I reverted back to numbered variables and introduced the data provider approach.

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Went over the patch a couple of times, with @alexpott
Also did a manual test trying to uninstall menu ui and it now behaves like it should.

Only one more nit, but can be fixed on commit. Moving to RTBC to get more eyes on it.

+++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigDependencyTest.php
@@ -221,14 +222,34 @@ public function testConfigEntityUninstall() {
+  /**
+   * @dataProvider providerConfigEntityUninstallComplex
+   */
+  public function testConfigEntityUninstallComplex(array $entity_id_suffixes) {

$entity_id_suffixes isn't in the docblock, it also doesn't really say what it will test

alexpott’s picture

@swentel good point.

alexpott’s picture

Issue tags: +DevDaysMilan
alexpott’s picture

Creditting @swentel and @mr.baileys because they brought this issue to my attention and I discussed it with them. Plus they both manually tested the patch.

catch’s picture

Version: 8.2.x-dev » 8.1.x-dev

All the individual lines in the patch look fine, and conceptually this makes sense, but this is a tricky issue to be certain about.

However the added test coverage looks good, and if we find another edge case, we can add even more test coverage then. Also generally it's cool that we have a system robust enough that this is fixable, makes a nice change from the 6-8 migrations having to deal with bizarre data-inconsistencies in 6.x databases.

Leaving RTBC a bit (and would like this to sit in 8.2.x before it goes into 8.1.x), but happy with the RTBC.

  • catch committed 884aeef on 8.2.x
    Issue #2754477 by alexpott, mr.baileys, swentel: Unexpected config...
alexpott’s picture

Here's the patch for 8.1.x

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 30: 2754477-30.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
607 bytes
15.99 KB
611 bytes

Interesting the 8.1.x patch found another missing dependency.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Back to rtbc because the missing dependency in the default config means we don't sort in the correct order. The non-deterministic nature of topographical sorts when the dependencies are not correct means that in 8.1.x core.entity_view_display.taxonomy_term.forums.default is created before field.field.taxonomy_term.forums.forum_container. In 8.2.x this is not the case because the config is optional so the graph is different.

alexpott’s picture

#32 added a missing dependency to 8.2.x - @xjm correctly pointed out that this shows missing test coverage - i opened #2755843: The order in which config is saved affects dependency calculations to cover this case. If that patch doesn't land by the time we commit this to 8.1.x we should commit the 8.2 patch in #32 to keep the branches aligned.

jonathan1055’s picture

FileSize
93.33 KB

For D8.1.3 I have manually re-tested the steps in #2751495-7: Uninstalling Scheduler 8.x-1.0-alpha1 deletes all fields on content types with 2754477-32.patch. The uninstall works fine and the fields are not deleted.

scheduler uninstall

jonathan1055’s picture

Just for info, this core patch also fixes #2652036: Field widget set incorrectly after Scheduler module re-install and the un-install is much cleaner. Great news, thank you. Hopefully it can be committed to 8.1.x soon.

  • catch committed 0f4379a on 8.2.x
    Issue #2754477 by alexpott, jonathan1055, mr.baileys, swentel:...

  • catch committed 84c3578 on 8.1.x
    Issue #2754477 by alexpott, jonathan1055, mr.baileys, swentel:...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed df49801 and pushed to 8.2.x. Committed 84c3578 and pushed to 8.1.x. Thanks!

socketwench’s picture

alexpott’s picture

#2757395: Flag 8.x HEAD is broken because views need to depend on the flag that provides the relationship is occurring because of a missing dependency. I've posted a patch on the issue.

gclicon’s picture

Was there a specific reason for changing the structure of the return value for getDependentEntities()? Previously an array of arrays was return. With this update the an array of objects is returned instead. This breaks features #2761927: Fatal error: Cannot use object of type ConfigEntityDependency as array. If it was on purpose then changes will need to be made to the corresponding modules. If not, lets look into obtaining the same results without changing the structure of the return value.

alexpott’s picture

@gclicon I changed it to match the documentation - my bad. It was done because the graph now contains more objects which are not config entities. Prior to this change it only had config entities.

gclicon’s picture

Ah that makes sense. It was hard to follow why the change actually happened. So I wanted to check in first before fixing features. Thanks for the info and following up in the features issue as well. I'll jump over there to test the patch that was posted there.

ayalon’s picture

I think this patch introduced huge performance issues. It seems, that we are not the only on affected. But using page with more than 8 content types is almost unusable with features.
https://www.drupal.org/node/2765911

Someone has an idea what could be the reason for that?

arknoll’s picture

This change is causing our site installs to fail. It is failing on the view mentioned and any view with contains a relationship to a taxonomy term.

exception 'Exception' with message 'No entity type for field name on     [error]
view nir_assets' in
/docroot/core/modules/views/src/Plugin/views/HandlerBase.php:697
Stack trace:
#0
/docroot/core/modules/views/src/Plugin/views/field/Field.php(793):
Drupal\views\Plugin\views\HandlerBase->getEntityType()
#1
/docroot/core/modules/views/src/Plugin/views/field/Field.php(1010):
Drupal\views\Plugin\views\field\Field->getEntityFieldRenderer()
#2
/docroot/core/lib/Drupal/Core/Cache/CacheableMetadata.php(171):
Drupal\views\Plugin\views\field\Field->getCacheContexts()
#3
/docroot/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php(2286):
Drupal\Core\Cache\CacheableMetadata::createFromObject(Object(Drupal\taxonomy\Plugin\views\field\TermName))
#4
/docroot/core/modules/views/src/Entity/View.php(326):
Drupal\views\Plugin\views\display\DisplayPluginBase->calculateCacheMetadata()
#5
/docroot/core/modules/views/src/Entity/View.php(300):
Drupal\views\Entity\View->addCacheMetadata()
#6
/docroot/core/lib/Drupal/Core/Entity/EntityStorageBase.php(434):
Drupal\views\Entity\View->preSave(Object(Drupal\Core\Config\Entity\ConfigEntityStorage))
#7
/docroot/core/lib/Drupal/Core/Entity/EntityStorageBase.php(389):
Drupal\Core\Entity\EntityStorageBase->doPreSave(Object(Drupal\views\Entity\View))
#8
/docroot/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php(259):
Drupal\Core\Entity\EntityStorageBase->save(Object(Drupal\views\Entity\View))
#9
/docroot/core/lib/Drupal/Core/Entity/Entity.php(358):
Drupal\Core\Config\Entity\ConfigEntityStorage->save(Object(Drupal\views\Entity\View))
#10
/docroot/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php(637):
Drupal\Core\Entity\Entity->save()
#11
/docroot/core/lib/Drupal/Core/Config/ConfigInstaller.php(321):
Drupal\Core\Config\Entity\ConfigEntityBase->save()
#12
/docroot/core/lib/Drupal/Core/Config/ConfigInstaller.php(125):
Drupal\Core\Config\ConfigInstaller->createConfiguration('', Array)
#13
/docroot/core/lib/Drupal/Core/ProxyClass/Config/ConfigInstaller.php(75):
Drupal\Core\Config\ConfigInstaller->installDefaultConfig('module',
'nir_assets')
#14
/docroot/core/lib/Drupal/Core/Extension/ModuleInstaller.php(248):
Drupal\Core\ProxyClass\Config\ConfigInstaller->installDefaultConfig('module',
'nir_assets')
#15
/docroot/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php(83):
Drupal\Core\Extension\ModuleInstaller->install(Array, false)
#16 /docroot/core/includes/install.core.inc(1775):
Drupal\Core\ProxyClass\Extension\ModuleInstaller->install(Array,
false)
#17 [internal function]: _install_module_batch('nir_assets',
'Assets', Array)
#18 /docroot/core/includes/batch.inc(252):
call_user_func_array('_install_module...', Array)
#19 /docroot/core/includes/form.inc(870):
_batch_process()
#20 /docroot/core/includes/install.core.inc(609):
batch_process(Object(Drupal\Core\Url), Object(Drupal\Core\Url))
#21 /docroot/core/includes/install.core.inc(530):
install_run_task(Array, Array)
#22 /docroot/core/includes/install.core.inc(115):
install_run_tasks(Array)
#23 /usr/local/share/drush/includes/drush.inc(726):
install_drupal(Object(Composer\Autoload\ClassLoader), Array)
#24 /usr/local/share/drush/includes/drush.inc(711):
drush_call_user_func_array('install_drupal', Array)
#25 /usr/local/share/drush/commands/core/drupal/site_install.inc(80):
drush_op('install_drupal', Object(Composer\Autoload\ClassLoader),
Array)
#26 /usr/local/share/drush/commands/core/site_install.drush.inc(247):
drush_core_site_install_version('standard', Array)
#27 [internal function]: drush_core_site_install('standard')
#28 /usr/local/share/drush/includes/command.inc(366):
call_user_func_array('drush_core_site...', Array)
#29 /usr/local/share/drush/includes/command.inc(217):
_drush_invoke_hooks(Array, Array)
#30 [internal function]: drush_command('standard')
#31 /usr/local/share/drush/includes/command.inc(185):
call_user_func_array('drush_command', Array)
#32 /usr/local/share/drush/lib/Drush/Boot/BaseBoot.php(67):
drush_dispatch(Array)
#33 /usr/local/share/drush/includes/preflight.inc(66):
Drush\Boot\BaseBoot->bootstrap_and_dispatch()
#34 /usr/local/share/drush/drush.php(12): drush_main()
#35 {main}
arknoll’s picture

It appears that my issue in #46 was due to the new way dependencies are discovered/arranged. I moved the offending views to a later enabled module and the dependency problem was resolved.

To be clear this was not a problem in 8.1.3 and before. It became a problem when updating to 8.1.4 and later.

Status: Fixed » Closed (fixed)

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