Handle long comment bundle names

Checked if patch needed a re-roll. Offset spotted. Patch file updated.

Hunk #1 succeeded at 26 (offset 2 lines).
patching file core/modules/field/migration_templates/d7_field_instance.yml
Hunk #1 succeeded at 11 (offset 2 lines).

Now that dedupe has been renamed, this needs a re-roll.

Updated references to plugins "dedupe_entity" ("make_unique_entity_field") and "migration" ("migration_lookup").

Patch in #8 no longer applies. Re-rolled.

Correct invalid YAML.

+++ b/core/modules/comment/migration_templates/d6_comment_type.yml
@@ -8,7 +8,15 @@ source:
+        length: 32

+++ b/core/modules/comment/migration_templates/d7_comment_type.yml
@@ -8,7 +8,15 @@ source:
+        length: 32

If we combine the name of 32 characters with 1 or 2 additional numbers to make it unique, then that results in a unique name of 33 or 34 characters. I think we want to drop the length to 30 characters.

Reduced the number of characters from 32 to 30, as suggested.
Fixed bugs in a couple of tests.

Many tests will still fail because the new markup to obtain the bundle:

  bundle:
    -
      plugin: migration_lookup
      migration:
        - d7_node_type
        - d7_comment_type
      source: bundle

...fails if the bundle is "user". I have been unable to come up with a solution. Any suggestions?

Adding Drupal 6 tag because this happens on 8.5.x when migrating from 6.x as well.

We hit this hard 32 character limit on multiple CCK content types when migrating from D6.

When we changed the class constant from 32 to 64 all errors disappeared. Though we likely just silenced some errors.

What I don't understand is the motivation for keeping the limit at around 30 to begin with. Why not allow even 100 characters? Verbosity can be a good thing, right?

The original report is for D7->D8, so adding that tag.

See to CR Entity type and bundle machine names have a maximum length of 32 characters

I tried to do a reroll from fc4a1d6be073a2542d21a2b8479c76370a6adcf4 but failed.

I altered path according to #2917883: Rename 'migration_templates' directories in core modules to 'migrations' so files are found at least. Hope that is useful?

I cannot remember we got (D6) ID warning from #2876085: Before upgrading, audit for potential ID conflicts

Regarding core/modules/comment/migrations/d6_comment_type.yml I'm not sure how to apply this to the new settings

Current core

process:
  target_entity_type_id: 'constants/entity_type'
  id:
    -
      plugin: concat
      source:
        - 'constants/id_prefix'
        - type
    -
      plugin: static_map
      bypass: true
      # The Forum module provides its own comment type (comment_forum), which we
      # want to reuse if it exists.
      map:
        comment_node_forum: comment_forum
  label:

and this patch

  process:
    target_entity_type_id: 'constants/entity_type'
+   id:
+       -
+         plugin: get
+         source: comment_type
+       -
+         plugin: make_unique_entity_field
+         entity_type: comment_type
+         field: id
+         length: 30
    label: label

Here's a patch I've rebuilt from scratch, based on #22, against the latest 8.6.x core.
It applies cleanly but doesn't solve the issue (I'm yet to write any migrations of my own so my knowledge is limited).

Maybe someone can take a look at it?

Add related issue

Hi,
I also bumped in to this error:

