Problem/Motivation

I migrated my D7 site to 8.4.x-dev. When migrating forum content, I noticed that the Replies and Last reply columns of the forum thread list view are not taking the migrated comments into account.

Please see the attached screenshot: d8-forum-migrate.PNG

  • The first thread in the screenshot has actually 32 migrated comments and 1 comment added in D8. The view is showing that the thread would only have 1 comment instead of 33. This one comment is the one added in D8.
  • The second thread has only 1 comment which was added in D8. This comment is shown correctly in the comment count.
  • The third thread is created in D8 and it has 1 comment which was added in D8. This comment is shown correctly in the comment count.
  • All other threads are migrated ones and they all have comments but the Replies and Last reply columns are not showing them.
  • The comments can be seen if you drill into the thread.
  • The issue persists even if I clear all caches.

The root cause for this bug is that the database table comment_entity_statistics is populated incorrectly when migrating comments.

  • Drupal 8 forum reads the comment statistics from database table comment_entity_statistics with field_name comment_forum. See comments #4 and #5 for more details.
  • The migrate process currently creates an incorrect entry to this table with field_name comment_node_forum
  • See attached screesnshot on how the entries look like: comment_entity_statistics.PNG

When investigating further, this same bug has also another symptom. The built-in content types Article and Forum have two comment types after migration:

  • See attached screenshot from article field settings: 2853872-duplicate-comment-types-article.png
  • Vanilla D8 article content type already has a comment type comment but the D7-D8 migration adds also comment type comment_node_article. As a result, the content type has now two commen types. When comments are migrated, the comment_entity_statistics table gets incorrectly populated.
  • See attached screenshot from forum field settings: 2853872-duplicate-comment-types-forum.png
  • Vanilla D8 forum content type already has a comment type comment_forum but the D7-D8 migration adds also comment type comment_node_forum. As a result, the content type has now two commen types. When comments are migrated, the comment_entity_statistics table gets incorrectly populated as illustrated above in the earlier screenshot.

Proposed resolution

  • Do not create comment types comment_node_forum and comment_node_article for forum and article content types because these content types already have the out of the box comment fields. Several comment / field migrations need to be updated.
  • Introduce a new process plugin skip_row_if_in_blacklist which allows us to skip comment_node_forum and comment_node_article rows in these migrations.
  • Use static_map process plugin to map the D7 comment / field migrations to the D8 standard comment_forum and comment comment types
  • Remark: the third standard D8 content type Page does not have any comment fields by default so that is not suffering from this issue. No actions required for Page content type.

Remaining tasks

1. Review patch.
2. Evaluate if this same bug applies to D6-D8 migration as well.

User interface changes

None.

API changes

None.

Data model changes

None.

Comments

masipila created an issue. See original summary.

masipila’s picture

Issue summary: View changes
masipila’s picture

Issue summary: View changes

I just reproduced this with a vanilla D7. I can now confirm that this has nothing to do with the text formatting issue I mentioned in the issue summary earlier so I removed that part of the issue summary.

masipila’s picture

I was troubleshooting this further. The topic list is returned by ForumManager::getTopics.

The comment count and last comment information is read from the database table comment_entity_statistics:

      $query->join('comment_entity_statistics', 'ces', "n.nid = ces.entity_id AND ces.field_name = 'comment_forum' AND ces.entity_type = 'node'");
      $query->fields('ces', array(
        'cid',
        'last_comment_uid',
        'last_comment_timestamp',
        'comment_count'
      ));

I had a look at the database table comment_entity_statistics for one of the forum nodes that I migrated. We have two entries in this table, please have a look at the attached screenshot comment_entity_statistics.PNG.

  • ForumManager::getTopics is joining the comment_entity_statistics table with the field_name = 'comment_forum'
  • The comment count is migrated to comment_entity_statistics table to the other entry, where field_name equals to comment_node_forum

Markus

