Problem/Motivation

Follow-up to #2903007: [D7] Forum containers are migrated as forums.
Original report:

I successfully migrated my site from D7 to D8, but when I try to create new forum container it saves as forum after submit. I checked taxonomy_term__forum_container and it doesn't have new records after I tried to create new containers.

The problem is that vocabulary migration create machine names based on vocabulary labels. Also, on D7, we can modify the machine name for the forum vocabulary. Since the forum module already provides his own forum vocabulary, we should make sure the migration map the D7 forum vocabulary to this one. The D7 vocabulary that D7 Forum uses is defined in variable 'forum_nav_vocabulary'.

Proposed resolution

  • D8 Forum uses hard-coded vocabulary with vid 'forums'
  • Determine the D7 Forum vocabulary from 'forum_nav_vocabulary' variable
  • Map this D7 vocabulary to D8 'forums' vocabulary

Remaining tasks

Review.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#16 interdiff-2914249-11-16.txt8.26 KBmaxocub
#16 2914249-16.patch11.48 KBmaxocub
#11 2914249-10.patch10.2 KBmaxocub
#11 2914249-10-failing.patch1.87 KBmaxocub

Comments

maxocub created an issue. See original summary.

maxocub’s picture

maxocub commented

@dillix: I have tried many different way to reproduce the problem you have but with no success. I looked at a few modules you say you have enabled in your comment here but haven't found anything there neither.

Obviously, I can't try them all, so it's really hard to understand what is going wrong. We also have different people having manually tested the forum migration without being able to reproduce the forum containers creation bug.

If we look at the two methods dealing with forum and container creation :

  /**
   * Returns add forum entity form.
   *
   * @return array
   *   Render array for the add form.
   */
  public function addForum() {
    $vid = $this->config('forum.settings')->get('vocabulary');
    $taxonomy_term = $this->termStorage->create([
      'vid' => $vid,
      'forum_controller' => 0,
    ]);
    return $this->entityFormBuilder()->getForm($taxonomy_term, 'forum');
  }

  /**
   * Returns add container entity form.
   *
   * @return array
   *   Render array for the add form.
   */
  public function addContainer() {
    $vid = $this->config('forum.settings')->get('vocabulary');
    $taxonomy_term = $this->termStorage->create([
      'vid' => $vid,
      'forum_container' => 1,
    ]);
    return $this->entityFormBuilder()->getForm($taxonomy_term, 'container');
  }

I don't see how a migration can affect the creation of forum containers. I can only suppose that one of your enabled contrib or custom modules might be messing with either the creation of taxonomy terms of altering the taxonomy terms forms.

If you can, please try to debug what is happening in with those methods above (and the forms they redirect to) on your migrated site with the bug.

dillix’s picture

dillix commented

@maxocub I can create containers before starting migration, so I don't think that this issue is a contrib bug. I have issue after migration, so I think its a migration bug...

maxocub’s picture

maxocub commented

@dillix: Yes, I understand, be assured that I want to be able to reproduce your bug so it can be fixed but so far nobody was able.

Do you have any custom migrations?

dillix’s picture

dillix commented

@maxocub no all migrations are standard. In my D8 site config I have only one language (Russian) and it sets as default in settings.

I investigated a bit more my problem and found that during migration drupal creates new vocabulary Forums and imports here terms and after migration I have 2 vocabs (Forums and Форумы), so relation between forum functionality and vocabulary breaks. Also when I open taxonomy_forums field settings for forum content type there isn't any taxonomy vocab checkbox marked here (but before migration it has been set to Форумы after I enabled forum module).

PS: May be #2914251: Move forum related logic from taxonomy migrations to new forum migrations will fix my issues...

masipila’s picture

masipila commented

I tried to reproduce this using a D8 site that was in Finnish only but unfortunately I was not able to reproduce this.

I propose that we approach this very systematically and figure out exact steps to reproduce. @dillix, could you please provide the following information (and all observations that you can make).

1. What is the language setup of your D7 site?