Attempt to create a bundle with an ID longer than 32 characters: comment_node_gallery_image_landen(). (/web/core/lib/Drupal/Core/Entity/EntityBase.php:439 

Is there any news that this can be fixed, or do I need to alter my nodetypes to smaller amount of characters?

greetings, Martijn

I worked around the problem by not migrating the config. Manually created the D8/9 node bundles and only migrated the content.

I should have made a reroll from #23 but I just forgot. IIRC the changes were to the setup methods of the test files because there were cleanup up in the meantime. If someone wants to see it I will make one.

So, just making a start here. This patch , aside from the reroll, adds a node type with a 32 character name to the drupal 7 test fixture. Then the MigrateNodeTypeTest and MigrateCommentTypeTests were updated and that is all.

The other migrations in this patch need to be checked and there is still a D6 version to make. And fail patches.

Should we really (care about) migrate D6 to D9?

Removed the changes to d7_field_formatter_settings.yml. I am not sure why that is changing the bundle process to allow for a comment bundle. The field is definitely on a node not a comment. And fixed an error in the fixture.

This introduces a new process plugin to determine the bundle in d7_field_instance and changes the migration yml accordingly. And also changes d7_field_formatter to do a migration lookup on d7_field_instance to get the bundle whereas before it repeated the process.

There is still work to do here but I'm finished with this for now.

Didn't appreciate what the static_map was doing with the bundle. So, restore that and alter the new process plugin to use the output of the static_map unless it is a comment bundle, then do a migration lookup to get the correct name.

Fixing some more tests and fixing coding standard issues.

Still working on the tests.

A few more test fixes.

Add assertions to D7 field tests and add changes to the D6 fixture and d6 tests.

Unrelated fail, retesting.

The patch in #48 is for another issue and I didn't even notice until I came back to self-review.

So, here is the correct patch, which add assertions to D7 field tests and adds a D6 migration.

Change the name of the new node type in the drupal6 test fixture. Need to install comment into d6/MograteFieldInstanceTest in order to test the long comment name.

Still need to check the other d6 field migrations and test.

Correct the name of the new node type in the drupal6 fixture, add more d6 tests.

Added interdiff txt file for #57 to fixing test cases.

I am working on it.

Hi @quietone,

Fixed test case issue, Kindly review.

@shaktik, thank you.

Did a bit of cleanup and add comments. The biggest change was to rewrite the new process plugin, field_bundle.

Found two instances of an extra full stop.

I'm also getting this issue. I tried applying the patch from #65, but I'm getting an error:

- Applying patches for drupal/core
https://www.drupal.org/files/issues/2020-05-20/2565931-65.patch (Handle long comment bundle names)
Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2020-05-20/2565931-65.patch

Any idea why?

Thanks!

@Sseto, it needs a reroll.

Patch rerolled.

That was an error in the reroll of the D7 fixture.

Back to fixing tests and entity counts.

Hi quietone,

I tried applying patch #69, #70, and #71 and I'm still getting the error.

- Applying patches for drupal/core
https://www.drupal.org/files/issues/2020-06-02/2565931-71.patch (Handle long comment bundle names)
Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2020-06-02/2565931-71.patch

Hi @Sseto,

Could you try with fresh drupal Installation with 9.1.x version? if still getting the error please share the screenshot where you getting, Skipping or an error was: Cannot apply the patch.

Thanks,
Shakti

Ah no wonder...I'm using D8...

This might not be the place to comment. But I am curious if there is a similar patch to Drupal 8. All of my search results keep on bringing me back to this one.

@azussman, no there isn't one for Drupal 8. If you need this for a local project you can just take the functional changes and skip the changes to the tests and fixtures. The changes to the tests and the fixtures are the most difficult part to reroll. Of course, you should do some manual testing to make sure it is working as expected for your Drupal 8 site(s).

1. +++ b/core/modules/field/src/Plugin/migrate/process/d7/FieldBundle.php
   @@ -0,0 +1,78 @@
   +/**
   + * Determines the bundle for a field.
   + *
   + * @MigrateProcessPlugin(
   + *   id = "field_bundle"
   + * )
   + */
   

   I think this needs more documentation as to how to use this plugin. I see the opening line of the transform function is [$entity_type, $bundle] = $value so I assume we need to pass in an array, but there is no documentation at the top of the process plugin.

2. +++ b/core/modules/migrate_drupal/tests/fixtures/drupal6.php
   @@ -45854,6 +45854,22 @@
   +  'type' => 'a_thirty_two_character_type_name',
   

   I absolutely love that "a_thirty_two_character_type_name" is a 32 character type name.. Was part of this issue to ensure that if there was a second type name that started with "a_thirty_two_char" would not clash with the first one with a bundle of "comment_node_a_thirty_two_char", or something like that? Are we testing for that?

Before addressing #77, this needs a reroll.

77.1. Expanded the doc block with a description and examples.
77.2 Yes, I like that name as well. I don't think there is a need to confirm that name clashes are avoided because the migration is using make_unique_entity_field which is well tested.

Thanks for the extra documentation and the explanation as to why we don't need to test for clashing comment names!

+++ b/core/modules/field/src/Plugin/migrate/process/d7/FieldBundle.php
@@ -0,0 +1,116 @@
+   */
+  public function transform($value, MigrateExecutableInterface $migrate_executable, Row $row, $destination_property) {
+    [$entity_type, $bundle] = $value;
+    if ($bundle === 'comment_node_forum') {
+      return 'comment_forum';
+    }
+    $lookup_result = NULL;
+    // For comment entity types get the destination bundle from the
+    // d7_comment_type migration.
+    if ($entity_type === 'comment') {
+      $value = preg_replace('/comment_node_/', NULL, $bundle);
+      $migration = 'd7_comment_type';
+      $lookup_result = $this->migrateLookup->lookup([$migration], [$value]);
+      $lookup_result = empty($lookup_result) ? NULL : reset($lookup_result[0]);
+    }
+    return $lookup_result ? $lookup_result : $bundle;
+  }
+

Could we not still use the static map for comment_node_forum/comment_forum in the migration itself somehow to override the behaviour, instead of doing it here?

We could, but I don't see the point. Doing so means the conversion of the bundle name is a two step process, a static map and process plugin, instead of just in one place, the process plugin. That alone adds some overhead. Then in field_bundle we either test for 'comment_forum' or we always run an extra migration_lookup. For me, the way it is now is the simplest for understanding and minimizes execution.

Just to be sure I tried it out. The following changes worked locally MigrateFieldInstance.php and MigrateFieldInstanceWidgetSettingsTest.php.

  bundle_mapped:
    plugin: static_map
    source: bundle
    bypass: true
    map:
      comment_node_forum: comment_forum
  bundle:
    plugin: field_bundle
    source:
      - entity_type
      - '@bundle_mapped'

  public function transform($value, MigrateExecutableInterface $migrate_executable, Row $row, $destination_property) {
    [$entity_type, $bundle] = $value;
    $lookup_result = NULL;
    // For comment entity types get the destination bundle from the
    // d7_comment_type migration.
    if ($entity_type === 'comment' && $bundle != 'comment_forum') {
      $value = preg_replace('/comment_node_/', NULL, $bundle);
      $migration = 'd7_comment_type';
      $lookup_result = $this->migrateLookup->lookup([$migration], [$value]);
      $lookup_result = empty($lookup_result) ? NULL : reset($lookup_result[0]);
    }
    return $lookup_result ? $lookup_result : $bundle;

We could, but I don't see the point.

It would mean that the generic field_bundle plugin wouldn't need to special case comment module - that'd be the only reason to do it.

Sort of see your point and I do like to use the pipeline instead of a process plugin. For me, this is an exception for the reasons stated above (and influenced by the pain of migrating fields in Commerce Migrate).

I have made a patch and will let whoever reviews this make the choice. That is, assuming the tests pass.

I don't feel strongly one way or the other. Adding to yaml is feels cleaner. RTBC #84.

Sadly, needs a reroll. From the error message it looks suitable for a novice.

Hear a reroll patch.

@kapilkumar0324, thanks for the reroll. It helps reviewers to have an interdiff, or diff, for rerolls. See Creating an interdiff. And if for any reason you think a diff is not necessary please add a comment staring why one is not included (and yes, I forgot to do that myself sometimes).

I've added a diff for the reroll. It looks good to me so I support this going back to RTBC.

I did not review the patch as a whole, but I did compare the patches in #84 and #88. The only difference is one line of context, as shown in the diff attached to #90.

I am setting the status back to RTBC based on the review in #85.

Re-uploading the patch from #88 so that testing happens on 9.2.x not 9.1.

It looks as though the patch needs a reroll. That is usually a Novice task, so I am adding the tag for that.

If you stick to the patch workflow, then please attach a diff (not an interdiff). See What about rerolled patches?.

You could also create an issue fork. Sometimes a change can be merged: the easy case of a reroll.

Rerolled patch #88

The patch #98 working fine for me. Failed test not related to the issue

@anushrikumari, thanks for the reroll. Can you add the diff for the reroll as asked for in #95.

1. I will ask the testbot to retest the patch in #98. It is good to have an opinion that it was an unrelated failure (and we do see those from time to time) but it is also good to get confirmation from the bot.
2. The testbot also shows three coding-standard violations. Search https://www.drupal.org/pift-ci-job/1904506 for "coding standards messages".
3. Either create an issue fork or attach a diff (not an interdiff) comparing to the patch in #88. See #95 for a link to instructions.

I am setting the status to NW for (2) and (3). This is still a Novice task.

Should fix the coding standard issues.

@sokru, thanks for the reroll.

This changes the value passed to the lookup to a string, not an array. The array was causing an exception when the plugin was not found because the error message correctly assumes the migration id is a string when adding it to the error message.

An unused use statement crept into the previous patch.

Comparing the patches in #88 and #106, I see a few differences:

1. The patch in #88 added a line to both the D6 and D7 versions of MigrateCommentTypeTest.php, and the patch in #106 adds a line to the D7 version only. Is that on purpose?
2. A line of context in that file. I guess that is why we need the reroll.
3. An updated comment and the change explained in #105.
4. A stray space was removed from the D7 fixture.

Of these, (2) and (4) look like improvements, and (1) looks like an accident. (Did the D6 version of that file also have a conflict?)

I wonder about the explanation in #105. The line in question (version from #88) is

+      $lookup_result = $this->migrateLookup->lookup([$migration], [$value]);

The migrateLookup property is injected from the migrate.lookup: service. Checking MigrateLookupInterface, the first parameter for lookup() can be either a string or an array of strings.

Where is the error message generated? (I guess the answer to that starts with looking at the failing test, but if you have already done that then I do not have to repeat the effort.) I think we may need a follow-up issue to fix that error message.

From 107,

1. Sorry, I did notice that the reroll in 106 had removed the line from the d6 version of the test but I forgot to go back and fix that. The test is restored now in this patch.

Now to explain why the migration_id to the migrate lookup service should be a string in this case. The error in DoubleSlashTest when the first parameter is an array, $lookup_result = $this->migrateLookup->lookup([$migration], [$value]); is:

1) Drupal\Tests\migrate_drupal_ui\Functional\d7\DoubleSlashTest::testMigrateUpgradeExecute
Exception: Notice: Array to string conversion
Drupal\Component\Plugin\Exception\PluginNotFoundException->__construct()() (Line: 22)

That is because PluginNotFoundException expects the first parameter to be a string.

The MigrateLookup service needs to use a custom message and handle an array of migration ids, instead of doing this:

  public function lookup($migration_id, array $source_id_values) {
    $results = [];
    $migrations = $this->migrationPluginManager->createInstances($migration_id);
    if (!$migrations) {
      throw new PluginNotFoundException($migration_id);
    }

The fix here is valid but we need a followup one for the lookup service. I plan to check for an existing issue, which I can do later.

Didn't find an existing issue so made a followup as per #108, #3187386: Use a custom error message for PluginNotFoundException in the migratelookup service.

I compared the current patch (#108) to the one in #88. I now see changes 2, 3, 4, but not 1 (referring to the list in #107). The line of context (item 2) has changed for both the D6 and D7 versions of MigrateCommentTypeTest.php.

Based on the review in #85 and the changes since then, I am satisfied. RTBC

Thanks for the further explanation and the follow-up issue. I agree with the decision to work around that bug here instead of postponing on the new issue.

Have reviewed this a few times, and always think I've found an issue, then talk myself out of it, then leave it for a while, then do the same thing again. I think this is fine so committed/pushed to 9.2.x and cherry-picked to 9.1.x, thanks!

+++ b/core/modules/field/migrations/d7_field_instance.yml
@@ -19,12 +19,17 @@ process:
-  bundle:
+  bundle_mapped:
     plugin: static_map
     source: bundle
     bypass: true
     map:
       comment_node_forum: comment_forum
+  bundle:
+    plugin: field_bundle
+    source:
+      - entity_type
+      - '@bundle_mapped'

FYI: This change broke the Paragraphs migration in contrib! 😞

See #3203739: Paragraphs migrations broken in Drupal >=9.1.4 for details + fix.