masipila’s picture

Assigned: Unassigned » masipila

Notes from further investigation. I installed a vanilla D8 (no migrations) and checked how the comment_entity_statistics table behaves.

Tests and findings:

  • I created an article node (entity_id: 1) and added a comment. comment_entity_statistics shows 1 record with field_name comment which is the Default comments comment type that can be seen at admin/structure/comment
  • I enabled forum, created a topic (entity_id: 2) and added a comment to this topic. comment_entity_statistics shows 1 record with field_name comment_forum which is the Comment_forum comment type that can be seen at admin/structure/comment
  • This matches my earlier finding in comment #4: The forum topic list is checking the statistics from comment_entity_statistics by a join to field_name = comment_forum

Conclusion: The D7-D8 comment migration is incorrectly creating entries to comment_entity_statistics with field_name comment_node_forum. It should create the entries with field_name comment_forum.

Additional note: the comment migration seems to create separate comment types for each D7 content type. There is probably some conceptual reason for this behavior but that logic is NOT OK for forum comments according to the analysis above. I'll create a patch that handles forum comments correctly.

Cheers,
Markus

masipila’s picture

Assigned: masipila » Unassigned
Issue summary: View changes

This turned out to be a bit bigger topic than I first thought and I'm not able to create the patch for now. Issue summary updated with a proposed resolution.

masipila’s picture

Title: Migrated forum comments not taken into account on forum thread list view » Comment entity statistics incorrectly migrated for forum comments
masipila’s picture

Title: Comment entity statistics incorrectly migrated for forum comments » Migration for forum and article comments: duplicate comment types and incorrect comment_entity_statistics
Issue summary: View changes
FileSize
22.35 KB
31.3 KB

I tested this further. Article and Forum content types have duplicate comment fields after the migration. This is because of the same issue than comment_entity_statistics described earlier.

Issue summary updated with the detailed findings.

masipila’s picture

Assigned: Unassigned » masipila

I'll try to get a patch for this during this week.

masipila’s picture

Assigned: masipila » Unassigned
Status: Active » Needs review
FileSize
4.5 KB

Here's the initial patch for the first round of review. No tests have been updated yet, I'd like to get feedback on the approach first.

Status: Needs review » Needs work

The last submitted patch, 10: 2853872-article_forum_comment_type_migration-10.patch, failed testing.

masipila’s picture

Issue summary: View changes

Issue summary updated to help reviewers. I was previously wondering if we should try to write a hook_update to fix previously migrated (corrupted) data but that goes very difficult very quickly and I don't think that will be realistic.

quietone’s picture

I wonder if we can use the existing cck/field plugins to do the mapping that is needed.

alayham’s picture

I think the root cause of this issue is that comment_forum is hardcoded in core. I created a feature request #2874487: Hardcoded field names in Core should be assigned/returned dynamically to remove hardcoded field names in future versions while we work on solving this problem with the current core hardcoded fields.

phenaproxima’s picture

Apparently this is the flip side of #2669030: Can't migrate forum from drupal 6 to drupal 8, which is Migrate critical. So this one joins the family too.

phenaproxima’s picture

Issue tags: +Needs tests

This will certainly need tests.

masipila’s picture

@phenaproxima, do you have any input on the proposed approach? See the Remaining Tasks of the issue summary.

Cheers,
Markus

phenaproxima’s picture

I think my main problem with the approach here is that we are altering source plugins to make special exceptions for certain cases. And I think that is a dangerous path for us to take.

I'd rather we skipped rows, or made the required changes, in process pipelines where things are more visible and explicit. If needed, we can write specialized process plugins to do what's needed.

Granted, I'm saying this off the top of my head, without having read through the issue, so maybe that approach has already been tried and abandoned...but if not, I'd prefer approaching it that way.

masipila’s picture

Hi, that approach has not been evaluated and abandonded.