2. Could you please take screenshot of the D7 admin page admin/structure/forum

3. Could you please take screenshot of the D7 admin page admin/structure/taxonomy
- It is especially interesting to know what is the machine name of the D7 vocabulary that you have for forum terms
- The machine name can be seen if you go to edit the vocabulary

4. Your D8 site is installed in Russian only, correct?

5. Is your D8 site is installed using Standard installation profile?

6. Could you please take screenshot of the D8 admin page admin/structure/forum *after* your migration

7. Could you please take screenshot of the D8 admin page admin/structure/taxonomy *after* your migration
- Is it so that you have now two vocabularies here?

Cheers,
Markus

dillix’s picture

dillix commented
Status: Postponed (maintainer needs more info) » Active

1. English + Russian, Russian set to default, here is screenshot: http://joxi.ru/nAyBGnvuYqQ19r

2. Here is screenshot of D7 admin page admin/structure/forum: http://joxi.ru/823bB9dsJy4e5m

3. Here is screenshot of D7 admin page admin/structure/taxonomy: http://joxi.ru/RmzeG9ES0x5oEr
and here is screenshot with machine name: http://joxi.ru/Vrw3vVEIOWXnNr

4. Yes, here is only Russian lang, here is screenshot: http://joxi.ru/82QD9ZBFjJdEwm

5. Yes we used a Standard installation profile.

6. Here is D8 admin/structure/forum on migrated site:
http://joxi.ru/Vrw3vVEIOWX1Nr
and here is D8 admin/structure/taxonomy after migration: http://joxi.ru/DrlNXvghvqZpe2

Some additional info on our setup:
- we have dev server on d8.example.com in pre migration state where we commit new modules and stage server test.example.com it is copy of dev site with patches where we test migrations. Before we test new migration patch we resync test server with fresh copy of dev site.
- as you can see on D7 we have machine name forums and on D8 all terms migrated to forumy and there is also empty vocab with machine name forums
- also we have container Инфо-Бухгалтер 8 and forum with the same name in container Дилеры (Dealers), may be this is a source of migration bug?

maxocub’s picture

maxocub commented

One other thing, could you give us the value (vid) of the forum_nav_vocabulary variable on your Drupal 7 site, and what is the machine name of the vocabulary associated with that vid?

maxocub’s picture

maxocub commented
Issue tags: +migrate-d7-d8, +i18n

OK, I've been able to reproduce it, finally. The cause of the bug is the duplicated forum vocabulary. The first one (machine name 'forums') is the one provided by the forum module. The second one (machine name 'forumy') has been created by the d7_taxonomy_vocabulary migration:

  vid:
    -
      plugin: machine_name
      source: name
    -
      plugin: make_unique_entity_field
      entity_type: taxonomy_vocabulary
      field: vid
      length: 32
      migrated: true

Since the vocabulary name was translated, the migrated vocabulary name will be generated based on that translation.

With the D6 forum migrations, we had to do some static mapping to get the forum field name to match with the ones provided (and hardcoded) by the forum module. I will try to see if we have to do something similar here for the vocabulary name.

dillix’s picture

dillix commented

@maxocub great news!

# drush vget forum_nav_vocabulary
forum_nav_vocabulary: '12'

Machine name on D7 is "forums": http://joxi.ru/4AkvnygFyJlMR2

maxocub’s picture

maxocub commented
Title: Can't create forum container after migration from D7 » Translated forum vocabulary migration creates duplicate forum vocabularies
Issue summary: View changes
Issue tags: +migrate-d6-d8
StatusFileSize
new 2914249-10-failing.patch1.87 KB
new 2914249-10.patch10.2 KB

Here's a first try.

maxocub’s picture

maxocub commented
Status: Active » Needs review

NR.

dillix’s picture

dillix commented
Status: Needs review » Reviewed & tested by the community

Wow, thanks @maxocub!

I tried patch: https://www.drupal.org/files/issues/2914249-10.patch and it works.
All containers was migrated fine: http://joxi.ru/V2Va9DbTxlqGpm
There is one vocab Форумы with machine name: forums (as on D7)

