Modules providing non-configurable field storage definitions can be uninstalled, leaving orphaned unpurged data

I posted a slightly different approach in #2298525-94: Test issue for Make ContentEntityDatabaseStorage handle changes in the entity schema definition. Not rerolling this as you made progress in parallel. I think the approach over there makes sense (it checks field data instead of the whole entity data), feedback welcome :)

Re #5: well, if this is not about field types, I'm not sure what the rationale is ?

This includes the new approach mentioned in #10. The aggregator changes are needed to fix the ConfigImportAllTest, that was failing with the following exception:

Drupal\aggregator\Entity\Feed::baseFieldDefinitions(Object)
Drupal\Core\Entity\EntityManager-
>buildBaseFieldDefinitions('aggregator_feed')
Drupal\Core\Entity\EntityManager-
>getBaseFieldDefinitions('aggregator_feed')
Drupal\Core\Entity\EntityManager-
>getFieldStorageDefinitions('aggregator_feed')
system_system_info_alter(Array, Object, 'module')
Drupal\Core\Extension\ModuleHandler->alter('system_info', Array, Object,
'module')
_system_rebuild_module_data()
system_rebuild_module_data()
drupal_flush_all_caches()
Drupal\Core\Config\ConfigImporter->flush(Array)
Drupal\Core\Config\ConfigImporter->doSyncStep('flush', Array)
Drupal\config\Form\ConfigSync::processBatch(Object, 'flush', Array)
call_user_func_array(Array, Array)
_batch_process(Array)
_batch_progress_page()
_batch_page(Object)
Drupal\system\Controller\BatchController->batchPage(Object)
call_user_func_array(Array, Array)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\PageCache->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1)
Stack\StackedHttpKernel->handle(Object, 1, 1)
Drupal\Core\DrupalKernel->handle(Object)

Which means the Feed.php is loaded before aggregator.module. Moving the constant to an autoloaded file fixes this.

Rerolled after #2337927: SqlContentEntityStorage::onFieldStorageDefinition(Create|Update|Delete)() is broken for base fields.

Sorry, wrong patch

+++ b/core/modules/aggregator/aggregator.module
@@ -11,11 +11,6 @@
-const AGGREGATOR_CLEAR_NEVER = 0;

+++ b/core/modules/aggregator/src/Entity/Feed.php
@@ -50,6 +50,11 @@
+  const CLEAR_NEVER = 0;

Itsy-bitsy API change that would need a CR, I guess, though I think it would make sense to move this fix to its own issue? Especially since I don't follow why the patch is exposing this.

I'm trying to wrap my head around the implications of this. Is there a specific way to reproduce the data integrity issue in HEAD? Also, we'll for sure need test coverage here.

At least $entity_type->isFieldable() needs to change to $entity_type->isSubclassOf('\Drupal\Core\Entity\ContentEntityInterface')

FieldableEntityinterface, actually, but yes.

So this is closely related to #2278017: When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken reference, which is currently blocked on #2324055: Split up the module manager into runtime information and extension information.. @chx suggested the solution being proposed there might be more robust than a system_info_alter(). Thoughts?

It probably makes sense to follow the same approach, so let's wait for #2278017: When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken reference to go in and figure out how to proceed here. Actually if we are able to address base field purging soon, we might even won't fix this.

#2278017: When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken reference is close thus getting started with this one.

Patch attached.

Attaching a version including #2278017: When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken reference for the test bot as well.

Fixed an accidental change in KernelTestBaseTest.

#2278017: When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken reference was committed.

This is already looking very good to me, I can't RTBC it though because I provided patches here.

Thanks. Re-uploading patch re-rolled against 8.0.x without any further changes.

yay no more hook_system_info_alter . can we remove those that were added prior? just a random note.

#2392293: Refactor hook_system_info_alter implementations to use ModuleUninstallValidatorInterface was opened for that

We should probably skip the fields in #15 to avoid *lots* of count queries being run for configurable fields. For configurable fields field data is going to be purged on uninstall anyway.

Also, as discussed with plach it would be nice to have countFieldData() available for all fieldable storages, but it looks like we miss an interface for them. I think it would make sense to create one and move it there though. It's a small API change, but in practice I doubt anyone is doing fieldable storage controllers which aren't based on ContentEntityStorageBase right now.

Discussed with @fago the introduction of FieldableEntityStorageInterface so we can simplify code a bit.

Also implemented @chx's suggestion from #36:

+++ b/core/lib/Drupal/Core/Entity/FieldableEntityStorageInterface.php
@@ -0,0 +1,46 @@
+   *   TRUE if the storage contains data, FALSE if not.
...
+  public function hasData();

There is #2391829: ContentUninstallValidator relies on the not required ContentEntityStorage::hasData() method for moving this one, so I'd suggest to leave it where it is for now to prevent two changes moving it around.