When I analyzed this issue some months back, I had zero experience with the whole migration API and as I wrote with the first quick & dirty patch I started to think that I'm not doing the right thing. That's why I wrote to the issue summary that the very first thing is to evaluate the whole approach.

@quietone was planning to check this issue and provide some guidance on the direction but unfortunately he has not had time to do that.

I've now written some custom migrations for my own sites so I'm now more familiar with the migration API but as I'm still fresh on the topic I would still appreciate some guidance. That is, if you wish me to help with the actual fix in addition to the root cause analysis I've already done. If some of the module maintainers / more experienced regular contributors want to take this I'm of course ok with that as well.

Long story short: I'm offering my help forvthis but I would need some mentoring. I'm on the #drupal-migrate IRC channel so you can also pijg me there.

Cheers,
Markus

masipila’s picture

Hi again,

I was educating myself a bit on the process pipeline. Would fhe more elegant approach be as follows, I'll use the migration of comment types as an example:

Problem that we are trying to solve, using comment type migration as an example
Currently d7_comment_type is creating duplicate / unwanted comment types for forum and article. These content types already have the out-of-the-box comment types and we want to use these instead of creating second comment types. See #8 for the details on this.

Introduce process pipeline, still using comment type migration as an example
1. Write a new process plugin which would be called something like skip_on_forum_and_article

2. Modify migration template d7:comment_type so that we first pass the row through the skip_on_forum_and_article. If the row represents forum / article, we skip the row. For all others we continue to the second part of the pipeline which would be like today i.e. we create the new comment type for the current row

As mentioned, I'm still a migration newbie so I'd appreciate some guidance if I'm on the correct / incorrect path with this.

Cheers,
Markus

masipila’s picture

I was thinking this a bit further. What I propose is as follows:

Let's create a new process plugin called skip_if_in_array which takes an array of values as an input parameter. Using this new plugin, we could check for example in comment_type migration if the Row is comment / article and skip these rows.

If you like this approach, I can give this a shot and try to write a patch with this approach.

Cheers,
Markus

masipila’s picture

Assigned: Unassigned » masipila

I managed to write a process plugin skip_row_if_in_blacklist. I'll try to find time to test it tomorrow with these comment types.

Markus

masipila’s picture

Update: I'm almost done with this. I'll still do some manual testing and provide the patch hopefully this evening.

masipila’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
8.23 KB

Alright, here we go finally.

First disclaimer: The patch does not include updates to automated tests. This is a remaining tasks, I've mentioned this also in the issue summary.

The approach has now been changed to what phenaproxima suggested in #18. Instead of hacking and hardcoding the special handling for comment types in the source layer, we handle this now in the process layer. For this, a new process plugin skip_row_if_in_blacklist has been introduced. I wrote this as a generic plugin so that we avoid hard coding and so that this plugin can be re-used for other purposes as well.

 * Example:
 *
 * @code
 *  process:
 *    id:
 *      # Skip the row if 'bundle' is 'foo' or 'bar'.
 *      plugin: skip_row_if_in_blacklist
 *      source: bundle
 *      blacklist:
 *        - foo
 *        - bar
 *      message: "Bundle value is in the blacklist"
 * @endcode
 *
 * If $value is in the blakclist, the row will be skipped and the message
 * "Bundle value is in the blacklist" will be logged in the message table.
 * Otherwise, $value will be returned.

As mentioned above, this process plugin allows us to skip comment_node_forum and comment_node_article in multiple comment / field migrations as we will use the out-of-the-box comment_forum and comment instead. Quite many comment and field migrations were affected by this, see a list below.

Updated comment migrations

The following comment migrations are now skipping comment_node_forum and comment_node_article
d7_comment_type
d7_comment_field
d7_comment_field_instance
d7_comment_entity_display
d7_comment_entity_form_display_subject

Remark, pay attenttion when reviewing.
d7_comment_entity_form_display: This is NOT modified. The migration goes through without errors. Please have a second look at this when reviewing!

