Problem/Motivation

  • Proper existence/functioning of field data requires that both the module providing the field storage definition (FSD) and the module providing the field type are installed.
  • For configurable fields, the module providing the FSD is field.module, and field_system_info_alter() makes the field type's module required. Meanwhile, each field type module lists field.module as its dependency, which maybe doesn't make sense anymore, but as a result, that's effectively making field.module required so long as there's any configurable field, which is the desired result.
  • For non-configurable fields, the module defining the FSD can/should add the field type's module as a dependency, so as long as the FSD-providing module is installed, we're successfully preventing uninstallation of the type's module.
  • But, there's currently nothing preventing the FSD-providing module itself from being uninstalled.

Postponed on #2278017: When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken reference (and thus also on #2335879: Change SqlContentEntityStorageSchema::requiresEntityDataMigration() to ask the old storage handler if it has data rather than assuming yes unless NULL storage which is blocking 2278017).

Proposed resolution

  • Add code to system_system_info_alter() to make FSD-providing modules required if they have data.

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Add automated tests Instructions
Draft a change record for the API changes Instructions

User interface changes

API changes

CommentFileSizeAuthor
#59 entity-field_prevent_uninstall-2338873-59.patch14.75 KBplach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,537 pass(es). View
#59 entity-field_prevent_uninstall-2338873-59.interdiff.txt2.66 KBplach
#55 entity-field_prevent_uninstall-2338873-55.patch14.69 KBplach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,472 pass(es). View
#55 entity-field_prevent_uninstall-2338873-55.interdiff.txt706 bytesplach
#54 interdiff.txt2.09 KBswentel
#54 entity-field_prevent_uninstall-2338873-54.patch14.91 KBswentel
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,473 pass(es). View
#52 entity-field_prevent_uninstall-2338873-52.patch15 KBplach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,399 pass(es). View
#52 entity-field_prevent_uninstall-2338873-52.interdiff.txt1.23 KBplach
#50 entity-field_prevent_uninstall-2338873-50.patch14.98 KBplach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,404 pass(es). View
#50 entity-field_prevent_uninstall-2338873-50.interdiff.txt1.72 KBplach
#47 entity-field_prevent_uninstall-2338873-46.patch14.93 KBplach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,398 pass(es). View
#47 entity-field_prevent_uninstall-2338873-46.interdiff.txt7.68 KBplach
#44 entity-field_prevent_uninstall-2338873-44.patch22.43 KBplach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,406 pass(es). View
#44 entity-field_prevent_uninstall-2338873-44.interdiff.txt4.59 KBplach
#41 entity-field_prevent_uninstall-2338873-41.patch19.22 KBplach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,385 pass(es), 4 fail(s), and 2 exception(s). View
#41 entity-field_prevent_uninstall-2338873-41.interdiff.txt3.9 KBplach
#39 Uninstall___Drupal_8_x.png53.11 KBplach
#39 entity-field_prevent_uninstall-2338873-38.patch19.01 KBplach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,388 pass(es), 2 fail(s), and 2 exception(s). View
#39 entity-field_prevent_uninstall-2338873-38.interdiff.txt10.82 KBplach
#34 d8_uninstall_field_storage.patch10.93 KBfago
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,191 pass(es). View
#31 uninstall-field-storage.interdiff.txt1.03 KBfago
#31 uninstall-field-storage-only.patch.txt10.93 KBfago
#31 uninstall-field-storage-combined.patch31.15 KBfago
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch uninstall-field-storage-combined_0.patch. Unable to apply patch. See the log in the details link for more information. View
#29 uninstall-field-storage-combined.patch32.02 KBfago
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#29 uninstall-field-storage-only.interdiff.txt11.8 KBfago
#17 field-prevent-uninstall-if-has-data-2338873-17.patch9.36 KBplach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#15 field-prevent-uninstall-if-has-data-2338873-15.patch10.06 KBplach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#13 field-prevent-uninstall-if-has-data-2338873-13.patch12.1 KBplach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#13 field-prevent-uninstall-if-has-data-2338873-13.interdiff.txt8.03 KBplach
#8 interdiff.txt1.97 KBeffulgentsia
#8 field-prevent-uninstall-if-has-data-2338873-8.patch4.33 KBeffulgentsia
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,310 pass(es), 10 fail(s), and 0 exception(s). View
#6 field-prevent-uninstall-if-has-data-2338873-6.patch3.89 KBeffulgentsia
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,088 pass(es), 11 fail(s), and 18 exception(s). View
#1 field-block-uninstall.patch3.89 KBeffulgentsia
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

effulgentsia’s picture

FileSize
3.89 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
effulgentsia’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 1: field-block-uninstall.patch, failed testing.

effulgentsia’s picture

Come to think of it, I'm confused why we need this. Since this is for code-defined fields, shouldn't the module that adds the field to the entity type be the one to declare a dependency on the field type module? For example, node.info.yml has a dependency on text.module.

effulgentsia’s picture