Fixed missing pieces from #38 and #40.

This should be green again.

That overlaps with #2392293: Refactor hook_system_info_alter implementations to use ModuleUninstallValidatorInterface, which has the 1:1 conversion but also with PHPUnit tests.

Reverted changes...

Meh

This looks good enough to me, RTBC anyone?

+++ b/core/lib/Drupal/Core/Field/FieldModuleUninstallValidator.php
@@ -0,0 +1,67 @@
+    foreach ($this->entityManager->getDefinitions() as $entity_type_id => $entity_type) {
+      // We skip entity types defined by the module as there must be no content
+      // to be able to uninstall them anyway. We also skip the Field module as
+      // it implements field purging.
+      // See \Drupal\Core\Entity\ContentUninstallValidator.
+      if ($entity_type->getProvider() != $module_name && $entity_type->isSubclassOf('\Drupal\Core\Entity\FieldableEntityInterface')) {
+        foreach ($this->entityManager->getFieldStorageDefinitions($entity_type_id) as $storage_definition) {
+          if ($module_name != 'field' && $storage_definition->getProvider() == $module_name) {

The $module_name != 'field' check in the inner if() should be moved first thing in the method ?

It should, thanks :)

+++ b/core/lib/Drupal/Core/Field/FieldModuleUninstallValidator.php
@@ -0,0 +1,69 @@
+    if ($module_name != 'field') {
+      foreach ($this->entityManager->getDefinitions() as $entity_type_id => $entity_type) {
...
+        // to be able to uninstall them anyway. We also skip the Field module as
+        // it implements field purging.

Thanks. Then the comment about why we skip field module should be moved accordingly.

Oh, god, I'm sorry :(

Had a quick look, found a couple of documentation issues.

1. +++ b/core/lib/Drupal/Core/Entity/FieldableEntityStorageInterface.php
   @@ -0,0 +1,38 @@
   + * Contains \Drupal\Core\Entity\DynamicallyFieldableEntityStorageInterface.
   + */
   

   s/Dynamically//

2. +++ b/core/lib/Drupal/Core/Entity/FieldableEntityStorageInterface.php
   @@ -0,0 +1,38 @@
   +/**
   + * A storage that supports entity types with dynamic field definitions.
   + *
   + * A storage that implements this interface can react to the entity type's field
   + * definitions changing, due to modules being installed or uninstalled, or via
   + * field UI, or via code changes to the entity class.
   + *
   + * For example, configurable fields defined and exposed by field.module.
   + */
   +interface FieldableEntityStorageInterface extends EntityStorageInterface {
   

   This documentation is still identical to DynamicallyFieldableEntityStorageInterface, but this contains a subset of that, and having the "dynamic" part removed I suppose this is not correct any more.

3. +++ b/core/lib/Drupal/Core/Entity/FieldableEntityStorageInterface.php
   @@ -0,0 +1,38 @@
   +  /**
   +   * Determines the number of entities with values for a given field.
   +   *
   +   * @param \Drupal\Core\Field\FieldStorageDefinitionInterface $storage_definition
   +   *   The field for which to count data records.
   +   * @param bool $as_bool
   +   *   (Optional) Optimises the query for checking whether there are any records
   +   *   or not. Defaults to FALSE.
   +   *
   +   * @return bool|int
   +   *   The number of entities. If $as_bool parameter is TRUE then the
   +   *   value will either be TRUE or FALSE.
   +   *
   +   * @see \Drupal\Core\Entity\FieldableEntityStorageInterface::purgeFieldData()
   +   */
   +  public function countFieldData($storage_definition, $as_bool = FALSE);
   

   This smells like it could better be split up in two methods: countFieldData() and hasFieldData(). That is probably out of scope for this issue.

4. +++ b/core/lib/Drupal/Core/Field/FieldModuleUninstallValidator.php
   @@ -0,0 +1,70 @@
   +        // We skip entity types defined by the module as there must be no content
   +        // to be able to uninstall them anyway.
   

   Oops! Comment exceeds 80 characters ;)

Fixed some more PHP docs.

1. +++ b/core/lib/Drupal/Core/Field/FieldModuleUninstallValidator.php
   @@ -0,0 +1,70 @@
   +                $reasons[] = $this->t('There is content for the field @field-name on entity type @entity_type.', array(
   

   content is what Node entities are called. Not everything is content. So this message could read something like "There is content for the field Blah on entity type Content"

2. +++ b/core/modules/system/src/Tests/Field/FieldModuleUninstallValidatorTest.php
   @@ -0,0 +1,141 @@
   +    // @todo: Use better field definition classes once there are any.
   

   An @todo without an issue - do we really want to leave this in? If we do it needs a better explanation of why it is there and what better means.

Also faics the reason text is never actually tested.

Fixed #57 and #58.

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 85764e4 and pushed to 8.0.x. Thanks!