Finally, d7_comment: This is mapping to the out-of-the-box comment types for forum and article comments using static_map.

Field migrations

The follwing migrations are now skipping comment_node_forum and comment_node_article
d7_field_instance
d7_field_instance_widget_settings
d7_field_formatter_settings

Remark, pay attention when reviewing.
d7_field: I did not add an explicit skipping for the two comment types as the existing code seems to skip the 2 comment types already when the other migration mofidications listed above are applied. When reviewing this, please consider if we want to explicitly skip these two comment types like we do for the other comment/field migrations.

Offtopic: I need to say that despite the last bugs that we have in the D7-D8 migrations, I'm still more than impressed about the migration API and D7-D8 migrations. Hats off for all individuals who have contributed to this piece of art.

Cheers,
Markus

masipila’s picture

Issue summary: View changes

Screwed up with the comment # in the file name, sorry :/ should have used 24 instead of 22. Time to get some sleep...

Status: Needs review » Needs work
masipila’s picture

I checked the Kernel test failures. I'll fix most of them this evening. There seem to be some bit more complex tests that will probably need input from someone else but the trivial ones I can update.

Markus

masipila’s picture

Setting to Needs review to trigger testbot. One Kernel test has been updated to see the behavior of testbot. No interdiff here, I'll create a second new patch soon after this one after I've seen the testbot results for this one.

Status: Needs review » Needs work
masipila’s picture

Assigned: masipila » Unassigned

Hmm... The one kernel test that I tried to update is still failing, probably because the patch is intentionally skipping the migration of article comments but the test is still checking that it is migrating.

I'm unassigning this from myself now and let others pick this up. I don't want to just comment out the failing tests - somebody with more conceptual understanding of what each migration is doing should review the patch and then update the tests accordingly.

When reviewing, the latest patch is the only one that makes sense to check.

Cheers,
Markus

maxocub’s picture

Assigned: Unassigned » maxocub

Thanks @masipila for working on this. I'll take a look at those failing tests.

maxocub’s picture

Status: Needs work » Needs review
FileSize
20.49 KB
23.18 KB

@masipila: Great work with the skip_row_if_in_blacklist process plugin, but personally I think we can achieve the same thing with only the static_map plugin.
If instead of skipping rows we map the the correct bundles/field_names with static_map, they would not be duplicated after the migration.
Also, if someone would migrate D7 article comments without having installed the D8 standard profile (where the default comment type comes from), the D7 article comments won't migrate since the default comment type would not exists and the migration would have skip them.
So here's a new patch using only static_map.

Status: Needs review » Needs work

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

maxocub’s picture

Status: Needs work » Needs review
FileSize
22.11 KB
1.62 KB

Fix the failing test.

masipila’s picture

@masipila: Great work with the skip_row_if_in_blacklist process plugin, but personally I think we can achieve the same thing with only the static_map plugin.

If instead of skipping rows we map the the correct bundles/field_names with static_map, they would not be duplicated after the migration.

Also, if someone would migrate D7 article comments without having installed the D8 standard profile (where the default comment type comes from), the D7 article comments won't migrate since the default comment type would not exists and the migration would have skip them.

Hi, I didn't test your patch yet but on conceptual level I believe your static_map approach is better than the skip_row_if_in_blacklist approach.

Did you already manually test that the comment/field migrations are actually migrating the D7 settings correctly to D8? The skip_row_if_in_blacklist approach would have simply skipped these and the D8 standard settings would be left as they are out of the box but your patch is probably also taking care of this.

I'll update the issue summary next with the static_map appoach.

Markus

masipila’s picture

Issue summary: View changes
masipila’s picture

Assigned: maxocub » Unassigned

@maxocub, I changed this to unassigned since your patch is now passing the tests and this is now ready for the review of other members of the community.

maxocub’s picture

Issue tags: -Needs tests

