d7_language_content_comment_settings triggers MigrateException if the source bundle is longer than 32 chars: use migration_lookup

Ah, nice discovery. I haven't checked but are there any other pipelines that should be changed?

+++ b/core/modules/content_translation/migrations/d7_language_content_comment_settings.yml
@@ -8,22 +8,20 @@ source:
+      plugin: default_value
+      default_value: null

This shouldn't be needed migration_lookup returns NULL if destination_ids are not found.

And, of course, will need a test.

@huzooka, as we have learned I prefer shorter titles, because they are easier to skim, to yours. Here, I has just removed the phrase that was in parenthesis.

This looks closely related to #2565931: Handle long comment bundle names.

For that issue a node type a_thirty_two_letter_type_name was added to the drupal 7 fixture. That node type isn't translatable though. Seems like the cleanest way to add test coverage here would be to update the fixture such that this long-named node type is declared as translatable.

That said, I haven't been able to successfully edit the fixture. I've been trying to do it manually in a code editor but I'm getting de-serialization problems even when I'm super careful. I'm going to try to learn how to do that.

Thanks for the link and the advice, @huzooka.

I think I have a test here that will work for us. I updated the fixture such that a_thirty_two_character_type_name has the same multilingual setting as the page node type. I'm expecting the fail test to throw an exception and report a message like the one in the IS.

Turns out the serialization problem I was seeing was unrelated to the fixture, by the way. It was in fact the result of an uncaught exception, which should be caught in the updated test.

Here's another patch that uses startCollectingMessages() instead of try/catch. It also updates the migrate_drupal_ui test that failed due to the updated fixture.

There are a couple things I still feel kind of funny about.

1. Is it weird to do an assertion in setUp()?
2. I don't really understand the difference between MigrateLanguageContentCommentSettingsTest and MigrateLanguageContentCommentSettingsNoEntityTranslationTest. There's a ton of repeated code. I decided to just go along with it and continue duplicating code. This looks like it could be cleaned up, but maybe that would be an issue of its own.

@danflanagan8, thanks for working on this, especially as it requires altering the fixture. Ping me any time in #migration with your questions.

13.1 For a migration test, yes. I can't speak for other sub systems. I'd move the assertion to the test method and add a comment. Right now I am thinking along the lines of 'There should be no error message about a missing bundle'.
#13.2. The difference is that MigrateLanguageContentCommentSettingsNoEntityTranslationTest alters the dump file to disable the entity translation on the comment fields, in the variable entity_translation_entity_types. That value of that variable is put on the row by the source plugin LanguageContentSettings and processed in ContentTranslationEnabledSetting to determine if the comment field should be translatable or not. Does that help?

It is not ideal but not uncommon for migration tests to have repeated code. It is just how things evolved where getting the system functional was high priority. (But I was able to not make some of the same mistakes when I worked on the Commerce Migrate tests).

1. Yes, but the real weirdness here is that the migration is running in the ::setUp() IMHO :D It'd be better to move that into the test method.

Thanks for pushing this forward, @danflanagan8! :)

Thanks @Wim Leers and @quietone. Here's the latest. After I confirm everything still fails correctly (if you know what I mean!) then I'll post a patch that adds the fix from #4.

Now let's add the fix. I took the patch form #4 and incorporated the feedback from #5.

LOVELY!

#17 fails with

1) Drupal\Tests\language\Kernel\Migrate\d7\MigrateLanguageContentCommentSettingsTest::testLanguageCommentSettings
Missing bundle entity, entity type comment_type, entity id comment_node_a_thirty_two_character_type_name. (/var/www/html/core/lib/Drupal/Core/Entity/EntityType.php:877)
Failed asserting that an array is empty.

#19 only adds the original fix by @huzooka from #4. And now it's green.

So… RTBC! :)

@danflanagan8, Nice work!

I uploaded the fixture to a D7 site and confirmed it is still functional. Yay! I can't recall, how does one change the variable 'language_content_type_a_thirty_two_character_type_name" via the UI?

I then skimmed the comments and read the patch.

Re #7, For migration tests it is not unusual for the migrations to run in the setup, in fact, it has been the practice. Whether that is good or best practice is a topic for debate, but I don't think this issue is the place. Nor is this issue the place to change existing practice of several years. Speaking for myself, reading the patch took longer simply because of the change of pattern. But it does make this patch larger than it needs to be. So, since I already applied the patch I chose to make a new patch with those changes..

And one other question.

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/Upgrade7Test.php
@@ -89,7 +89,7 @@ protected function getEntityCounts() {
-      'language_content_settings' => 22,
+      'language_content_settings' => 24,

Anyone know why this increased by 2?

Anyone know why this increased by 2?

Not me :)

As for putting the migrations back into setUp(), I'm personally fine with that's the de facto standard in the migration system.

I though it would be comment and it was. The extra language_content_settings are

language.content_settings.comment.comment_node_a_thirty_two_char
language.content_settings.node.a_thirty_two_character_type_name

The patch from #21 just changed the tests back to where they were in #13. (With the small change that $this->assertEmpty($this->migrateMessages, $this->migrateMessages['error'][0] ?? ''); runs in the test instead of the setup.)

#13 fails with

Drupal\Tests\language\Kernel\Migrate\d7\MigrateLanguageContentCommentSettingsNoEntityTranslationTest::testLanguageCommentSettings
Missing bundle entity, entity type comment_type, entity id comment_node_a_thirty_two_character_type_name. (/var/www/html/core/lib/Drupal/Core/Entity/EntityType.php:877)
Failed asserting that an array is empty.

just like #17.

The fix did not change between #17 and #21.

So this one should be ready to RTBC again, but I can't be the one that flips the switch.

#21 === #19, with the only difference being

Re #7, For migration tests it is not unusual for the migrations to run in the setup, in fact, it has been the practice. Whether that is good or best practice is a topic for debate, but I don't think this issue is the place. Nor is this issue the place to change existing practice of several years.

This is not a functional change, so back to RTBC.

Committed and pushed 0acc2f337c to 9.3.x and b9e1ed7cb9 to 9.2.x. Thanks!