Entity Reference Purger is not workflow aware

Created on 16 March 2023, almost 2 years ago
Updated 19 April 2023, over 1 year ago

Problem/Motivation

The module is not aware of the difference between default and most recent revisions. Consider this scenario:

Steps to reproduce

1. Node 1 exists and does not reference any taxonomy terms.
2. Editor creates a forward draft of the node and references Taxonomy Term A.
3. Taxonomy term A is deleted. Entity Reference Purger never finds the dangling reference because it's not on the default revision of the node.
4. The forward draft is published. Now the dangling reference causes all the problems that this module was designed to fix.

Proposed resolution

Find occurrences in both the default revision, and the most recent revisions that are not also the default. Make adjustments to both without blowing away the other.

Remaining tasks

🐛 Bug report
Status

Active

Version

1.0

Component

Code

Created by

🇨🇦Canada dalin Guelph, 🇨🇦, 🌍

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @dalin
  • 🇨🇦Canada dalin Guelph, 🇨🇦, 🌍
  • First commit to issue fork.
  • 🇮🇳India rajiv.singh

    Fixing translation + workflow issue

  • 🇮🇳India rajiv.singh

    Fixed code redundancy in #4 patch

  • 🇺🇸United States johnle

    Great job on the patch! Here are some bugs that I've updated to resolve from the previous patch
    * Sometimes the revisions are not all deleted when a term is removed. I've just re-order the revision so that the main revision is deleted last that way it would not stop the revision from being deleted.
    * SetSyncing still update the modified date to the most recent dates of the active revision on all languages, causing an issue with the content nove allowing to save with a message that it has been modified by another user. To fix this, used the code from the https://www.drupal.org/project/preserve_changed to keep the modified dates untouched if we are not creating a revision.
    * Minor coding standard fixes and comments.

  • Status changed to Needs work over 1 year ago
  • 🇨🇦Canada dalin Guelph, 🇨🇦, 🌍
    1. +++ b/entity_reference_purger.module
      @@ -9,8 +9,16 @@ use Drupal\Core\Entity\EntityInterface;
      +function preserve_changed_field_info_alter(array &$info): void {
      

      This should be `entity_reference_purger_field_info_alter()`

    2. +++ b/entity_reference_purger.module
      @@ -9,8 +9,16 @@ use Drupal\Core\Entity\EntityInterface;
      +  $info['changed']['class'] = PreservedChangedItem::class;
      

      What happens if both entity_reference_purger and preserve_changes modules are installed? Should ERP just require PC as a dependency?

    3. +++ b/entity_reference_purger.module
      @@ -105,8 +113,7 @@ function entity_reference_purger_entity_delete(EntityInterface $entity) {
      +                  \Drupal::service('entity_reference_purger.process_entity_reference_items')->processentities($parent_entity, $field_name, $delta, $field_item);
      

      Should be `processEntities()`

    4. +++ b/src/Plugin/QueueWorker/EntityReferencePurgerWorker.php
      @@ -64,12 +74,12 @@ class EntityReferencePurgerWorker extends QueueWorkerBase implements ContainerFa
      +        $this->entityReferencePurgerService->processentities($parent_entity, $field_name, $delta, $field_item);
      

      Here too

    5. +++ b/src/ProcessEntityReferenceItems.php
      @@ -0,0 +1,106 @@
      +      // Move the current revision last on the array so that it does not break
      +      // the not stop revision from being able to stop saving due to no longer
      +      // being the main revision active.
      

      This comment doesn't quite make sense. Maybe:

      The current revision can't be deleted until the rest have, so move it to the end of the array.

    6. +++ b/src/ProcessEntityReferenceItems.php
      @@ -0,0 +1,106 @@
      +              $terms = $translated_entity->get($field_name)->getValue();
      +              foreach ($terms as $key => $tid) {
      +                if ($tid['target_id'] === $field_item->target_id) {
      +                  $translated_entity->get($field_name)->removeItem($key);
      +                  $translated_entity->setSyncing(TRUE);
      +                  $translated_entity->changed->preserve = TRUE;
      +                  $translated_entity->save();
      +                }
      +              }
      

      This duplicated code should be moved into a protected method.

  • 🇺🇸United States johnle

    @dalin
    Thanks for the code review, I'll get all recommended change in place, one thing I am kinda struggling on is when adding a dependencies to preserve_change module, I've ran into the following issue:
    * When applying a composer patch, it did not install the require preserve_change change module, likely due to it running the composer install on the module first, and then later on patching it with the composer changes.

    +++ b/composer.json
    @@ -21,6 +21,7 @@
             "source": "https://git.drupalcode.org/project/entity_reference_purger"
         },
         "require": {
    -        "drupal/core": "^8.8.0 || ^9.0 || ^10"
    +        "drupal/core": "^8.8.0 || ^9.0 || ^10",
    +        "drupal/preserve_changed": "8.8.0 || ^9.0 || ^10"
         }
     }
    
    

    * Adding the dependencies in the .info.yml file, seem to give conflict. I've tried the suggestion of adding an additional line break, but still no luck.

    +++ b/entity_reference_purger.info.yml
    @@ -2,3 +2,8 @@ name: 'Entity Reference Purger'
     description: 'Removes orphaned entity references when an entity is deleted.'
     type: module
     core_version_requirement: ^8.8.0 || ^9 || ^10
    
    +dependencies:
    +  - preserve_changed:preserve_changed
    
    

    I think for the time being it might be easier to add the changedItem alter to the module, but use maybe a different key instead so it doesn't conflict with the preserve_change module properties key, what do you think?

  • 🇨🇦Canada dalin Guelph, 🇨🇦, 🌍

    @johnle

    > When applying a composer patch, it did not install the require preserve_change change module, likely due to it running the composer install on the module first, and then later on patching it with the composer changes.

    Yes, that's a limitation with Composer Patches that can't be avoided.
    https://docs.cweagans.net/composer-patches/troubleshooting/non-patchable...
    But I wonder if you could get around that by using an issue fork (see the info at the top of this page) rather than a .patch file.

  • 🇺🇸United States johnle

    Thanks, I will try pushing the changes to once https://git.drupalcode.org/ is stable again.

  • Open in Jenkins → Open on Drupal.org →
    Core: 10.1.x + Environment: PHP 8.1 & MySQL 8
    last update over 1 year ago
    Fetch Error
  • 🇺🇸United States johnle

    Ok I was able to get this working that resolve the 2 issues above
    Added this above the drupal.org patckagelist

    {
                "type": "composer",
                "url": "https://packages.drupal.org/8"
    },
    

    with:

    {
                "type": "package",
                "package": {
                    "name": "drupal/entity_reference_purger",
                    "type": "drupal-module",
                    "version": "dev-3348323-entity-reference-purger",
                    "source": {
                        "url": "https://git.drupalcode.org/issue/entity_reference_purger-3348323.git",
                        "type": "git",
                        "reference": "3348323-entity-reference-purger"
                    },
                    "require": {
                        "drupal/preserve_changed": "^2.0@alpha"
                    }
                }
            },
    .......
    

    And ran:

    composer require drupal/entity_reference_purger:dev-3348323-entity-reference-purger
    

    It seem to installed the require dependencies. I'll need to retest this again with the replacement of the dependencies and the queuing bug that was reported from the patch from my team.

  • Open in Jenkins → Open on Drupal.org →
    Core: 10.1.x + Environment: PHP 8.1 & MySQL 8
    last update over 1 year ago
    Composer require failure
  • Open in Jenkins → Open on Drupal.org →
    Core: 10.1.x + Environment: PHP 8.1 & MySQL 8
    last update over 1 year ago
    Composer require failure
  • 🇬🇧United Kingdom scott_euser

    Updating issue summary to match the code which gets all revisions rather than just 'forward' revisions. Since the MR already covers the scenario of reverting to an older revision as well.

Production build 0.71.5 2024