Problem/Motivation

The module is not aware of the difference between default and other revisions (typically 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.

The same problem happens when reverting to an older revision which still contains the reference.

Proposed resolution

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

Remaining tasks

CommentFileSizeAuthor
#6 entity-reference-purger-3348323-6.patch10.06 KBjohnle
#5 entity-reference-purger-3348323-5.patch7.2 KBrajiv.singh
#4 entity-reference-purger-3348323-4.patch4.79 KBrajiv.singh

Issue fork entity_reference_purger-3348323

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

dalin created an issue. See original summary.

dalin’s picture

dalin
he/him
Primary language English
Location 🇨🇦, 🇲🇽, 🌍
commented
Issue summary: View changes

vengador made their first commit to this issue’s fork.

rajiv.singh’s picture

rajiv.singh commented
StatusFileSize
new entity-reference-purger-3348323-4.patch4.79 KB

Fixing translation + workflow issue

rajiv.singh’s picture

rajiv.singh commented
StatusFileSize
new entity-reference-purger-3348323-5.patch7.2 KB

Fixed code redundancy in #4 patch

johnle’s picture

johnle commented
StatusFileSize
new entity-reference-purger-3348323-6.patch10.06 KB

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.

dalin’s picture

dalin
he/him
Primary language English
Location 🇨🇦, 🇲🇽, 🌍
commented
Status: Active » Needs work 
  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.

johnle’s picture

johnle commented

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

dalin’s picture

dalin
he/him
Primary language English
Location 🇨🇦, 🇲🇽, 🌍
commented

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

johnle’s picture

johnle commented

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

johnle opened merge request !1

johnle’s picture

johnle commented

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.

scott_euser’s picture

scott_euser commented
Issue summary: View changes

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.