+++ b/core/modules/system/system.module
@@ -1032,11 +1033,55 @@ function system_sort_themes($a, $b) {
+                  if ($storage_definition->getProvider() == $module_name) {
+                    $info['required'] = TRUE;
+                    $info['explanation'] = t('Fields type(s) in use');
+                    break;
+                  }

Oh, I see. The above is what threw me. So it's not necessarily the "field type" that's in use. The "provider" is the module that adds the field to the entity type, not the module that provides the field type.

Issue title and summary probably need fixing then.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
3.89 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,088 pass(es), 11 fail(s), and 18 exception(s). View

Same patch as #1, but without ending in *install.patch, since apparently, testbot doesn't like that.

Status: Needs review » Needs work

The last submitted patch, 6: field-prevent-uninstall-if-has-data-2338873-6.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
4.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,310 pass(es), 10 fail(s), and 0 exception(s). View
1.97 KB

Status: Needs review » Needs work

The last submitted patch, 8: field-prevent-uninstall-if-has-data-2338873-8.patch, failed testing.

plach’s picture

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

yched’s picture

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

catch’s picture

Issue tags: +D8 upgrade path
plach’s picture

Status: Needs work » Needs review
FileSize
8.03 KB
12.1 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

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.

Status: Needs review » Needs work

The last submitted patch, 13: field-prevent-uninstall-if-has-data-2338873-13.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
10.06 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

Status: Needs review » Needs work

The last submitted patch, 15: field-prevent-uninstall-if-has-data-2338873-15.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
9.36 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

Sorry, wrong patch

xjm’s picture

+++ 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.

Status: Needs review » Needs work

The last submitted patch, 17: field-prevent-uninstall-if-has-data-2338873-17.patch, failed testing.

effulgentsia’s picture

Title: Field type modules in-use by nonconfigurable fields can be uninstalled, resulting in data integrity bugs » Modules providing non-configurable field storage definitions can be uninstalled, leaving orphaned unpurged data
Issue summary: View changes
Related issues: +#2282119: Make the Entity Field API handle field purging

Fixed the title and summary based on me actually understanding the issue now :)

Removed reference to #2282119: Make the Entity Field API handle field purging from the summary, since I think the solution here might continue to make sense even after then, so adding that issue as just a related issue instead.

chx’s picture

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

Berdir’s picture

FieldableEntityinterface, actually, but yes.

xjm’s picture

plach’s picture

Title: Modules providing non-configurable field storage definitions can be uninstalled, leaving orphaned unpurged data » [pp-2] Modules providing non-configurable field storage definitions can be uninstalled, leaving orphaned unpurged data
Status: Needs work » Postponed

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.

xjm’s picture

Issue tags: +Triaged D8 critical
YesCT’s picture

Issue summary: View changes
fago’s picture

Assigned: Unassigned » fago
Status: Postponed » Needs work
fago’s picture

Status: Needs work » Needs review
Issue tags: +Ghent DA sprint
FileSize
11.8 KB
32.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
fago’s picture

Assigned: fago » Unassigned
fago’s picture

FileSize
31.15 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch uninstall-field-storage-combined_0.patch. Unable to apply patch. See the log in the details link for more information. View
10.93 KB
1.03 KB

Fixed an accidental change in KernelTestBaseTest.

The last submitted patch, 29: uninstall-field-storage-combined.patch, failed testing.

plach’s picture

Title: [pp-2] Modules providing non-configurable field storage definitions can be uninstalled, leaving orphaned unpurged data » Modules providing non-configurable field storage definitions can be uninstalled, leaving orphaned unpurged data

#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.

fago’s picture

FileSize
10.93 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,191 pass(es). View

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

The last submitted patch, 31: uninstall-field-storage-combined.patch, failed testing.

chx’s picture

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

tim.plunkett’s picture

fago’s picture

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.

plach’s picture

FileSize
10.82 KB
19.01 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,388 pass(es), 2 fail(s), and 2 exception(s). View
53.11 KB

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

Also implemented @chx's suggestion from #36:

fago’s picture

+++ 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.

plach’s picture

FileSize
3.9 KB
19.22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,385 pass(es), 4 fail(s), and 2 exception(s). View

Fixed missing pieces from #38 and #40.

The last submitted patch, 39: entity-field_prevent_uninstall-2338873-38.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 41: entity-field_prevent_uninstall-2338873-41.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
4.59 KB
22.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,406 pass(es). View

This should be green again.

tim.plunkett’s picture

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

plach’s picture

Reverted changes...

plach’s picture

FileSize
7.68 KB
14.93 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,398 pass(es). View

Meh

plach’s picture

This looks good enough to me, RTBC anyone?

yched’s picture

+++ 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 ?

plach’s picture

FileSize
1.72 KB
14.98 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,404 pass(es). View

It should, thanks :)

yched’s picture

+++ 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.

plach’s picture

FileSize
1.23 KB
15 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,399 pass(es). View

Oh, god, I'm sorry :(

pfrenssen’s picture

Status: Needs review » Needs work

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

swentel’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
14.91 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,473 pass(es). View
2.09 KB
plach’s picture

FileSize
706 bytes
14.69 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,472 pass(es). View

Fixed some more PHP docs.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record

Patch looks good. "Needs change record" tag was added in #18, but the patch no longer includes that change, so removing the tag. I think this is ready.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  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.

alexpott’s picture

Also faics the reason text is never actually tested.

plach’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
2.66 KB
14.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,537 pass(es). View

Fixed #57 and #58.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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!

  • alexpott committed 85764e4 on 8.0.x
    Issue #2338873 by plach, fago, effulgentsia, swentel: Modules providing...

Status: Fixed » Closed (fixed)

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