As documented in the handbook. It's doxygen'd, it's tested, it's necessary. Go!

Note: the plugin is mostly in an abstract base class because another dedupe based on id map was present in D7 and it's not unlikely we will want it again. Having DedupeBase in place makes it a rather trivial exercise adding that back (the scaffolding for the class and the test will be work; writing the actual logic, much as in DedupeEntity is just 1 LoC and wouldn't even need a getter method: function exists($value) { return $this->migration->getIdMap()->lookupDestinationId($value); }).

CommentFileSizeAuthor
#4 2147815_4.patch7.22 KBchx
#4 interdiff.txt2.45 KBchx
#2 interdiff.txt3.27 KBchx
#2 2147815_2.patch7.09 KBchx
entity_dedupe.patch7.93 KBchx

Comments

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/process/DedupeBase.php
@@ -0,0 +1,41 @@
+ * This abstract base contains the dedupe logic.

+++ b/core/modules/migrate/tests/Drupal/migrate/Tests/process/DedupeEntityTest.php
@@ -0,0 +1,127 @@
+/**
+ * @file
+ * Contains
+ */
...
+
+class DedupeEntityTest extends MigrateProcessTestCase {

As on the other issue we should document what is tested here. Kind of nice would be also @group Drupal and @group Migrate

chx’s picture

chx commented
StatusFileSize
new 2147815_2.patch7.09 KB
new interdiff.txt3.27 KB

I have realized I accidentally added DedupeSql. That is not going to happen , the other dedupe plugin will be DedupeId as described in the issue summary. So removed that, added doxygens and @group annotations.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

Here are some more points and suggestions.

  1. +++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/process/DedupeEntity.php
@@ -0,0 +1,39 @@
+ */
+
+
+namespace Drupal\migrate\Plugin\migrate\process;

    Extra white space. 

  2. +++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/process/DedupeEntity.php
@@ -0,0 +1,39 @@
+  /**
+   * @return \Drupal\Core\Entity\Query\QueryInterface
+   */
+  protected function getEntityQuery() {

    Function desc line missing. 

  3. +++ b/core/modules/migrate/tests/Drupal/migrate/Tests/process/DedupeEntityTest.php
@@ -0,0 +1,131 @@
+class DedupeEntityTest extends MigrateProcessTestCase {

    MigrateProcessTestCase has no use statement so I presume it has same namespace. 

  4. +++ b/core/modules/migrate/tests/Drupal/migrate/Tests/process/DedupeEntityTest.php
@@ -0,0 +1,131 @@
+   * Test the entity deduplication plugin when there is no duplication.
...
+   * Test the entity deduplication plugin when there is duplication.
...
+   * Test the entity deduplication plugin when there is no duplication.
...
+   * Test the entity deduplication plugin when there is duplication.

    Tests not Test 

  5. +++ b/core/modules/migrate/tests/Drupal/migrate/Tests/process/DedupeEntityTest.php
@@ -0,0 +1,131 @@
+   * Helper adding expectations to the mock entity object.

    "Helper function/method to add" will make more sense. 

  6. +++ b/core/modules/migrate/tests/Drupal/migrate/Tests/process/DedupeEntityTest.php
@@ -0,0 +1,131 @@
+   * @param $count

    Typhint missing. 

  7. +++ b/core/modules/migrate/tests/Drupal/migrate/Tests/process/DedupeEntityTest.php
@@ -0,0 +1,131 @@
+class TestDedupeEntity extends DedupeEntity {

    Class doc block missing. 

  8. +++ b/core/modules/migrate/tests/Drupal/migrate/Tests/process/DedupeEntityTest.php
@@ -0,0 +1,131 @@
+  function setEntityQuery(QueryInterface $entity_query) {

    Function doc block and scope missing.

chx’s picture

chx commented
StatusFileSize
new interdiff.txt2.45 KB
new 2147815_4.patch7.22 KB
dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Aweseome

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Nice job on the abstract class docs here. Was able to figure this one out pretty well, apart from entityQueryExpects() throwing me a bit for a loop.

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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