Non-persistent fields become unpurgeable zombies without their target entity type

As discussed last night, I think as a first step, \Drupal\field\FieldUninstallValidator::validate() should be extended to also check for fields of entity types that would be removed by the uninstall.

Possibly only deleted fields, as non-deleted fields can just be removed if there is no data.

I'm wondering when exactly the field does get set to being purged status. if that only happens on uninstall due to the media bundles being deleted, then we might hit a weird state where the uninstall validator already did run?

In that case, a possibly alternative fix would be to check for a field being empty when deleting it and then immediately drop it, without going through the purge process.

Quoting @catch at #2831936-188: Add "File" MediaSource plugin:

+++ b/core/modules/media/media.install
@@ -28,6 +28,15 @@ function media_install() {
 
 /**
+ * Implements hook_uninstall().
+ *
+ * @TODO Remove when https://www.drupal.org/node/2884202 is fixed.
+ */
+function media_uninstall() {
+  \Drupal::moduleHandler()->invoke('field', 'cron');
+}
+

I don't mind that as a workaround, field purging is painful and the other issue looks potentially critical.

Emphasis mine.

Yeah, data integrity sadness. Let's promote this to at least review the potential criticality.

Here's a test-only patch for whoever wants to look into this bug without spending their lifetime running/waiting for \Drupal\system\Tests\Module\InstallUninstallTest :)

Thanks @amateescu! :)

So I think I was actually wrong in #2, at least the first part. The interesting thing is that we *explicitly* exclude entity types that are being uninstalled as well, with the argument that those need to have no data to be deleted anyway.

Which makes sense. Problem is that we also send empty fields through the purge process and therefore condem them to this zombie-state for eternity.

But what I wrote in the last paragraph should work. If a field is empty, there's no point in trying to purge something, so lets just remove it.

At least it makes this test path, but I had to stop using $this->container, because that module installer still had stale services injected. See #2066993: Use \Drupal consistently in tests.

Now the question is how many tests explicitly or implicitly expect that empty fields get purged.

Also, I would suggest we use something like "Problem is that we also send empty fields through the purge process and therefore condem them to this zombie-state for eternity." for the issue title ;)

This should fix those test fails. Updated some of the existing tests to specifically test deleting fields/storages without data because we have a whole separate test (BulkDeleteTest) for deleting and purging. I also updated some *very* old @todo's to point to the test that is actually testing the purging.

I think that gives us enough test coverage.

The new logic "delete the field directly if it doesn't have any data" makes perfect sense to me. Here's a few points that could be fixed:

1. +++ b/core/lib/Drupal/Core/Field/FieldModuleUninstallValidator.php
   @@ -46,6 +46,7 @@ public function validate($module_name) {
              foreach ($this->entityManager->getFieldStorageDefinitions($entity_type_id) as $storage_definition) {
   +
                if ($storage_definition->getProvider() == $module_name) {
   

   Unrelated change?

2. +++ b/core/modules/field/src/Entity/FieldConfig.php
   @@ -194,8 +195,13 @@ public static function preDelete(EntityStorageInterface $storage, array $fields)
   +    /** @var \Drupal\field\FieldConfigInterface $field */
   
   +++ b/core/modules/field/src/Entity/FieldStorageConfig.php
   @@ -409,8 +410,12 @@ public static function preDelete(EntityStorageInterface $storage, array $field_s
   +    /** @var \Drupal\field\FieldStorageConfigInterface $field_storage */
   

   Really minor point, but I usually prefer to describe the array itself, e.g. /** @var \Drupal\field\FieldConfigInterface[] $fields */. Feel free to ignore this one :)

3. +++ b/core/modules/field/src/Entity/FieldConfig.php
   @@ -194,8 +195,13 @@ public static function preDelete(EntityStorageInterface $storage, array $fields)
   +      $target_entity_storage = \Drupal::entityTypeManager()->getStorage($field->getTargetEntityTypeId());
   
   +++ b/core/modules/field/src/Entity/FieldStorageConfig.php
   @@ -409,8 +410,12 @@ public static function preDelete(EntityStorageInterface $storage, array $field_s
   +      $target_entity_storage = \Drupal::entityTypeManager()->getStorage($field_storage->getTargetEntityTypeId());
   

   Let's get the entity type manager outside the foreach, maybe at the top of the method like we do for $state.

4. +++ b/core/modules/system/tests/src/Functional/Module/UninstallTest.php
   @@ -21,7 +21,18 @@ class UninstallTest extends BrowserTestBase {
   +  /**
   +   * @group failing
   +   */
   +  public function testUninstallMedia() {
   +    \Drupal::service('module_installer')->uninstall(['media']);
   +
   +    \Drupal::entityManager()->clearCachedDefinitions();
   +
   +    \Drupal::service('module_installer')->uninstall(['file']);
   +  }
   

   Assuming that we want to keep this test, we need a description for the method and a couple of code comments inside.

Thanks!

1. Removed change.
2. Hm, I prefer it on the variable in the loop :)
3. Changed.
4. What about converting it to a kernel test? Verified it still fails.

Awesome!

Would be more awesome if I would not have used the wrong KernelTestBase class :)

And now with the right namespace, clearly it was too late.

Yeah, namespaces are tricky :)

@Cottser, @cilefen, @effulgentsia, and I discussed this issue and agreed that it' a critical bug as a data integrity problem that prevents un/reinstallation of modules without workaround, as well as interfering with cleanup/removal of data.

+++ b/core/modules/field/src/Entity/FieldConfig.php
@@ -189,13 +190,19 @@ public function calculateDependencies() {
+      // Only mark a field for purging if there is data. Otherwise, just remove
+      // it.
+      $target_entity_storage = $entity_type_manager->getStorage($field->getTargetEntityTypeId());
+      if (!$field->deleted && $target_entity_storage instanceof FieldableEntityStorageInterface && $target_entity_storage->countFieldData($field->getFieldStorageDefinition(), TRUE)) {
         $config = $field->toArray();

Is there a reason this hunk couldn't use $field->getFieldStorageDefinition()->hasData()?

Otherwise looks good, nice find, and maybe the first nail in the coffin of field purging.

Not really a reason, could change that. Only problem is that hasData is on FieldStorageConfig and not FieldStorageDefinitionInterface, so the IDE wouldn't be happy about it but we know (I think?) that it is a FieldStorageConfig.

Since it's functionally the same, I think it's OK to leave it like it is.

So I was/am looking at this for #2895685: Create release notes (a.k.a CHANGELOG) for Drupal 8.4.0-alpha1 and Drupal 8.4.0. Is this only a problem for empty fields? That seems to be the solution committed. It seems like so as per #7. Unfortunately the proposed solution did nto make it to the issue summary so trying to summarize for #2895685: Create release notes (a.k.a CHANGELOG) for Drupal 8.4.0-alpha1 and Drupal 8.4.0 myself.

Yes, this special case only occurred with empty fields, where it allowed to uninstall because it is empty but then ended up in limbo. Non-empty fields would need to be deleted first before you can uninstall.

This change however isn't just a bugfix for that, it also leads to a neat improvement during site building. I'm sure everyone had the situation where they picked the wrong field type or made a typo in the field name that they wanted to fix and then delete and re-create the field. In 8.3, this resulted in temporary purge tables and they were only really removed on cron, so especially when not running cron locally, you might end up with a considerable amount of those being-purged tables. Now they are immediately dropped as they are still empty.