Migrate: Don't deduplicate forum vocabulary

This looks like a duplicate of #2682705: Migrate process plugin does not save stubs to the idmap, leads to duplicates and broken references, which has a patch.

That patch included some test coverage, but as far as I know we don't have test coverage for the following:

- take a 6.x site with forum/taxonomy references
- migrate to 8.x
- confirm the references are still correct

Since that's the bug that was reported here, re-opening for that extra test coverage.

@infopicard, just to confirm, you tested it with the 8.1.x branch (either via git clone or dev tarball)?

Add tag.

We already have a round-trip test for term references: \Drupal\Tests\taxonomy\Kernel\Migrate\d6\MigrateTermNodeTest .

The problem here is actually that when we migrate vocabularies, we deduplicate them. In case the D8 site already has a vocabulary 'foo', a D6 vocabulary 'foo' will be renamed 'foo1'. But this does the wrong thing for forums, where we definitely want to migrate into the existing 'forums' vocabulary.

I guess we're deduping in this case just because D6 vocabs don't have machine_names. But what we really want isn't "make sure this vocab has a globally unique name". It's "make sure this vocab has a unique name amongst migrated vocabs". Maybe we can have a new kind of dedupe, that checks if the "duplicate" is in the IdMap?

Ok, here's a fix for the taxonomy duplication. But there's more!

• The term-reference field imported by d6_vocabulary_field has the wrong name: 'forums' instead of 'taxonomy_forums'. So the field is duplicated.
• As a consequence of the above, the settings for that field are all applied to the duplicated field.
• The setting for whether each forum term is a "container" is not migrated. That's stored in the 'forum_container' field in D8, and the 'forum_containers' variable in D6.

We'll definitely need to test these, and find a solution.

In particular, is there any good existing way to munge field names in our migrations?

I'm unassigning this because my system is too far ahead to setup a working D6 forum.

Edit: please disregard my misguided rant, I missed that these issues are not trying to do what I was railing about...

Thinking about this some more, especially in light of #2716133: Roles are duplicated instead of updated existing roles (another issue where we want to avoid deduping in at least some instances)...

In the general case, code alone can not determine whether, when it sees a 'foobar' on the source side and a 'foobar' on the destination side, they represent the same thing to the site-builder (and should be mapped as-is) or whether they should represent different things (and thus the source-side foobar should become a new thing on the destination side). Ideally, the tools would allow the migrator to decide for each source item whether to map it to an existing destination item, or create a new one - and indeed, migrate_d2d in D7 provided this option for both roles and vocabularies. And it is my intent to provide that functionality in the D8 version of migrate_d2d, regardless of the default core behavior.

So, the question is, what should the default core behavior be - specifically the uncustomized "upgrade" scenario. This scenario is assuming an "empty" Drupal 8 site - one which has been installed from some install profile, and has any desired modules enabled, without any substantial configuration or content creation beyond that. Our problem here is that site isn't quite empty - the standard profile creates an administrator role and tags vocabulary, the forum module adds a forums vocabulary, etc. At least in core, the things that create conflict are carryovers from older versions of Drupal and most definitely *should* be mapped directly without deduping. And, more broadly, if you've created a new Drupal 8 site to hold your old Drupal 6/7 content and configuration, if something is named the same on both sides it's almost certainly because they have the same meaning and role for you.

Or, another angle on this - the core upgrade process is intended to replicate the old site as precisely as possible in D8, and thus should not rename things if at all avoidable.

Thus, my opinion is that the default vocabulary and role migrations in core should *not* dedupe - they should overwrite the "conflicting" entities.

Thoughts?

*sigh* - guess I needed to look more closely at the actual vocabulary migrations as they are at the moment, deduping is only being done for D6 to make sure machine names generated from similar human-facing names don't conflict.

<emily_litella>Never mind.</emily_litella>

Patch no longer applies, so reroll.

From #10,

• The setting for whether each forum term is a "container" is not migrated. That's stored in the 'forum_container' field in D8, and the 'forum_containers' variable in D6

This variable is not used in the either the D6 or the D7 test fixture. It will have to be added.

Changed the input array to lookupSourceID to be an array keyed by field name. It just might pass tests now. But, it will still need work as per #10 and #18 regarding forum_containers.

Works better with the patch.

Silly to forget to fix DedupeEntityTest.

This will dedupe if the entity is not in the id map. But it doesn't check the status of that migration. Should it?

