Move forum related logic from taxonomy migrations to new forum migrations

I think the source of the problem is that the taxonomy source plugins do not exclude forum. Moving code related to the process plugins, as suggested in #13, and isn't going to change that.

To see what this looks like here is a patch that removes the forum processes from the taxonomy migrations and adds them back in hooks. There are updates to the forum fixtures, so I have made a patch without that to make it easier to review. And I did not look at the Functional tests, so I expect failure.

Most failures due to errors in forum_migration_plugins_alter.

The error in \Drupal\Tests\forum\Kernel\Migrate\d6\MigrateForumTest is fixed in 📌 Change MigrateForumTest to use forum test fixture Fixed .

The test MigrateLanguageContentTaxonomyVocabularySettingsTest.php isn't right. There should be one in forum that tests for the changed name.

The last submitted patch, 17: 2914251-17.patch, failed testing. View results →

The last submitted patch, 18: 2914251-18.patch, failed testing. View results →

I thought I ran those failing tests locally but I ran the ones in migrate_drupal_ui out of habit and not the ones in forum. The updates the Upgrade tests for the changes to the source fixtures. I also sorted the entity count array which make is appear that more counts were changed. I did that because it is just a pain that the lists never got sorted.

This should results in 1 failing test, \Drupal\Tests\forum\Kernel\Migrate\d6\MigrateForumTest which is fixed elsewhere.

The last submitted patch, 21: 2914251-21.patch, failed testing. View results →

🔥 this is amazing! Looks like just that one fail (one is random)

1. +++ b/core/modules/forum/forum.module
   @@ -643,3 +654,95 @@ function template_preprocess_forum_submitted(&$variables) {
   +        $forum_container_tids = unserialize($result[0]);
   ...
   +      $forum_nav_vocabulary = unserialize($result[0]);
   

   paranoia but let's add the ['allowed_classes' => FALSE] as the second argument here

2. +++ b/core/modules/forum/forum.module
   @@ -643,3 +654,95 @@ function template_preprocess_forum_submitted(&$variables) {
   +function merge_forum_vocabulary($process) {
   

   this is only used inside the alter hook, so let's make it a closure inside that function instead

3. +++ b/core/modules/forum/tests/src/Kernel/Migrate/d6/MigrateTaxonomyTermTest.php
   @@ -0,0 +1,134 @@
   +    $this->assertEquals($expected_parents, $this->getParentIDs($id));
   

   this could use array_column($entity->get('parent')->getValue(), 'target_id') and save the need for the protected method as well as a few extra queries

If I don't update this by Monday, feel free to set to 'unassigned'.

This patch address all the items in #23. There were no issues, it was all straightforward.

The last submitted patch, 25: 2914251-25.patch, failed testing. View results →

Rerolled. Also, the incremental migrations are not run in the functional test so the method returns an empty array instead of entity counts.

The last submitted patch, 28: 2914251-28.patch, failed testing. View results →

Adding a test file to the d6 and d7 Functional tests.

The last submitted patch, 30: 2914251-30.patch, failed testing. View results →

This will fix one of the tests. It needed checks forum_migrate_prepare_row and forum_migrate_d7_taxonomy_vocabulary_prepare_row that the results from a query exist before trying to use them. And a change to \Drupal\Tests\forum\Functional\migrate_drupal\d6\Upgrade6Test::getEntityCountsIncremental to return an empty array because we are not testing incremental migrations here. Also re-installed Upload in the d6 database.

The last submitted patch, 32: 2914251-32.patch, failed testing. View results →

It too a long time but I finally spotted the problem. In forum_migration_plugins_alter I have the machine name as 'forum' instead of 'taxonomy_forum'. With that fixed all the remained was to correct the tests.

The last submitted patch, 34: 2914251-34.patch, failed testing. View results →

Ignore #34.

While investigating the error in #32 I saw that forum is being installed in two Migration tests in the taxonomy module. This should fix both of those and move what needs to move to forum tests. I also made MigrateTestTrait in forum to make the tests a bit easier to follow.

Just entity count changes.

The last submitted patch, 37: 2914251-37.patch, failed testing. View results →

Now to fix the error in MigrateForumTest. It seems that I did not complete forum_migration_plugins_alter and didn't have the correct process for the field name, that have been removed from the taxonomy migrations. With that fixed some tests needed to use the correct field name.

Ready for review again.

I went through each of the tests here that extend from core tests (other than base classes) and made sure we've named the test method the same and that the parent classes don't contain any other test methods (so we're not inadvertently running the same identical test twice).

All of them looked good.

The changes since #23 look good to me.

This was a huge effort @quietone and my comment 'just one fail' was famous last words.

Thanks for all your efforts here!

This needed a reroll. I also sorted the list of entity types in the forum tests, Upgrad6Test and Upgrade7Test

The last submitted patch, 42: 2914251-42.patch, failed testing. View results →

Ignore #42. I wasn't working from a clean site.

Since this was previously RTBC and reroll #44 seems good going to remark.

Looks great and how it should have been done all along as evidenced by the age of the issue.

Committed/pushed to 11.x, thanks!

This needs a change record and then a followup to correct the URL in the deprecation messages. I am working on this now.

Change record made and published. The followup is 🐛 Fix change record link added in #2914251 Active

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

    if (is_a($source_plugin, D7Vocabulary::class)
      && !is_a($source_plugin, D7VocabularyTranslation::class)
      && !is_a($source_plugin, D7LanguageContentSettingsTaxonomyVocabulary::class)) {
      if (isset($migration['process']['vid'])) {
        $process[] = $migrations[$migration_id]['process']['vid'];
        $migrations[$migration_id]['process']['vid'] = $merge_forum_vocabulary($process);
      }
    }

This code can lead to problems.

In core 'd7_taxonomy_vocabulary', $migrations[$migration_id]['process']['vid'] is a single process plugin, so all is good.

But with a custom migration, as with migrate_plus, $migrations[$migration_id]['process']['vid'] is already an array of process plugins. The $process[] = .. assignment will result in a nesting level that is one level too deep.

As a result, I get "Undefined array key "plugin" Migration.php:727" for the upgrade_d7_taxonomy_vocabulary migration.

In fact, the upgrade_d7_taxonomy_vocabulary generated with migrate_plus contains has the 'forum_vocabulary' processor in 'vid'.
So adding it again would result in duplication.

As a workaround, I can edit the custom migration, by copying the 'vid' process setup from 'd7_taxonomy_vocabulary'.
Still, this should be fixed.