@masipila: Yes, I did test it manually, but only with limited content, I'd be curious to see if it works well with a real site with a ton of articles, forums and comments.

@phenaproxima: I think that the existing tests are sufficient, they were already testing the right stuff, but with the wrong names for the article and forum's comment types.

phenaproxima’s picture

This patch raises a lot of questions for me, mostly because comment migration is already incredibly confusing. But this is the main thing that raises my eyebrow:

+++ b/core/modules/comment/migration_templates/d7_comment_type.yml
@@ -8,7 +8,13 @@ source:
+      comment_node_article: comment

Why are we doing this for articles, but all other content types (except for forum) are untouched?

phenaproxima’s picture

I was reviewing this and honestly, I found it incredibly confusing. However! That is 100% my fault, not @masipila's or @maxocub's, or anyone else who worked on this patch.

#2532534: Migration Files for Drupal 7 Comments, which introduced the d7_comment_type migration, is janky and was hacked together in less than a day, and having a comment type for each node is incredibly illogical and makes further work on comments very, very confusing.

So I've opened #2886839: D7 comment type migration should emulate Standard in the hope of clearing that up, and ideally I'd like to postpone this issue on that one. Once that lands, this can be rerolled much, much more simply. In fact, it might not be necessary at all. But in any event, the comment type migration stuff desperately needs to be untangled, and I'd rather fix that underlying problem before we deal with more specific issues, like this one.

phenaproxima’s picture

phenaproxima’s picture

Status: Needs review » Needs work

I think I finally grok the issue, and the general fix (make an exception for the Forum comment type) makes sense to me. However, I have a few problems with the approach taken in the patch:

  1. +++ b/core/modules/comment/migration_templates/d7_comment.yml
    @@ -16,8 +16,20 @@ process:
    +  comment_type:
    +    plugin: static_map
    +    source: comment_type
    +    bypass: true
    +    map:
    +      comment_node_article: comment
    +      comment_node_forum: comment_forum
    

    I think this should be using the migration_lookup process plugin to get the migrated comment type ID from the d7_comment_type migration, rather than static mapping.

  2. +++ b/core/modules/comment/migration_templates/d7_comment.yml
    @@ -16,8 +16,20 @@ process:
    +  field_name:
    +    plugin: static_map
    +    source: comment_type
    +    bypass: true
    +    map:
    +      comment_node_article: comment
    +      comment_node_forum: comment_forum
    

    We can just map this to the migrated comment_type value, since they seem to be identical: field_name: '@comment_type'

  3. +++ b/core/modules/comment/migration_templates/d7_comment_entity_display.yml
    @@ -13,7 +13,13 @@ source:
    +  field_name:
    +    plugin: static_map
    +    source: bundle
    +    bypass: true
    +    map:
    +      comment_node_article: comment
    +      comment_node_forum: comment_forum
    

    As with d7_comment, this should use migration_lookup to get the migrated comment type ID.

  4. +++ b/core/modules/comment/migration_templates/d7_comment_entity_form_display_subject.yml
    @@ -22,7 +22,13 @@ process:
    +  bundle:
    +    plugin: static_map
    +    source: bundle
    +    bypass: true
    +    map:
    +      comment_node_article: comment
    +      comment_node_forum: comment_forum
    

    Same here.

  5. +++ b/core/modules/comment/migration_templates/d7_comment_field.yml
    @@ -9,9 +9,21 @@ source:
    +  field_name:
    +    plugin: static_map
    +    source: bundle
    +    bypass: true
    +    map:
    +      comment_node_article: comment
    +      comment_node_forum: comment_forum
    

    And here.

  6. +++ b/core/modules/comment/migration_templates/d7_comment_field.yml
    @@ -9,9 +9,21 @@ source:
    +  'settings/comment_type':
    +    plugin: static_map
    +    source: bundle
    +    bypass: true
    +    map:
    +      comment_node_article: comment
    +      comment_node_forum: comment_forum
    

    This could just reuse the migrated field_name value: 'settings/comment_type': '@field_name'

  7. +++ b/core/modules/comment/migration_templates/d7_comment_field_instance.yml
    @@ -12,7 +12,13 @@ process:
    +  field_name:
    +    plugin: static_map
    +    source: bundle
    +    bypass: true
    +    map:
    +      comment_node_article: comment
    +      comment_node_forum: comment_forum
    

    This should also use migration_lookup.

  8. +++ b/core/modules/comment/migration_templates/d7_comment_type.yml
    @@ -8,7 +8,13 @@ source:
    +      comment_node_article: comment
    

    I don't understand why we're making a specific exception for articles. IMHO, we should only do the mapping for comment_forum, and everything else should be left untouched.

  9. +++ b/core/modules/field/migration_templates/d7_field_formatter_settings.yml
    @@ -26,7 +26,13 @@ process:
    +      comment_node_article: comment
    

    As per my previous comment, I'd rather we only made an exception for forum posts, but not for any other node type.

  10. +++ b/core/modules/field/migration_templates/d7_field_instance.yml
    @@ -11,7 +11,13 @@ source:
    +      comment_node_article: comment
    

    Ditto.

  11. +++ b/core/modules/field/migration_templates/d7_field_instance_widget_settings.yml
    @@ -26,7 +26,13 @@ process:
    +      comment_node_article: comment
    

    Ditto.