Also I was able to create new container: http://joxi.ru/1A5yB5KIn8Jk4A

So I can mark this issue to RTBC.

The last submitted patch, 11: 2914249-10-failing.patch, failed testing. View results

masipila’s picture

masipila commented
Status: Reviewed & tested by the community » Needs work

Couple nits from the comments:

1. This appears on multiple places

+      # This plugin checks if the vocabulary being migrated is the one used for
+      # forums. If it is, the specified machine name, the one expected by the
+      # forum module, will be used . Otherwise, it is left unchanged.

The new sentence structure is quite complex. While it's propably grammatically correct English (except one whitespace before .) it is quite difficult to understand for non-native speakers. If I remember coding standrds correctly module names are supposed to be written with first capital letter and without 'module' i.e. 'Forum' instead of 'forum module'.

Proposed wording:

This plugin checks if the vocabulary being migrated is the one used by Forum. If yes, we map to the machine name Forum expects. Otherwise we leave it unchanged.

2.

+++ b/core/modules/taxonomy/src/Plugin/migrate/process/ForumVocabulary.php
+ * The forum module is expecting specific machine names for its field and
+ * vocabulary names. [...]

+ * D8 Forum is expecting specific machine names for its field and
+ * vocabulary names.

3.

+++ b/core/modules/taxonomy/src/Plugin/migrate/source/d7/Vocabulary.php
+  public function prepareRow(Row $row) {
+    // If the vid of the vocabulary being migrated is equal to the vid in the
+    // 'forum_nav_vocabulary' variable, set the 'forum_vocabulary' source
+    // property to true so we can know this is the vocabulary used for forums.

If the vocabulary being migrated is the one defined in the 'forum_nav_vocabulary' variable, set the 'forum_vocabulary' source property to true so we know this is the vocabulary used for Forum.

Cheers,
Markus

maxocub’s picture

maxocub commented
Priority: Normal » Critical
Status: Needs work » Needs review
Issue tags: +Migrate critical
StatusFileSize
new 2914249-16.patch11.48 KB
new interdiff-2914249-11-16.txt8.26 KB

Thanks for the review @masipila, I agree with your comments but I made small adjustments, even though English is not my native tongue!

Setting as Critical since it causes data loss and breaks Forum.

masipila’s picture

masipila commented
Status: Needs review » Reviewed & tested by the community

Nits from #15 are addressed.

Only changes between 11-16 are in comments so the manual test results from #13 are still valid.

Tests seem to be are covering this bug.

RTBC.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Reviewed & tested by the community » Needs review 
  1. +++ b/core/modules/migrate_drupal/tests/fixtures/drupal7.php
@@ -45365,8 +45365,8 @@
-  'name' => 'Forums',
-  'machine_name' => 'forums',
+  'name' => 'Sujet de discussion',
+  'machine_name' => 'sujet_de_discussion',

    I don't understand why this is being changed in the fixture itself? 

  2. +++ b/core/modules/taxonomy/src/Plugin/migrate/source/d7/Vocabulary.php
@@ -49,6 +50,20 @@ public function fields() {
+  public function prepareRow(Row $row) {
+    // If the vocabulary being migrated is the one defined in the
+    // 'forum_nav_vocabulary' variable, set the 'forum_vocabulary' source
+    // property to true so we know this is the vocabulary used by Forum.
+    if ($this->variableGet('forum_nav_vocabulary', 0) == $row->getSourceProperty('vid')) {
+      $row->setSourceProperty('forum_vocabulary', TRUE);
+    }
+
+    return parent::prepareRow($row);
+  }

    We should open a follow-up to move this logic into a Forum-specific variant of this plugin. Forum could even swap in its own implementation of the plugin (using the ol' plugin hijacking technique) which implements this logic.

dillix’s picture

dillix commented

@phenaproxima followup #2914251: Move forum related logic from taxonomy migrations to new forum migrations was created by @maxocub

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Related issues: +#2914251: Move forum related logic from taxonomy migrations to new forum migrations

Cool, let us reference it from here then :)

maxocub’s picture

maxocub commented
Related issues: -#2914251: Move forum related logic from taxonomy migrations to new forum migrations

Re #18.1: This change is for testing that if someone change the name or machine_name of the forum vocabulary, it will still be migrated into the vocabulary provided by D8 Forum. Do you think of another way to test that?

maxocub’s picture

maxocub commented
Related issues: +#2914251: Move forum related logic from taxonomy migrations to new forum migrations

Oups, cross post error.

dillix’s picture

dillix commented

I think that if we move forum migrations to forum module can it break containers again?

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

I think that if we move forum migrations to forum module can it break containers again?

This is why we write tests. :)

masipila’s picture

masipila commented

Re: #18.2.

I clarified the issue summary of the follow-up so that it's clear that the follow-up has one part for the Term migration and now also another for Vocabulary part. #2914251: Move forum related logic from taxonomy migrations to new forum migrations.

Cheers,
Markus

masipila’s picture

masipila commented

Re #18.1 and #21.

We were working with this with maxocub in parallel and we found two things.

One way to reproduce this is related to the translations; this is how @dillix ran into this issue.

  • His forum vocabulary indicated by 'forum_nav_vocabulary' was vid 12.
  • His vocabulary 12 had machine_name 'forums' and name 'Форумы'
  • D8 vid is currently being derived from 'name' and that's why migrations created a new vocabulary for him instead of mapping to 'forums' what D8 Forum expects.

I found that it is possible to change the 'machine_name' of a vocabulary in D7 UI.

  • As explained above, the Vocabulary migration derives the D8 vid from D7 'name' before this patch.
  • Because the D7 'machine_name' can be manually changed, deriving the D8 vid from 'machine_name' and assuming hard-coded 'forums' does not work for all sites even though it would have worked for @dillix
  • Hence: maxocub wrote this patch so that we determine the 'Forum relevant' vocabulary based on D7 'forum_nav_vocabulary' variable and map that to D8 'forums'
  • (And I created an independent issue #2914649: [D7] Vocabulary migration: vid incorrectly mapped from vocabulary name instead of machine_name which changes the D8 vid mapping so that it uses 'machine_name' and not 'name' as the source.)

The fixture change that was asked in #18.1 covers all scenarios:

Hope this helps. I'll leave the update to RTBC to @phenaproxima but both 18.1 and 18.2 have been addressed properly in my opinion.

Cheers,
Markus

Edit: typos, minor clarifications.

masipila’s picture

masipila commented
Issue summary: View changes

Updated issue summary to help reviewers.

dillix’s picture

dillix commented

I've found another bug on forum migration:
entity reference field in forum node type didn't attached to forum vocab after migration (as it was before migration on D8), so I need to edit field settings and set vocab manually. Is it related to this issue or I should create separate?

masipila’s picture

masipila commented

@dillix, I'd say it's a separate issue. The scope of this issue is the mapping of D7 Forum vocabulary to D8 Forum vocabulary.

Could you please open a new issue with screenshots before the migration and what you would have expected? I can have a look at it right away.

Cheers,
Markus

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

My points of review are addressed. Back to RTBC.

I don't like how we're suddenly having to do all this dancing around Forum weirdness, but then again I also think Forum should be kicked out of core by a leg in a boot that is on fire. So I greatly look forward to the day when all the Forum-specific migration nuttiness is confined to the Forum module, thanks to the follow-up(s), after we fix the immediate critical issues.

Let's get this in.

  • catch committed a20201a on 8.5.x 
    Issue #2914249 by maxocub, dillix, masipila, phenaproxima: Translated...

  • catch committed 152e49f on 8.4.x 
    Issue #2914249 by maxocub, dillix, masipila, phenaproxima: Translated...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Really good to see this one reproduced and fixed, glad we got there!

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

I'll un-postpone the follow-up.

Status: Fixed » Closed (fixed)

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