Let me tell you a sad Drupal story:

  1. Imagine a module called Pastafazoul. It provides a content entity type. It also provides a file field and field storage in its default configuration (although this bug would happen with any field type), attached to the entity type it provides. The important thing is that the field storage does not persist if there are no fields. Imagine also that the module provides some other form of configuration, like a bundle or view display, which uses, and therefore depends on, that field.
  2. You install Pastafazoul. All is hunky-dory.
  3. Without having created any content, you decide to uninstall Pastafazoul. When you try, you get a screen confirming the configuration changes that will occur as a result. Among those changes is that the field and field storage will be deleted. So far, so good.
  4. You uninstall Pastafazoul. The field, as expected, is deleted. However, it is not purged. But that's OK, because purging is done during cron runs (field_cron()). The uninstall goes OK otherwise, and the entity type it provided no longer exists. All good.
  5. You now try to uninstall File (let's pretend that Pastafazoul was the only thing depending on it), but it won't let you because it says that File has fields pending deletion. Ah, no problem, you think -- you haven't run cron yet.
  6. You run cron and try again to uninstall File. It still says that File has fields pending deletion.
  7. Mildly puzzled, you run cron again. Then you try to uninstall File again. It still says that File has fields pending deletion.
  8. You run cron until you are blue in the face. File continues to insist that has fields pending deletion. You try again and again, eventually go insane, and die of old age. Later, the sun becomes a red giant and incinerates the Earth. Still the damn module sits there, welded to your site, insisting that it has fields pending deletion.

What went wrong?

The problem lies in core/modules/field/field.purge.inc, circa line 92:

    if (!isset($info[$entity_type])) {
      continue;
    }

The entity type to which the field was attached was destroyed when Pastafazoul was uninstalled, but the field was merely deleted, with the expectation of being purged later. Therefore the isset() failed, and the field was not purged.

Meanwhile, \Drupal\field\FieldUninstallValidator has been loading that deleted, effectively unpurgeable field (see the getFieldStorageByModule() method). Because it was able to load the deleted field, it thinks the field is still waiting to be purged (and it's not wrong). Therefore, it is not allowing File to be uninstalled, because as far as it can tell, there are file fields pending deletion.

So in other words: if you remove a content entity type by uninstalling the module that provides it, any fields attached to that entity type will stick around forever, in limbo, deleted but not purgeable. And the modules which provide those fields will never be uninstallable, because they will continue to think that their fields are pending deletion -- not realizing that those fields are trapped in a state where they can never be purged.

I think this is a pretty major bug because it can crap up a Drupal site with zombie fields, prevent modules from being uninstalled, and trip up core tests like InstallUninstallTest, which expect to be able to install and uninstall modules in a logical way. Not sure what the right fix would be -- I suspect there are several possible approaches -- but we need to start thinking about this.

This bug was turned up by InstallUninstallTest and #2831936: Add "File" MediaSource plugin.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

phenaproxima created an issue. See original summary.

Berdir’s picture

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.

Wim Leers’s picture

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.

xjm’s picture

Priority: Major » Critical

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

amateescu’s picture

FileSize
1.42 KB

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 :)

Berdir’s picture

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 the testing fixture container in tests instead of \Drupal.

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

Berdir’s picture

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 ;)

phenaproxima’s picture

Title: Non-persistent fields can never be purged without their target entity type » Non-persistent fields become unpurgeable zombies without their target entity type

Wish granted. :)

Status: Needs review » Needs work

The last submitted patch, 6: non_persistent_fields-2884202-6.patch, failed testing. View results

Berdir’s picture

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.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.