masipila’s picture

Summary of the discussions with phenaproxima today in IRC:

  • The approach suggested in #2886839: D7 comment type migration should emulate Standard is NOT OK. We need to have separate comment types for each node types because the D7 comments might have additional fields in addition to the comment subject, body and author. These additional fields may be different for each node type.
  • Drupal 8 Forum uses the hard coded comment type comment_forum so that needs to be handled separately
  • Drupal 8 Standard has comment type comment for Article contenty type. We concluded that we will use this comment type for Articles. This might lead to a WTF moment when using other installation profiles than Standard (i.e. all other node types use comment types comment_node_foo but article is using comment. The functioanlity should be OK, this might just be confusing for the site builder. However, the conclusion in our IRC discussion was that this is smaller evil than trying to add even more complexity to the migrations like use comment_node_article if we didn't have comment but use comment if it exists. The latest patch #34 should work already now like this.
  • d7_comment_entity_form_display needs work: the field_name needs to be updated
  • d7_comment_field_instance and d7_field_instance seem to be doing almost the same thing. To be evaluated what do we want to do with this.
phenaproxima’s picture

+1 what @masipila said in #43. There is basically no elegant way to do this properly, so the approach in #34 is what we will do. However, we acknowledge that it's a confusing workaround, so we're going to thoroughly document it by adding comments to all of the migrations that are affected by this change.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
21.95 KB
4.18 KB

This addresses my feedback from #42 except for points 8-11, as per @masipila's comment in #43. It doesn't add documentation, but I'll do that once this passes all tests.

masipila’s picture

Hi,

I can do manual testing for this hopefully later this evening.

Did you have a chance to check if we need / if we want to have almost identical migrations d7_comment_field_instance and d7_field_instance? If I remember correctly by heart, they seemed to do the same thing except they had one difference which was related to translatability. I think the question is that do we need d7_comment_field_instance migration at all?

Cheers,
Markus

phenaproxima’s picture

Added a bit of documentation. All opinions welcome! I'd like it to be crystal-clear, since this is such a confusing and thorny issue.

maxocub’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldInstanceTest.php
    @@ -56,14 +56,14 @@ protected function setUp() {
        * @param string $id
        *   The node type ID.
        */
    -  protected function createType($id) {
    +  protected function createType($node_type, $comment_type) {
    

    I forgot to change the doc block here to account for the new arguments.

  2. +++ b/core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserTest.php
    @@ -67,14 +67,14 @@ protected function setUp() {
        * @param string $id
        *   The node type ID.
        */
    ...
    +  protected function createType($node_type, $comment_type) {
    

    Same here.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
23.97 KB
1.48 KB

Fixed!

phenaproxima’s picture

masipila’s picture

Hi,

I have the latest D8.4.x from git + patch #49 applied cleanly.

Unfortunately the test results are not OK. Results from the selected migrations below. Pay attenttion to the following:

  • upgrade_d7_comment_field
  • upgrade_d7_comment_field_instance
  • upgrade_d7_comment_entity_display
  • upgrade_d7_comment_entity_form_display_subject
  • upgrade_d7_field_instance_widget_settings
  • upgrade_d7_comment
drush9 mi upgrade_d7_node_type
 [status] Processed 16 items (16 created, 0 updated, 0 failed, 0 ignored) - done with 'upgrade_d7_node_type'

drush9 mi upgrade_d7_comment_type
 [status] Processed 16 items (16 created, 0 updated, 0 failed, 0 ignored) - done with 'upgrade_d7_comment_type'

 drush9 mi upgrade_d7_comment_field
 [status] Processed 16 items (0 created, 0 updated, 0 failed, 16 ignored) - done with 'upgrade_d7_comment_field'

drush9 mi upgrade_d7_comment_field_instance
 [status] Processed 16 items (0 created, 0 updated, 0 failed, 16 ignored) - done with 'upgrade_d7_comment_field_instance'

drush9 mi upgrade_d7_comment_entity_display
 [status] Processed 16 items (0 created, 0 updated, 0 failed, 16 ignored) - done with 'upgrade_d7_comment_entity_display'

 drush9 mi upgrade_d7_comment_entity_form_display
 [status] Processed 16 items (16 created, 0 updated, 0 failed, 0 ignored) - done with 'upgrade_d7_comment_entity_form_display'

drush9 mi upgrade_d7_comment_entity_form_display_subject
 [status] Processed 16 items (0 created, 0 updated, 0 failed, 16 ignored) - done with 'upgrade_d7_comment_entity_form_display_subject'

drush9 mi upgrade_d7_field
 [status] Processed 77 items (76 created, 0 updated, 0 failed, 1 ignored) - done with 'upgrade_d7_field'

drush9 mi upgrade_d7_field_instance
[status] Processed 140 items (137 created, 0 updated, 3 failed, 0 ignored) - done with 'upgrade_d7_field_instance'
Note: the three failed once are not related to this

drush9 mi upgrade_d7_view_modes
 [status] Processed 8 items (8 created, 0 updated, 0 failed, 0 ignored) - done with 'upgrade_d7_view_modes'

drush9 mi upgrade_d7_field_instance_widget_settings
 [status] Processed 142 items (0 created, 0 updated, 142 failed, 0 ignored) - done with 'upgrade_d7_field_instance_widget_settings'

drush9 mi upgrade_d7_taxonomy_vocabulary
 [status] Processed 6 items (6 created, 0 updated, 0 failed, 0 ignored) - done with 'upgrade_d7_taxonomy_vocabulary'

drush9 mi upgrade_d7_taxonomy_term_vocabulary_1
 [status] Processed 7 items (7 created, 0 updated, 0 failed, 0 ignored) - done with 'upgrade_d7_taxonomy_term_vocabulary_1'

All nodes migrated succesfully.

drush9 mi upgrade_d7_comment
 [status] Processed 3624 items (0 created, 0 updated, 0 failed, 3624 ignored) - done with 'upgrade_d7_comment'

masipila’s picture

Status: Needs review » Needs work
phenaproxima’s picture

Issue tags: +Needs tests

The failures most likely have to do with the use of migration_lookup (as I requested), which probably failed and caused the rows to be skipped/ignored. I'll have to fix this and expand the test coverage to catch those cases.