+++ b/core/modules/migrate/tests/src/Unit/process/DedupeEntityTest.php
@@ -161,4 +161,48 @@ protected function entityQueryExpects($count) {
+    foreach (['forums', 'test_vocab', 'test_vocab1'] as $id) {
+      $query = $this->getMockBuilder('Drupal\Core\Entity\Query\QueryInterface')
+        ->disableOriginalConstructor()
+        ->getMock();
+      $query->expects($this->once())
+        ->method('execute')
+        ->will($this->returnValue($id === 'test_vocab1' ? [] : [$id]));
+      $map[] = ['test_field', $id, NULL, NULL, $query];
+    }

This makes sense if I squint at it, but it's quite difficult to parse. Can we use Prophecy here instead, purely for readability reasons?

That's my only complaint. Beyond that, I think this patch makes sense.

Marking NW for #25. And novice because should be a fairly easy introduction to PHPUnit, if someone wants to pick this up.

Lets see if we can do something about #25 today.

For reference I've found this page: https://www.drupal.org/docs/8/phpunit/using-prophecy

Changed to Prophecy as per #25.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/DedupeEntity.php
   @@ -51,12 +62,25 @@ public static function create(ContainerInterface $container, array $configuratio
   +      $idMap = $this->migration->getIdMap();
   +      foreach ($query->execute() as $id) {
   +        $dest_id_values[$this->configuration['field']] = $id;
   +        if ($idMap->lookupSourceID($dest_id_values)) {
   +          return TRUE;
   +        }
   +      }
   

   I doubt this is going to scale well on very large migrations, but until ID maps support looking up a large set of source IDs at once, we might be stuck with it. Would like to get @mikeryan or @neclimdul's opinion about this.

2. +++ b/core/modules/migrate/tests/src/Unit/process/DedupeEntityTest.php
   @@ -161,4 +162,45 @@ protected function entityQueryExpects($count) {
   +      $query = $this->prophesize(QueryInterface::class);
   +      $query->willBeConstructedWith([]);
   +      $query->execute()->willReturn($id === 'test_vocab1' ? [] : [$id]);
   +      $map[] = ['test_field', $id, NULL, NULL, $query->reveal()];
   

   I don't understand why the query mock is being included in the value map. This needs a comment at least.

3. +++ b/core/modules/migrate/tests/src/Unit/process/DedupeEntityTest.php
   @@ -161,4 +162,45 @@ protected function entityQueryExpects($count) {
   +    $this->entityQuery->expects($this->exactly(3))
   

   I'd rather we didn't assert on exactly how many times the method was called. If we must, can we assert that it's called at least 3 times, rather than exactly 3 times? That's coupling the test a little too closely to the internal implementation than I'm comfortable with.

4. +++ b/core/modules/migrate/tests/src/Unit/process/DedupeEntityTest.php
   @@ -161,4 +162,45 @@ protected function entityQueryExpects($count) {
   +    $this->idMap->expects($this->exactly(2))
   

   Same here.

2. Added some comments. Better?
3-4. Removed the number of times the method will be called.

Hmm, I was sure uploaded the patch.

I doubt this is going to scale well on very large migrations, but until ID maps support looking up a large set of source IDs at once, we might be stuck with it. Would like to get @mikeryan or @neclimdul's opinion about this.

If I understand correctly, it will iterate only over matches to the value we're deduping, and since we're deduping it there shouldn't normally be more than one in practice (barring a non-migration process creating many dupes) - right? I think the possible scalability issue is edge-casey enough that we don't need to spend a lot of cycles optimizing for it.

@mikeryan, I agree with your summary.

Manually tested with a D6 site - the forum taxonomy is properly deduped, so good as far as that goes. But, forum content is still not hooked to the containing forum, and the first view of a forum node after a cache rebuild gets array_flip errors. Looks like vasi's issues in https://www.drupal.org/node/2690001#comment-11290787 weren't addressed:

The term-reference field imported by d6_vocabulary_field has the wrong name: 'forums' instead of 'taxonomy_forums'. So the field is duplicated.

As a consequence of the above, the settings for that field are all applied to the duplicated field.

That being said, this patch does address the vocabulary angle well, and we have an existing issue for the field duplication/array_flip() at #2637286: Migrated forum topics generate array_flip warning in EntityStorageBase, so I say we go ahead and commit this, and pursue the field angle in that issue.

Let's have a change record to document the new 'migrated' option on the migration process plugin.

Still new to change records, but at least it is a start.
dedupe_entity process plugin now takes optional 'migrated' option

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!

Published change record.