Feed items are constantly updated due to missing entity reference

Good idea to add a configuration option. I think however, that resetting the hash when a reference is not found should remain the default. So it would be an opt-out instead of a opt-in for this behavior.

Thanks for the patch!

I would think that an update hook is not necessary and that setting defaults in defaultConfiguration() is enough.

Since the behaviour affects entity reference targets, I think it would be better if the configuration options are on the Entityreference target plugin instead of on the processor.

The test looks good. The code for the creation of the feed types could be shorter, there is a method called createFeedTypeForCsv() in FeedCreationTrait.

Thanks for the updated patch. I did not test this but have done a cursory review of the code for readability and noticed some minor nitpicks.

1. +++ b/src/Feeds/Target/EntityReference.php
   @@ -117,14 +117,16 @@ class EntityReference extends FieldTargetBase implements ConfigurableTargetInter
   +          // The referenced entity is not found. We need to enforce Feeds to try
   +          // to import the same item again on the next import.
   +          // Feeds stores a hash of every imported item in order to make the
   +          // import process more efficient by ignoring items it has already seen.
   +          // In this case we need to destroy the hash in order to be able to
   +          // import the reference on a next import.
   

   Now that this is indented it exceeds 80 characters on one line. While that's being fixed, I'd update the text so it has better wrapping and is reworded slightly. For example, adding a comma after "In this case". Here's a stab at simplifying the wording:

   Feeds stores a hash for every imported item, so that items with hashes can be ignored when re-importing. Since the referenced entity is not found, force Feeds to re-import this item on the next import by resetting the hash.

2. +++ b/src/Feeds/Target/EntityReference.php
   @@ -432,6 +435,13 @@ class EntityReference extends FieldTargetBase implements ConfigurableTargetInter
   +      '#description' => $this->t('When trying to import values for entity reference fields it is possible that the referenced entity does not exist yet and that the entity in question either appears later in the feed or that it gets imported via a different feed. Resetting the hash will cause the item to be updated on the next import. This is the default behaviour. Checking this option will override that behaviour.'),
   

   A couple things:

   1. The first sentence is very long.

   2. British English is being used.

   Here's a stab at rewording:

   When importing an entity reference, sometimes the referenced entity does not exist yet because it will be imported later. By default, the entity reference's hash is reset in these situations, so the entity reference can be re-imported on the next import. To override this behavior and not allow re-importing, enable this option.

3. +++ b/tests/src/Kernel/Feeds/Target/EntityReferenceTest.php
   @@ -175,6 +175,108 @@ class EntityReferenceTest extends FeedsKernelTestBase {
   +   * The default behaviour is to re-hash items if they have a missing entity reference.
   +   * @see testUpdatingMissingReferences. When empty_reference_no_hash_reset is checked, the
   +   * item is not re-hashed if it has a missing entity reference. This may be useful if
   +   * your feeds are always imported in a specific order and there is a possibility that
   +   * some referenced items might be missing. Without checking this option, the item with
   +   * a missing entity reference will always be updated, even if nothing changed.
   

   A few things:

   1. Lines exceed 80 characters.

   2. British English is being used.

   3. Feels like this is just rehashing (pun intended!) the form description information. Not sure this is necessary.

   Here's a stab at rewording:

   Tests the `empty_reference_no_hash_reset` option.

   When this option is disabled, which is the default, this is tested by `testUpdatingMissingReferences`. If this option is enabled, test empty entity references are not re-imported when importing again.

4. +++ b/tests/src/Kernel/Feeds/Target/EntityReferenceTest.php
   @@ -175,6 +175,108 @@ class EntityReferenceTest extends FeedsKernelTestBase {
   +    $this->createFeedTypeForCsv([
   +      'guid' => 'guid',
   +      'title' => 'title',
   +    ], [
   +      'id' => 'article_feed_type',
   

   It appears the formatting is off here.

5. +++ b/tests/src/Kernel/Feeds/Target/EntityReferenceTest.php
   @@ -175,6 +175,108 @@ class EntityReferenceTest extends FeedsKernelTestBase {
   +    $this->createFeedTypeForCsv([
   +      'guid' => 'guid',
   +      'title' => 'title',
   +      'article' => 'article',
   +    ],
   +    [
   +      'id' => 'page_feed_type',
   

   It appears the formatting is off here.

6. +++ b/tests/src/Kernel/Feeds/Target/EntityReferenceTest.php
   @@ -175,6 +175,108 @@ class EntityReferenceTest extends FeedsKernelTestBase {
   +    // Check node 2 too.
   

   Extreme nitpick: Odd to read... perhaps reword to just Check other node.

7. +++ b/tests/src/Kernel/Feeds/Target/EntityReferenceTest.php
   @@ -175,6 +175,108 @@ class EntityReferenceTest extends FeedsKernelTestBase {
   +    // And re-import first feed again.
   

   Extreme nitpick: Odd to read perhaps reword to just Re-import feed.

Note that the updated patch will need manual testing so tagging now so we don't forget.

Thanks for the updates! Comparing against feedback in #13:

1 - Partially addressed

IMO the following could be removed as the text I suggested above was meant to cover that part as well.

+          // The referenced entity is not found. We need to enforce Feeds to try
+          // to import the same item again on the next import.

2 - Addressed

3 - Addressed

4 - Not addressed

Sorry, I should have been more clear. It seems like the indenting is off for this variable.

+    $this->createFeedTypeForCsv([
+      'guid' => 'guid',
+      'title' => 'title',
+    ],
+    [
+      'id' => 'article_feed_type',

5 - Not addressed

Same as above, i.e. it's related to the indentation.

6 - Addressed

7 - Addressed

Tagging for testing steps.

Thanks for the update @achap.

Sounds good regarding the formatting.

Would you be able to add steps for testing this manually?

Wow! Amazing work @achap. I'll see if I can find someone to test in the next week.

Cleaning up tags.

I propose re-opening this issue. We have the following scenario where the hash reset is causing us problems:

* Feed contains a list of terms
* Destination site only uses a handful of the terms in the feed
* On import when a term doesn't exist, the hash is invalidated
* Every subsequent import causes the destination node to be updated even though nothing has changed.

I wrote basically the same patch to solve the problem and @megaChris pointed me here. I realize that #23 says this isn't a good fix for their problem, but in our instance it's exactly what we need. If there were an API around hash validation that could work too, but seems like a much larger effort.

My only feedback for getting #19 added is that it's very verbose and can be trimmed up (e.g $this->configuration['ignore_missing']). I do agree that FALSE should be the default for backwards compatibility.

Re-created patch against latest 3.x-dev

Would it be better to reverse the option? Instead of it saying "Do not reset hash", let it say "Reset hash". I'm not sure, but I think it's better UX that enabled options imply to "do" something instead of "do not" something. Opinions?

Also, since there is a patch, the status "Needs review" would apply here. Though it would be better to create a MR from it, because the testbot on drupal.org no longer evaluates patches.

It would be nice indeed to check how Migrate approaches this problem.

Updated the remaining tasks.

Needs a reroll (and MR) now that #2973170: Importing new files fails when "Autocreate entity" option is turned on. is in.

Also, I suggested a change, see remaining tasks from the issue summary.

I am a first time contributor at DrupalCon Vienna and I will take a look at this issue.

I never got my local working enough to access the fork locally. I did prepare the re-roll against the 8.3-dev branch but not sure how much use that is. I will attach anyway in case. I never got to the final changes. I also did not get to test

I have added the re-rolled patch to the issue branch and created a draft MR.

Next step is to apply the changes and test