Entity uninstallation does not clean up field schema data in state properly

You can certainly reproduce that by install beta 13, uninstall comment and update to HEAD/beta15

The underlying problem is that uninstalls doesn't cleanup those state entries ...

This sounds major and possibly critical

If I'm reading #2 correctly it sounds like uninstalling an entity-providing module leaves the entity/field definitions in state - so the system thinks there are tables there which are not?

Bumping to critical for now since this leaves sites in an unrecoverable state and uninstalling a module then updating a core version is fairly basic.

If this is not a generic problem and only something wrong with the beta13-15 update path then it could go back to major though.

If I'm reading #2 correctly it sounds like uninstalling an entity-providing module leaves the entity/field definitions in state - so the system thinks there are tables there which are not?

Exactly

First the test, that should come out red.

This could be green..... please.

+++ b/core/modules/system/system.module
@@ -1424,3 +1424,21 @@ function system_path_insert() {
+
+/**
+ * Implements hook_modues_uninstalled().
+ */
+function system_modules_uninstalled($modules) {
+  // Clean the storage schema of entities that are gone because their modules

We don't need to implement a hook for this, just put the logic in ModuleInstaller::uninstall().

ok, moved it.

1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -461,6 +461,19 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
   +    $key_value_store = \Drupal::keyValue('entity.storage_schema.sql');
   

   Do we need to take care of entity.definitions.installed as well?

2. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -461,6 +461,19 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
   +    foreach ($schema as $item_name => $item) {
   

   It feels like a better name for $item would be nice!

3. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -461,6 +461,19 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
   +      $entity_item = substr($item_name, 0, strpos($item_name, '.'));
   

   We should name it $entity_type_id

4. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -461,6 +461,19 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
   +      if(!in_array($entity_item, $entities)) {
   
   +++ b/core/modules/comment/src/Tests/CommentUninstallTest.php
   @@ -83,6 +83,21 @@ function testCommentUninstallWithoutField() {
   +      if($entity_item == 'comment') {
   

   Nitpick: code style

5. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   index ad98d68..948e68d 100644
   --- a/core/modules/comment/src/Tests/CommentUninstallTest.php
   
   --- a/core/modules/comment/src/Tests/CommentUninstallTest.php
   +++ b/core/modules/comment/src/Tests/CommentUninstallTest.php
   

   In an ideal world: \Drupal\system\Tests\Entity\EntitySchemaTest would be extended.

   In an ideal world we would also have an update path test.

1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -461,6 +461,19 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
   +    // Clean the storage schema of entities that are gone because their modules
   +    // were uninstalled.
   

   I think we can more simply word the comment.

   Delete entities from the storage schema if they are from modules that are not installed.

   Hm...

   We are getting all the entity definitions, and then deleting items from the store if they are not in that list.

   More general than only the case of a module being uninstalled. Oh, but maybe this is in uninstall code... Yep. helps to apply the patch and look at it in context. ;)

   Actually, I read this a few more times, and leaving the comment as is is probably fine.

2. +++ b/core/modules/comment/src/Tests/CommentUninstallTest.php
   @@ -83,6 +83,21 @@ function testCommentUninstallWithoutField() {
   +    $this->assertEqual($item_count, 0, 'After uninstallation the schema' .
   +      'still contains fields for the the comment entity.');
   +  }
    }
   

   why is this string split into two strings and concatenated?

   maybe to fit on 80 char line? it can just be all one line.

   On thinking some more, the string probably would be better as a comment before the assert. "After uninstalling, ensure there are no comment entity fields in the schema." Then, just let the assert give the default message.

3. also we want to keep the empty like that was before the closing of the class. https://www.drupal.org/node/608152#indenting

The fail in #8 was nice, just what was expected for the tests only patch.
The fail was:

fail: [Other] Line 101 of core/modules/comment/src/Tests/CommentUninstallTest.php:
After uninstallation the schemastill contains fields for the the comment entity.
Value 19 is equal to value 0.

Interesting, that we got both the custom message and the default X is equal to Y.

(not we should still not concatenate the message though)

After talking to @dawehner, I try to move the test code to EntiySchemaTest

@YesCT this long-awaited feature for assertions is from #2571291: In WebTest code, assertEqual and assertIdentical should always show the 2 values on failure. :)

We moved the test code to EntitySchemaTest.

After feedback from yched and plach we will try to move the fix to SqlContentEntityStorageSchema->onEntityTypeDelete().

+    $entities = \Drupal::entityManager()->getDefinitions();
+    foreach ($entities as $name => $definition) {
+      if ($definition->getProvider() == 'entity_test') {
+        $this->installEntitySchema($name);
+      };
+    }

Nice!

Now we just need to merge everything together properly

k4v and I are working together.

we figured out how to run the test on the command line! yay
php scripts/run-tests.sh --sqlite /tmp/db2.sqlite --dburl sqlite://localhost//tmp/db2.sqlite --class "\Drupal\system\Tests\Entity\EntitySchemaTest::testCleanUpStorageDefinition"

https://www.drupal.org/node/645286 helped us figure that out (with help from Sutharsan and @dawehner)

--

great. I'm going to do a few clean ups on the test.

let's let the testbot run on it in the mean time. setting to needs review.

oops, forgot the interdiff.

some standards and comment improvements on the test.

feedback on the test welcome. :) we are moving onto working on the fix.

This fixes a typo in the last patches.

+++ b/core/modules/system/src/Tests/Entity/EntitySchemaTest.php
@@ -139,4 +139,38 @@ public function testModifyingTranslatableColumnSchema() {
+    // Find all the entity types provided by the entity_test module and install
+    // the schema for them.
+    $entities = \Drupal::entityManager()->getDefinitions();
+    foreach ($entities as $name => $definition) {
+      if ($definition->getProvider() == 'entity_test') {
+        $this->installEntitySchema($name);
+      };
+    }
...
+    // Count the storage definitions that begin with entity_test.
+    foreach (array_keys($schema) as $storage_definition_name) {
+      $entity_item = substr($storage_definition_name, 0, strpos($storage_definition_name, '.'));
+      if ($entity_item == 'entity_test') {
+        $item_count++;
+      }
+    }

What about recording all the entity types above and check all of them after the uninstall?

So finally we moved the fix to SqlContentEntityStorageSchema->onEntityTypeDelete().

oh... regarding #31
I think I see. The concern is that the entity_test module is providing storage definitions that start with entity_test. and also other patterns.
and then we are only checking that the ones that match exactly the modulename. are being deleted.

we need to find a list of the ones entity_test provides and then make sure all of those were deleted.

We think we addressed all the concerns.

Before we we only deleting storage definitions that began with the module name. Now we delete storage definitions that begin with entity type ids that are provided by the module.

Also we count the right stuff in the test now and corrected the assertion message.

¡¡¡¡¡¡¡¡¡¡¡YAY!!!!!!!!!!

As far as we know we're done and are looking forward for thorough reviews.

Overall this looks kinda clean, just some non optional variable names.

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -377,6 +377,17 @@ public function onEntityTypeDelete(EntityTypeInterface $entity_type) {
   +      if($entity_type_id == $entity_type->id()) {
   

   Some space after the if would be nice

2. +++ b/core/modules/system/src/Tests/Entity/EntitySchemaTest.php
   @@ -139,4 +139,42 @@ public function testModifyingTranslatableColumnSchema() {
   +    foreach ($entities as $name => $definition) {
   +      if ($definition->getProvider() == 'entity_test') {
   +        $this->installEntitySchema($name);
   +        $entity_type_ids[] = $name;
   

   We could rename $name to $entity_type_id and things would be just way clearer.

3. +++ b/core/modules/system/src/Tests/Entity/EntitySchemaTest.php
   @@ -139,4 +139,42 @@ public function testModifyingTranslatableColumnSchema() {
   +      $entity_item = substr($storage_definition_name, 0, strpos($storage_definition_name, '.'));
   +      if (in_array($entity_item, $entity_type_ids)) {
   

   If we rename $entity_item to $entity_type_id

4. +++ b/core/modules/system/src/Tests/Entity/EntitySchemaTest.php
   @@ -139,4 +139,42 @@ public function testModifyingTranslatableColumnSchema() {
   +    $this->assertEqual($item_count, 0, 'After uninstalling entity_test module the schema still contains fields from entities provided by this module.');
   

   The message should be written for the case in which its properly working

+++ b/core/modules/system/src/Tests/Entity/EntitySchemaTest.php
@@ -139,4 +139,42 @@ public function testModifyingTranslatableColumnSchema() {
+    $item_count = 0;

Let's rename $item_count to $containing_entity_type_ids

This looks great.

One possible suggestion to add while incorporating the feedback above:

+++ b/core/modules/system/src/Tests/Entity/EntitySchemaTest.php
@@ -139,4 +139,42 @@ public function testModifyingTranslatableColumnSchema() {
+    // Uninstall the entity_test module.
+    $this->container->get('module_installer')->uninstall(array('entity_test'));
+
+    // Get a list of all the entities in the schema.
+    $key_value_store = \Drupal::keyValue('entity.storage_schema.sql');
+    $schema = $key_value_store->getAll();
+
+    $item_count = 0;

For completeness sake (even though I'm sure this is tested elsewhere), I think it would be good to ensure these storage schemas are present prior to disabling the module, so we're absolutely sure of this test. A simple count while installing schemas, then an assertion of that count should suffice.

@dawehner Thank you for reviewing this again. Sorry about 1. You mentioned it before, and I thought I checked it was done... it *is* now!

ok. I am on it.

just addressing #37

I will do #42 next.

@jhedstrom thank you for reviewing this.

addressing #42. makes sure there are some storage definitions provided by the entity_test module, before we try and make sure there are none after uninstalling the module.

back to needing comprehensive review.

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -377,6 +377,17 @@ public function onEntityTypeDelete(EntityTypeInterface $entity_type) {
+      $entity_type_id = substr($storage_definition_name, 0, strpos($storage_definition_name, '.'));

+++ b/core/modules/system/src/Tests/Entity/EntitySchemaTest.php
@@ -139,4 +139,59 @@ public function testModifyingTranslatableColumnSchema() {
+      $entity_type_id = substr($storage_definition_name, 0, strpos($storage_definition_name, '.'));

list($entity_type_id, ) = explode('.', $storage_definition_name, 2); would be a bit easier to read?

Flight is delayed, I'm @airport

One thing that just came to my mind: This patch does not fix the original problem upgrading from Beta 13 to HEAD. Should there be code to work around this bug to make upgraders happy?

I think about putting the cleanup routine in the update hook. Feedback?

Working on a small improvement

@k4v:

Yes, feel free to write a db update function. It will need to detect all data that's not belonging to an installed entity type returned by the entity manager. We'll need also a db update test to check that this works properly.

This should be slightly cleaner

@k4v:

Please file a follow-up for the update stuff, I discussed this with @dawehner and @YesCT and we agreed that part is not critical. We should be done here now, thanks!

@k4v
We discussed this here again. We need an update path, as otherwise things are broken.

@k4v
When do you have boarding? We can work on the update here pretty easy.

I'll work on the test.

change in #51 makes a lot of sense. thanks @plach

Patch looks great! Just for completeness can we post a test-only patch?

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -367,9 +367,14 @@ public function onEntityTypeDelete(EntityTypeInterface $entity_type) {
+      // If we have a field having dedicated storage we need to drop it,

"have...having" sounds a bit odd, maybe just "have...with"?

Here's the test for the upcoming test function, this is now expected to fail.

And here is the update function, improved also test coverage.

Beside this I think this is ready to go.

+++ b/core/modules/system/system.install
@@ -1648,5 +1648,20 @@ function system_update_8007() {
+    list($entity_type_id, ,) = explode('.', $key);

I'd use explode(, , 2)

+++ b/core/modules/system/src/Tests/Entity/EntitySchemaTest.php
@@ -139,4 +139,59 @@ public function testModifyingTranslatableColumnSchema() {
+      $entity_type_id = substr($storage_definition_name, 0, strpos($storage_definition_name, '.'));
...
+      $entity_type_id = substr($storage_definition_name, 0, strpos($storage_definition_name, '.'));

+++ b/core/modules/system/system.install
@@ -1593,10 +1593,15 @@ function _system_update_create_block($name, $theme_name, array $values) {
+    list($entity_type_id, , ) = explode('.', $item_name);

@@ -1643,5 +1648,20 @@ function system_update_8007() {
+    list($entity_type_id, ,) = explode('.', $key);

Let's use the explode(, , 2) all over the place.

I will work on that now.

since I just want the first thing, I dont need the ,2.

Thank you YescCT, RTBC conce its green

Its green now, thank you

Silly things that can be fixed on commit:

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -367,9 +367,14 @@ public function onEntityTypeDelete(EntityTypeInterface $entity_type) {
   +      // If we have a field having dedicated storage we need to drop it,
   

   Nit: "have … having" is strange.

2. +++ b/core/modules/system/src/Tests/Entity/EntitySchemaTest.php
   @@ -139,4 +139,59 @@ public function testModifyingTranslatableColumnSchema() {
   +    // Get a list of all the entities in the schema.
   

   s/entities/entity types/

   I could be wrong.

Nice work everyone. Committed 3287afb and pushed to 8.0.x. Thanks!

I didn't address #68 - can be cleanup in a followup.