Taxonomy vocabulary with name "Type" cannot migrate from d6 to d8

Yep - because we create the taxonomy reference fields without a "field_" prefix, they can conflict with base fields like type, title, ... - I'm surprised there haven't been more reports of this. Here's a quick POC patch which prepends field_ to the reference field names, running through the testbot to see what fails there (hopefully only tests expecting the non-prepended names) then we'll set back to "Needs work".

Obviously we'll need to add tests. Also, in place of a simple truncation of the constructed field name we should run it through make_unique_entity_field to make sure we aren't creating conflicts against other fields (e.g., in the OP if there were a 'field_type' in D6). Simple enough to do in the d6_vocabulary_field.yml file, but how do the other pieces know what field name was ultimately generated?

Since this causes data loss in the straight upgrade scenario, upgrading the priority.

Encouraging that it looks like the only failures were tests using the field names without 'field_'...

I'm confused by these test results - which test is failing?

Yeah, it's strange, need to look at the full log to find:

15:19:58 Drupal\migrate_drupal_ui\Tests\d6\MigrateUpgrade6Test 622 passes 2 fails

I'll try running that test locally...

Fail      Other      MigrateUpgradeTes  159 Drupal\migrate_drupal_ui\Tests\Migr
    Found 71 field_config entities, expected 72.
    Value 72 is equal to value 71.
Fail      Other      MigrateUpgradeTes  159 Drupal\migrate_drupal_ui\Tests\Migr
    Found 46 field_storage_config entities, expected 47.
    Value 47 is equal to value 46.

So, my changes seem to be preventing one of the term reference fields from being created.

The test was failing because the field 'field_tags' is already provided by standard, so the migrated field 'tags' is renamed to 'field_tags' and the entity count for field_config and field_storage_config gets lowered by one. Let's see if this passes.

1. +++ b/core/modules/taxonomy/migration_templates/d6_vocabulary_entity_display.yml
   @@ -23,10 +24,25 @@ process:
   +  raw_field_name:
   

   Can we add a comment explaining that raw_field_name is only used further down in the processing pipeline, and otherwise essentially thrown out?

2. +++ b/core/modules/taxonomy/migration_templates/d6_vocabulary_entity_form_display.yml
   @@ -27,10 +28,25 @@ process:
   +    # Prepend field_ to avoid conflicts with base fields, and make sure the
   +    # result is no longer than 32 characters.
   +    -
   +      plugin: concat
   +      source:
   +        - constants/field_prefix
   +        - '@raw_field_name'
   +    -
   +      plugin: substr
   +      length: 32
   

   The fact that this is repeated suggests to me that this "transform to a field name" stuff should be its own process plugin, living in Migrate Drupal.

3. +++ b/core/modules/taxonomy/tests/src/Kernel/Migrate/d6/MigrateVocabularyFieldInstanceTest.php
   @@ -55,13 +55,13 @@ public function testVocabularyFieldInstance() {
   +    $this->assertIdentical(['field_tags'], $settings['handler_settings']['target_bundles'], 'The target_bundles handler setting is correct.');
   

   If you change any assertIdentical() calls, please change them to assertSame() while you're at it.

Re #12:
1) Added the comment.
2) This does not seem absolutely necessary, so we agreed to let this goes.
3) This too is not absolutely necessary, and it would create a lot of unrelated noise in this patch.

re #15.3, it looks like this was already done in the patch, there are issues already dealing with that, #2756519: Convert assertIdentical() to assertSame() for some uses in kernel tests that already have assertSame()'s parameter order just make sure new assertions use assertSame() (there are none, btw) and leave existing assertions alone, also the parameter order is different between assertSame and assertIdentical, so this change also causes more work/noise.

Asked @maxocub on IRC if I could tackle this. It looks RTBC to me otherwise.

FYI this is conflicting with #2754787: d6_term_node migration overwrites node changed timestamps

patching file core/modules/taxonomy/tests/src/Kernel/Migrate/d6/MigrateTermNodeTest.php
Hunk #1 FAILED at 38.

It shouldn't be but the context lines are close so it should be easy to re-roll either.

The reduction of the entity count in MigrateUpgradeTest is unexpected but well explained in #12.

Just noticed that a comment is a bit repetitive.

+++ b/core/modules/taxonomy/migration_templates/d6_vocabulary_entity_display.yml
@@ -23,10 +24,27 @@ process:
+  # This value is only used in the 'field_name' process pipeline below.
+  # Other than that, it is not used anywhere else.

The second sentence isn't really necessary. After all, the first sentence states it is *only* used in the pipeline. Note that this comment appears in other migrations as well.

Good catch.

My changes were trivial in #17. This looks to be the correct approach to deal with name conflicts.

The changes in the patch look good, but we don't have test explicit coverage for migrating a taxonomy vocabulary conflicting with the name of a base field, which was the original bug report, I think that's worth adding so CNW for that.

Here's new tests whit a D6 vocabulary named 'Type'. The test-only patch (which also serve as the interdiff) should generate the same error as in the IS.

Forgot to update the entity count...

Yes, this look good. I loaded up the test fixture and it is still good. Ran a full migration, just with the test fixture, and confirmed this fixes the problem in the IS.

However, I do agree with comments #14.2 that the repetitive code in the migration should be in a process plugin and #14.3 that the assertIdenticals should be changed to assertSame. How about a followup to look into the process plugin. And for the later, maybe I can get to it tomorrow. I don't want to hold up a critical, but there are plenty of RTBC issues waiting for attention.

Re #14.3: I changed all assertIdentical() to assertSame().

Re #14.2: Despite the fact that it was phenaproxima who suggested to make a process plugin to avoid code duplication, he also agreed (in person in a sprint in Montreal and in a Migrate meeting on hangout) that it was not that usefull in the end. As you can see in this latest patch, the process plugin is quite small. I already hear people asking if a process plugin that small is really usefull, since it's just doing concat & substr.
I did it anyway, for demonstration purpose, and if you think it is in fact better, then we'll need to add documentation & tests.

@maxocub, thanks for that. I think we've had a miscommunication about the process plugin. I just meant that it could be looked at in a follow up, not done here. And I do see that it is a tiny function. I also thought it was used elsewhere in core and now that I have looked I only see one case. And that one case I wrote, so maybe it was just on my mind as something useful.

I've removed the process plugin since it was error and even though I won't be able to RTBC this.

@quietone: No problem, I did it here because it wasn't that hard and to have a choice between these two options. I understand that my comment in #15 was not clear why this process plugin idea was not pursued.

That field_name_prefix process plugin wasn't necessary but cleaned up the duplication a bunch, I think a follow-up for that would be nice.

Needs a re-roll for 8.4.x/8.5.x

I'm fine with the duplication in YAML here for the moment, we can look at the specific plugin in a follow-up, but it's also good to have examples on where it's not necessary to write one.

I diffed the diffs, it's only context lines in /MigrateUpgrade6Test.php that failed and just a bit of fuzz for patch command, so re-RTBC.

CNW was due to not applying to 8.3.x, which is the point of the re-roll..

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!