Problem/Motivation

This module has a service called entity_reference_revisions.orphan_purgerand its function is to search for entities (and revisions) that are no longer referenced by another entity, also known as orphan entities. To execute this there is a form in /admin/config/system/delete-orphans.

This works well with parent fields created with "Field Storage Config" but not with fields created with BaseFieldDefinition on custom entities. Consequently, the entities that are related to these types of fields will be eliminated when the service is executed, despite the fact that they are not actually orphaned.

This is what I have to produce the issue:

Host entity class has this in base field definition:

    $fields['dimensions'] = BaseFieldDefinition::create('entity_reference_revisions')
      ->setLabel(t('Dimensions'))
      ->setRevisionable(TRUE)
      ->setCardinality(FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED)
      ->setSetting('target_type', 'record_dimension')
      ->setDisplayConfigurable('form', TRUE)
      ->setDisplayConfigurable('view', TRUE);

Record dimension (child entity)

/**
 * Defines the RecordDimension entity.
 *
 * @ContentEntityType(
 *   id = "record_dimension",
 *   ...
 *   handlers = {
 *     ...
 *   },
 *   ...
 *   entity_revision_parent_type_field = "parent_type",
 *   entity_revision_parent_id_field = "parent_id",
 *   entity_revision_parent_field_name_field = "parent_field_name",
 *   ...
 * )
 */
class RecordDimension extends ChildRecordEntityBase {
...

The ChildRecordEntityBase class has the base field definition for "parent_type", "parent_id" and "parent_field_name". Needed to integrate with entity reference revisions features.

Proposed resolution

In the method EntityReferenceRevisionsOrphanPurger::isValidParent add verification of whether the parent field can be one of the BaseFieldDefinition family too.

Remaining tasks

  1. Write a patch with new tests
  2. Review
  3. Commit

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#4 interdiff_2_4.txt1.76 KBmnico
#4 purger_service_base_field_definition_compatibility-3158156-4.patch17.4 KBmnico
#2 purger_service_base_field_definition_compatibility-3158156-2.patch17.75 KBmnico

Comments

mnico created an issue. See original summary.

mnico’s picture

mnico commented
Status: Active » Needs review
StatusFileSize
new purger_service_base_field_definition_compatibility-3158156-2.patch17.75 KB

I attach a patch that does the modification in the isValidParent method and adds a test that extends EntityReferenceRevisionsOrphanRemovalTest but instead of using nodes it changes to a custom entity.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented 
+++ b/src/EntityReferenceRevisionsOrphanPurger.php
@@ -349,18 +360,26 @@ class EntityReferenceRevisionsOrphanPurger {
     }
-    // Check if the parent field is valid.
-    elseif (!($parent_field_config = $this->entityTypeManager->getStorage('field_storage_config')->load("$parent_type.$parent_field_name"))) {
-      $status = static::PARENT_INVALID_DELETE;
+    // Check if the parent field is a field storage config.
+    elseif ($parent_field_config = $this->entityTypeManager->getStorage('field_storage_config')->load("$parent_type.$parent_field_name")) {
+      // In case the parent field has no target revision ID key we can not be
+      // sure that this revision is not used anymore.
+      if (empty($parent_field_config->getSchema()['columns']['target_revision_id'])) {
+        $status = static::PARENT_INVALID_SKIP;
+      }

Nice catch, I'm not sure why this used configurable field storage.

But can't we just use \Drupal\Core\Entity\EntityFieldManager::getFieldStorageDefinitions() instead of checking for base and configureabnle fields in the same way?

mnico’s picture

mnico commented
StatusFileSize
new purger_service_base_field_definition_compatibility-3158156-4.patch17.4 KB
new interdiff_2_4.txt1.76 KB

Wow, you are right ;). I attach a new patch with that.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Fixed

Nice work! Fix looks good, verified that the test is the same as the default orphan removal test, just with the different entity types/field names and so on.

Changing constructors can be a bit tricky, especially adding new arguments in the middle, but this isn't something that's used during updates or so, should be easy to handle with a manual cache clear if people forget to run update.php.

+++ b/tests/src/Functional/EntityReferenceRevisionsOrphanRemovalForBaseFieldDefinitionTest.php
@@ -0,0 +1,243 @@
+
+  /**
+   * {@inheritDoc}.
+   */
+  public function insertRevisionableData() {
+    /** @var \Drupal\node\NodeStorageInterface $entity_host_storage */

Should be {@inheritdoc}, lowercase and without . at the end.

Fixed that and some other minor coding standard issues on commit.

  • Berdir committed 33c2b20 on 8.x-1.x authored by mnico 
    Issue #3158156 by mnico, Berdir: Purger service removes entities that...

Status: Fixed » Closed (fixed)

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