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:

  • 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:

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:
  • D8 Standard installation profile creates content type Article which ships with 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 (note that this only applies to D8 Standard installation profile). When comments are migrated, the comment_entity_statistics table gets incorrectly populated.
  • See attached screenshot from forum field settings:
  • D8 Forum module uses comment type comment_forum. This comment type is created when D8 Forum module is installed. D7-D8 migration adds also comment type comment_node_forum. As a result, the content type has now two comment types. When comments are migrated, the comment_entity_statistics table gets incorrectly populated as illustrated above in the earlier screenshot.

Proposed resolution

Proposed resolution for Forum

  • Do not create comment type comment_node_forum for Forum because Forum uses hard coded comment type comment_forum. Several comment / field migrations need to be updated.
  • Use static_map process plugin to map the D7 comment / field migrations to the D8 standard comment_forum

Proposed resolution for Article

  • D8 Standard is the only installation profile that creates the comment type comment for Article. In other words, it is not guaranteed that D8 destination already has comment type comment for Article.
  • Conclusion in #68-71 is that migration will migrate Article comments to comment_node_article and site builder needs to manually delete the comment field and comment type comment if the D8 site was installed using D8 Standard installation profile.
  • Regarding the deletion of the comment field: there is another bug related to it, see #2906470: Orphan comments and entries in comment_entity_statistics after comment field instance has been deleted. However, #2906470 is not a blocker for this migration issue. Before #2906470 lands, the site builder can avoid all problems by manually deleting the comment comment field and comment type before running the migrations.

Remaining tasks

  • write patch
  • review it

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#114 interdiff-2853872-83-114.txt4.58 KBmaxocub
#114 2853872-114.patch21.88 KBmaxocub
#90 2853872-90-comment_entity_statistics.PNG29.13 KBmasipila
#86 2853872-86-migration-map.PNG44.75 KBmasipila
#83 interdiff_79-83.txt1.09 KBheddn
#83 2853872-83.patch21.62 KBheddn
#79 interdiff_77-79.txt1.89 KBheddn
#79 2853872-79.patch21.62 KBheddn
#77 2853872-77.patch21.39 KBheddn
#77 interdiff_75-77.txt717 bytesheddn
#75 2853872-75.patch21.39 KBheddn
#75 interdiff_74-75.txt5.52 KBheddn
#74 interdiff_58-74.txt12.98 KBheddn
#74 2853872-74.patch19.8 KBheddn
#58 2853872-58.patch23.68 KBmaxocub
#49 interdiff-2853872-47-49.txt1.48 KBphenaproxima
#49 2853872-49.patch23.97 KBphenaproxima
#47 interdiff-2853872-45-47.txt3.18 KBphenaproxima
#47 2853872-47.patch23.41 KBphenaproxima
#45 interdiff-2853872-34-45.txt4.18 KBphenaproxima
#45 2853872-45.patch21.95 KBphenaproxima
#34 interdiff-32-34.txt1.62 KBmaxocub
#34 2853872-34.patch22.11 KBmaxocub
#32 interdiff-28-32.txt23.18 KBmaxocub
#32 2853872-32.patch20.49 KBmaxocub
#28 2853872-article_forum_comment_type_migration-28.patch9.42 KBmasipila
#24 2853872-article_forum_comment_type_migration-22.patch8.23 KBmasipila
#10 2853872-article_forum_comment_type_migration-10.patch4.5 KBmasipila
#8 2853872-duplicate-comment-types-forum.png31.3 KBmasipila
#8 2853872-duplicate-comment-types-article.png22.35 KBmasipila
#4 comment_entity_statistics.PNG25.18 KBmasipila
d8-forum-migrate.PNG33.59 KBmasipila
Support from Acquia helps fund testing for Drupal Acquia logo

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: D6 Forum vocabulary is migrated to the wrong D8 field name, 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.

heddn’s picture

maxocub’s picture

Status: Needs work » Postponed

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

maxocub’s picture

Status: Postponed » Needs work

I don't know why I postponed this issue on #2887142: NodeType source plugin should include comment information, this seems unrelated.
We need to address #51 and add tests.

maxocub’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
23.68 KB

This needed a re-roll, so here it is.

@masipila: I don't understand why it's not working for you. I did test it with a test site and everything was migrated. Also, I find the test coverage to be adequate. Could you provide more information about the context of the failures you mention in #51? Thanks!

masipila’s picture

Hi, I'll try to find time to re-test my migration with a clean 8.5.x-dev install and patch #58. I'm quite busy for the rest of the week but I'll try to find time to do this either this evening (Finnish time) or tomorrow.

Cheers,
Markus

masipila’s picture

I did a full manual re-test with D8.5.x-dev with path #58 applied. Patch #58 seems to be OK with only one remark. All forum threads are closed after the migration. Unortunately I don't have time to test this without the patch so I propose that somebody gives this a second pair of eyeballs to check if this is related to patch #58 or not.

Testing notes.

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'

Manual inspection in the UI. Result: OK. We have comment, comment_forum. We don't have comment_node_article, we don't have comment_node_forum. For all other content types we have comment_node_foo.

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

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

Manual inspection in the UI. Result: OK. Article has a comment field comment, forum has comment_forum and those content types which had comments on D7, have now comment_node_foo

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

drush9 mi upgrade_d7_field_instance
 [error]  Attempt to create a field field_related_leagues that does not exist on entity type node. (path/to/core/modules/field/src/Entity/FieldConfig.php:286)
 [error]  Attempt to create a field field_clubs that does not exist on entity type node. (path/to/core/modules/field/src/Entity/FieldConfig.php:286)
 [warning] array_filter() expects parameter 1 to be array, null given FieldInstanceSettings.php:34
 [warning] array_filter() expects parameter 1 to be array, null given FieldInstanceSettings.php:34
 
 [status] Processed 141 items (139 created, 0 updated, 2 failed, 0 ignored) - done with 'upgrade_d7_field_instance'

The two failed ones are related to my custom module on D7. So these are OK.

drush9 mi upgrade_d7_field_instance_widget_settings
 [error]  The "select" plugin does not exist. (path/to/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php:52)
 [status] Processed 141 items (138 created, 0 updated, 1 failed, 2 ignored) - done with 'upgrade_d7_field_instance_widget_settings'

The error about the select plugin seems unrelated.

Migrated all users, taxonomy vocabularies and nodes. Had to manually tweak the filter formats but that is unrelated to this issue.

Then, finally

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

Had to manually tweak the filter formats for comment body but that is unrelated to this issue.

The comment statistics are now OK in the Forums. Yay.

Now some sleep...

Cheers,
Markus

masipila’s picture

Oh, forgot to comment about the problems I reported in #51. I tried to be very systematic both in #51 and #60 but I can't explain what went wrong in #51. The migrations seem to work now (except the closed forum threads is suspectious) so I can't figure out anything else then some human error on my side when testing #51.

Summa summarum:
@maxocub did test #58 manually and so did I but @maxocub is the key contributor of the patch so I don't think his test result count here. To me, everything seems to be OK except the closed forum threads. Having said this, I'm quite tired at the moment so I would highly recommend that somebody will give patch #58 another full manual test review before this will be RTBC.

Cheers,
Markus

maxocub’s picture

Thanks a lot @masipila for taking time to test this again.

About the closed forum threads, there is already an issue open about this: #2887043: D7 comment field instance migration: incorrect mapping for open/closed/hidden default value.
This problem was not apparent before applying the patch from #58 because the forums were still using the comment type provided by the forum module, not the migrated comment type. I think we can ignore this problem and deal with it in the other issue.

masipila’s picture

About the closed forum threads, there is already an issue open about this: #2887043: D7 comment field instance migration: incorrect mapping for open/closed/hidden default value

This problem was not apparent before applying the patch from #58 because the forums were still using the comment type provided by the forum module, not the migrated comment type. I think we can ignore this problem and deal with it in the other issue.

Hmm... I don't completely get this. The scope of #2887043: D7 comment field instance migration: incorrect mapping for open/closed/hidden default value is that the default value of open/closed/hidden was migrated incorrectly for a given content / comment type.

The problem that I was wondering here was the open/closed/hidden status of an individual forum node. This should have nothing to do with #2887043: D7 comment field instance migration: incorrect mapping for open/closed/hidden default value.

However, thinking this further, it should have nothing to do with with the issue that we are trying to fix here either because the open/closed/hidden status is a property of the node and not a comment of a node. Which suggests that patch #58 did not break this but something else did. Which suggests that we should create a separate issue for that one and investigate and fix it separately.

I'll create a separate issue for that.

Cheers,
Markus

masipila’s picture

heddn’s picture

Issue tags: +Migrate Drupal

Inventing a tag to mark this as pertaining to migrate_drupal and therefore blocking its stable release.

heddn’s picture

heddn’s picture

Issue summary: View changes

Updated IS.

heddn’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/comment/migration_templates/d7_comment_type.yml
    @@ -8,7 +8,21 @@ source:
    +    # If the destination site installed the Standard profile, there will
    +    # already be a 'comment' comment type, and the Article node type will be
    +    # using it. To avoid data integrity issues, we want to reuse that comment
    +    # type and the associated field. If the destination site did NOT install
    +    # Standard, this will create the 'comment' comment type. The Forum module,
    +    # meanwhile, also provides its own comment type (comment_forum), which we
    +    # want to reuse if it exists, for the same reason that we want to reuse
    +    # the 'comment' comment type if it exists.
    

    Until now, we've assumed that the destination site in D8 was vacant. This is the first (that I'm aware of) case where we try to work around any install profile naming of fields. I hate to start doing that now. But the process plugin shouldn't know anything about the destination either, so I don't think we have much option. We should just migrate, without concern of what the D8 site already has built in any install profile.

    Can we migrate to the semi-standard migration location for nodes (barring forum, which adds its field in the module install, not an install profile)? We do in all other cases. If someone is migrating from d6 and has an article node type there that has the body field removed and a field_description added, we'd result in a duplicate "body-like" field on articles. This isn't any different.

    What data integrity issues do we create if we have two comment fields on the article content type? Can someone speak to that? Beside the oddity of it, do we actually break/loose data?

  2. +++ b/core/modules/field/migration_templates/d7_field_formatter_settings.yml
    @@ -26,7 +26,17 @@ process:
    +  # core/modules/comment/migration_templates/d7_comment_type.yml
    
    +++ b/core/modules/field/migration_templates/d7_field_instance.yml
    @@ -11,7 +11,17 @@ source:
    +  # core/modules/comment/migration_templates/d7_comment_type.yml
    
    +++ b/core/modules/field/migration_templates/d7_field_instance_widget_settings.yml
    @@ -26,7 +26,17 @@ process:
    +  # core/modules/comment/migration_templates/d7_comment_type.yml
    

    Let's just use the filename instead of the full path.

  3. +++ b/core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldInstanceTest.php
    @@ -40,12 +38,12 @@ class MigrateFieldInstanceTest extends MigrateDrupal7TestBase {
    +    $this->createType('page', 'comment_node_page');
    
    @@ -53,17 +51,19 @@ protected function setUp() {
    +  protected function createType($node_type, $comment_type) {
    

    Can we move this to a trait? Seems generally useful enough. And the default for the comment name should then be comment_node_ . $node_type, unless $comment_type is non-null.

masipila’s picture

@heddn, regarding this:

What data integrity issues do we create if we have two comment fields on the article content type? Can someone speak to that? Beside the oddity of it, do we actually break/loose data?

If I remember correctly by heart from the previous investigations I had with comment_entity_statistics, we had duplicate entries there when Article had the two comment fields / types present. This lead to duplicate results in views that were using data originated from comment_entity_statistics.

I believe (without testing this) that these duplicate views result issue could be resolved by deleting the comment commenti field / type from the Article (assuming that Migrate populated the statistics to comment_node_article, which I don't remember by heart).

Other than that, I don't see any other breaks / data losses.

I believe we have a clear consensus that Forum must have comment_forum because that has been hard coded to Forum module?

Markus

heddn’s picture

Yes, no problem with forum. We've heard from the forum/comment maintainer it won't be changing. The issue we need to decide about is what to do with article, which comes from standard install profile. I'd like to propose we assume the destination is empty and if it isn't then user will have to delete the comment field from article to fix things.

heddn’s picture

Priority: Normal » Major

Reviewing in weekly migrate maintainers meeting. This should be marked as a major at least. Still looking to gain some insights around #68.1.

We discussed in the meeting and we would like to not have one-off logic for articles since it comes from an install profile, not the node module. As long as deleting the duplicate comment field fixes the statistics bug that started this issue.

masipila’s picture

@heddn, your proposed approach in #68-71 affects #2902766: Node "comment open / closed" status not migrated. Please see my comment https://www.drupal.org/node/2902766#comment-12243133

Short version: if we choose the approach I proposed in my comment linked above, I don't see unrecoverable data losses / breaks with your approach #68-71 here. We need to clearly document that site builders using D8 need to manually delete the extra comment field, though.

Cheers,
Markus

heddn’s picture

re #72: we can mention the issue with comment fields in https://www.drupal.org/docs/8/upgrade/known-issues-when-upgrading-from-d....

heddn’s picture

heddn’s picture

FileSize
5.52 KB
21.39 KB

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

heddn’s picture

FileSize
717 bytes
21.39 KB

Re #74 failure. Counts increased because we don't re-use the article node's comment type. Fixed here.

The last submitted patch, 75: 2853872-75.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

heddn’s picture

FileSize
21.62 KB
1.89 KB

Fixes code style issues caught by CI.

The last submitted patch, 77: 2853872-77.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

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

dillix’s picture

@heddn, is there a chance that we see a fix for this issue at 8.4?

heddn’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Needs work » Needs review
FileSize
21.62 KB
1.09 KB

Since migrate is still experimental, changes will almost always get backported to 8.4. In the case of this, it is a bug and should make its way into 8.4.

And now for those counts. It came back green locally. Let's see if it does on testbot too.

masipila’s picture

I was testing patch #83 but run into the same weird behavior that I encountered in #51.

$ drush9 mi upgrade_d7_node_type
 [status] Processed 5 items (5 created, 0 updated, 0 failed, 0 ignored) - done with 'upgrade_d7_node_type'
$ drush9 mi upgrade_d7_comment_type
 [status] Processed 5 items (5 created, 0 updated, 0 failed, 0 ignored) - done with 'upgrade_d7_comment_type'
$ drush9 mi upgrade_d7_comment_field
 [status] Processed 5 items (0 created, 0 updated, 0 failed, 5 ignored) - done with 'upgrade_d7_comment_field'
$ drush9 mi upgrade_d7_comment_field_instance
 [status] Processed 5 items (0 created, 0 updated, 0 failed, 5 ignored) - done with 'upgrade_d7_comment_field_instance'

For some reason the comment fields and comment field instances are ignored. I'll start to debug this and will post my findings here.

Markus

masipila’s picture

Status: Needs review » Needs work
masipila’s picture

FileSize
44.75 KB

The weird behavior here seems to be that migration_lookup process plugin is fragile.

About my test site:
1. Latest D8.5.x installed with git
2. Patch 83 from this issue applied. Should not make a difference since this patch is not changing the migration_lookup process plugin.

Migration steps performed:

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

Observation: We can see that all 5 comment fields were ignored when upgrade_d7_comment_field was run.

Looking at the migration process, we have this:

process:
  entity_type: constants/entity_type
  field_name:
    -
      plugin: migration_lookup
      source: bundle
      migration: d7_comment_type
    -
      plugin: skip_on_empty
      method: row

Observation: The skip_on_empty will cause the row to be skipped if migration_lookup does not return a value.

Debug step: I added a debug message to SkipOnEmpty process plugin as follows:

  public function row($value, MigrateExecutableInterface $migrate_executable, Row $row, $destination_property) {
    if (!$value) {
      drush_print("value is empty, skipping the row");
      $message = !empty($this->configuration['message']) ? $this->configuration['message'] : '';
      throw new MigrateSkipRowException($message);
    }
    return $value;
  }

Executed the upgrade_d7_comment_field migration again with Drush:

$ drush9 mi upgrade_d7_comment_field
value is empty, skipping the row
value is empty, skipping the row
value is empty, skipping the row
value is empty, skipping the row
value is empty, skipping the row
 [status] Processed 5 items (0 created, 0 updated, 0 failed, 5 ignored) - done with 'upgrade_d7_comment_field'

Indeed, the migration_lookup is passing an empty value to the skip_on_empty plugin in the migration pipeline.

Looking at the comment types in the UI at admin/structure/comment: I can see that the 5 comment types are present. So upgrade_d7_comment_type migration did it's job just like the migration output said earlier. I'm referring to this:

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

Let's look at the migration map in the database next, table migrate_map_upgrade_d7_comment_type

Observation: the mapping is present in the mapping table:

This starts to suggest that we have some weird stuff going on with the migration_lookup process plugin. Let's debug that next.

Let's add a debug info to the end of MigrationLookup::transform().

    drush_print("we have not exited the method yet");
    if ($destination_ids) {
      if (count($destination_ids) == 1) {
        return reset($destination_ids);
      }
      else {
        return $destination_ids;
      }
    }
    drush_print("Seems that we reach this point and don't return anything.");
  }

Let's run the migration again:

$ drush9 mi upgrade_d7_comment_field
we have not exited the method yet
Seems that we reach this point and don't return anything.
value is empty, skipping the row
we have not exited the method yet
Seems that we reach this point and don't return anything.
value is empty, skipping the row
we have not exited the method yet
Seems that we reach this point and don't return anything.
value is empty, skipping the row
we have not exited the method yet
Seems that we reach this point and don't return anything.
value is empty, skipping the row
we have not exited the method yet
Seems that we reach this point and don't return anything.
value is empty, skipping the row
 [status] Processed 5 items (0 created, 0 updated, 0 failed, 5 ignored) - done with 'upgrade_d7_comment_field'

Conclusion: we have some underlying bug in migration_lookup process plugin which is interfering here.

Proposed next steps: I'll open a follow-up issue for migration lookup being fragile.

Cheers,
Markus

masipila’s picture

Status: Needs work » Needs review

The issue with #84-86 was that Migrate Upgrade 8.x-3.0-rc1 does not work properly with migrate_lookup process plugin. This has been fixed as #2875405: Rewrite references in 'migration_lookup' plugins as well as 'migration' but that is only in Migrate Upgrade dev.

masipila’s picture

Status: Needs review » Needs work

Ngggh...

Either I screwed up my test environment when messing up with the #84-87 or the patch #83 needs to be re-rolled.

$ git pull origin 8.5.x
From https://git.drupal.org/project/drupal
 * branch            8.5.x      -> FETCH_HEAD
Already up-to-date.
$ git apply -v 2853872-83.patch
Checking patch core/modules/comment/migration_templates/d7_comment.yml...
Checking patch core/modules/comment/migration_templates/d7_comment_entity_display.yml...
Checking patch core/modules/comment/migration_templates/d7_comment_entity_form_display_subject.yml...
Checking patch core/modules/comment/migration_templates/d7_comment_field.yml...
Checking patch core/modules/comment/migration_templates/d7_comment_field_instance.yml...
Checking patch core/modules/comment/migration_templates/d7_comment_type.yml...
Checking patch core/modules/comment/tests/src/Kernel/Migrate/d7/MigrateCommentEntityDisplayTest.php...
Checking patch core/modules/comment/tests/src/Kernel/Migrate/d7/MigrateCommentEntityFormDisplaySubjectTest.php...
Checking patch core/modules/comment/tests/src/Kernel/Migrate/d7/MigrateCommentFieldInstanceTest.php...
Checking patch core/modules/comment/tests/src/Kernel/Migrate/d7/MigrateCommentFieldTest.php...
Checking patch core/modules/comment/tests/src/Kernel/Migrate/d7/MigrateCommentTypeTest.php...
Checking patch core/modules/field/migration_templates/d7_field_formatter_settings.yml...
Checking patch core/modules/field/migration_templates/d7_field_instance.yml...
Checking patch core/modules/field/migration_templates/d7_field_instance_widget_settings.yml...
Checking patch core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldFormatterSettingsTest.php...
Checking patch core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldInstanceTest.php...
Checking patch core/modules/field/tests/src/Kernel/Migrate/d7/RollbackFieldInstanceTest.php...
Checking patch core/modules/migrate/tests/src/Kernel/NodeCommentComboTrait.php...
error: core/modules/migrate/tests/src/Kernel/NodeCommentComboTrait.php: already exists in working directory
Checking patch core/modules/migrate_drupal_ui/tests/src/Functional/d7/MigrateUpgrade7Test.php...
Checking patch core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserTest.php...
heddn’s picture

Status: Needs work » Needs review

New release provided for migrate_upgrade. And the patch still applies cleanly against 8.4.x & 8.5.x.

masipila’s picture

Status: Needs review » Needs work
FileSize
29.13 KB

I set up my test environment from scratch and was now able to test patch #83. We're almost there, but...

1. Handling for Forum is correct now

  • Forum content type has only one comment field, comment_forum, as expected
  • Forum comments are migrated to comment_forum as expected
  • comment_entity_statistics are correct for Forum

2. Unfortunately the suggested approach from #71 for Article is not ok.
The proposed approach is this:

  • We let migrations create comment_node_article in addition to comment which came from D8 Standard installation profile.
  • Migrations will migrate all Article comments to comment_node_article.
  • And we would rely that the site builder will then manually delete the comment comment field from Article + delete the whole comment type comment.

I did this, but unfortunately the duplicate entries in comment_entity_statistics are not removed when comment comment field / type is deleted. See the screenshot below.

I see four possible approaches to this:
a) We return back to the drawing board and use the special handling for Article comments.
b) We instruct the site builders to remove the D8 Standard comment field *before* they start the migrations. I don't think that too many site builders will notice this instruction, though.
c) If s#it is already in the fan when the site builder notices the duplicate entries in comment_entity_statistics, then I guess it *most probably* is safe to delete the entries from comment_entity_statistics where field_name = comment. This is too dangerous instruction to write IMO because the site builder might have re-used the standard comment comment type by the time he/she realizes the issue.
d) D8 Core would be changed so that when comment field is removed from a content type, also associated comment_entity_statistics would be deleted.

3. Disclaimer: I did not test / pay attention to migrations d7_comment_entity_display, d7_comment_entity_form_display, d7_comment_entity_form_display_subject, d7_view_modes

Cheers,
Markus

Edit: added 2d, 3.

dillix’s picture

@masipila can we make patch that will do 2d automatically?

masipila’s picture

@dillix, I think that 2d is the way to go but I'm tempted to say that the behavior I tested and described in my previous comment is a bug in Core Comment module that should be fixed regardless of this migration issue.

Consider the following:
1. Content type has field foo. Nodes exist and we have content in this field.
2. Field foo is deleted from the content type.

I didn't test yet but I'm quite sure that all content of field foo from this instance will be deleted i.e. we don't leave orphan entries to the database. If this is correct as general Core behavior, then it should be the responsibility of the Comment module to clean up its data from the database, including related entries from comment_entity_statistics.

I'll test this later today but any thoughts to this anyone?

Markus

dillix’s picture

I think you are correct. But related issue #2874487: Hardcoded field names in Core should be assigned/returned dynamically addressed to 8.5, may be we should change that to 8.4?

masipila’s picture

I don't see how #2874487: Hardcoded field names in Core should be assigned/returned dynamically would be needed in order to get this issue resolved. According to heddn, the Forum module maintainers had informed that Forum is not goint to get rid of hard coded comment_forum. Patch #83 already handles comment_forum properly. The only remaining headache from migration poijt of view is the Article and D8 Standard comment comment type. If we can get the 2d approach I suggested, then we're done.

Markus

masipila’s picture

heddn’s picture

Status: Needs work » Needs review

I don't see #2906470: Orphan comments and entries in comment_entity_statistics after comment field instance has been deleted being a hard-blocker. Maybe a soft one. Let's add to https://www.drupal.org/docs/8/upgrade/known-issues-when-upgrading-from-d... what we know about comments (right now) and link the issue that is planning to fix things.

Putting back to NR.

masipila’s picture

I agree that #2906470: Orphan comments and entries in comment_entity_statistics after comment field instance has been deleted is only a soft blocker. Especially now that it became crystal clear there that the current core behavior upon comment field instance deletion is a bug and not a feature.

Then about this issue and remaining tasks. As I mentioned in #90, I did not test all the migrations that this patch changed as I don't even understand all the things that those migrations are supposed to migrate.

What my manual test covered was d7_node_type, d7_comment_type, d7_comment_field, d7_comment_field_instance, d7_field, d7_field_instance, d7_node_{node_type}, d7_comment.

The automated test coverage for the remaining affected migrations seems to be good but it would not hurt if somebody could give still another pair of eyeballs for them.

Cheers,
Markus

Markus

sbogner@insightcp.com’s picture

I'm upgrading a D6 forum to D8 (8.4 specifically) - are there plans to make this correction apply also to D6 migrations?

heddn’s picture

This is the first report of an issue on D6. Can you elaborate if does effect d6 as well? The solution would need to be duplicated for d6.

sbogner@insightcp.com’s picture

Yes, I'm seeing the exact same symptoms in my D6 site when I do the forum migration.

heddn’s picture

Status: Needs review » Needs work

Back to NW to pick up D6.

andypost’s picture

IMO mapping is a right way for standard profile provided types, I think forum case should also be covered with mapping

+++ b/core/modules/comment/migration_templates/d7_comment_type.yml
@@ -8,7 +8,22 @@ source:
+    map:
+      comment_node_article: comment
+      comment_node_forum: comment_forum

I like that idea!

maxocub’s picture

The problem when upgrading from D6 is that, right now, it's only creating two comment types, however how many node types there is: comment & comment_no_subject.
Before we can implement the solution from this issue for D6, we need to fix the D6 migration of comment types, and that's what we're doing in #2887142: NodeType source plugin should include comment information.

+++ b/core/modules/comment/migration_templates/d7_comment_type.yml
@@ -8,7 +8,22 @@ source:
+    map:
+      comment_node_article: comment
+      comment_node_forum: comment_forum

I like that idea!

I also like that idea, but I think that @heddn and @masipila agreed that we shouldn't add mapping for the standard profile.

masipila’s picture

Re #102-103:

The original approach of this issue was that the comment type for Article nodes would be the D8 Standard comment type called comment. I.e. this:

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

However, heddn and other maintainers of the migrate concluded that we don't want to do installation profile specific migrations.

So we have two possible approaches for the migration of Article comments:

  • Create a comment type comment_node_article. This means that D8 Standard site builders must delete the obsolete comment field comment.
  • Create a comment type comment for Article if it didn't exist already and migrate the Article comments to this comment field.

There is at least one other comment migration related issue that are depending on the decision here. I'm referring to #2902766: Node "comment open / closed" status not migrated. Currently that issue assumes that we use comment_node_article. If we choose to use comment, then that new approach needs to be reflected in #2902766.

I personally don't have a strong opinion to either directions. I do understand the point from Migrate maintainers (see #71) that we would like to avoid installation profile specific approaches in migrations.

Proposed next steps: I would propose that @heddn and @andypost will agree on the approach so that we have a clear guidance on the approach for this spiderweb.

Markus

Morbus Iff’s picture

Throwing in another "has problems on D6" - tried a migration this morning.

I'd personally prefer "comment" myself, too. If I'm having to start a D8 site "from scratch" to perform a migration, than I want the migration to migrate me as "D8-y default" as possible. I don't want to start in D8 with weirdness already ("comment_node_article" vs. "comment"), and no real ability to unweird it. I don't want half my "article" content using one Comment type (via the migration) and then any other content type I create using the default "comment" (I also wouldn't want to set new content types to use "comment_node_article", to standardize on one content type, cos it's clearly namespaced to article, even if that wasn't my choice),

heddn’s picture

Article and Forum are the weird ones here. If you add a comment manually to page or blog, or migrate content over from a page or blog, then they are going to use comment_node_page and comment_node_blog.

Morbus Iff’s picture

Gotcha. In which case, due to prior art, I support comment-node-article. Blame ignorance.

heddn’s picture

Issue summary: View changes
Status: Needs work » Needs review

Rather than slow down this patch, let's move the D6 stuff into a follow: #2909752: [D6] Migration for comments: duplicate comment types and incorrect comment_entity_statistics. That means this needs review.

andypost’s picture

I think it depends on #2887142: NodeType source plugin should include comment information

About naming - maybe file new issue and discus about how machine names should migrate?
It's like naming for content type - d6 "story" migrate to d7/8 "article"

heddn’s picture

larowlan’s picture

One code comment:

+++ b/core/modules/migrate/tests/src/Kernel/NodeCommentComboTrait.php
@@ -0,0 +1,37 @@
+trait NodeCommentComboTrait {
...
+  protected function createNodeCommentCombo($node_type, $comment_type = NULL) {

As an Australian I endorse the shortening of Combination to Combo, but I think our standards in core would dictate that we should use the full word. So I think this needs to be Combination instead of Combo

And one question:
I don't understand what this patch does to resolve comment_node_article - it may be that it does nothing and we're going to advise people to delete the comment field from article if they migrated - is that the case? If so, then I understand, if not, then I'm not qualified to review this (migrate isn't my speciality).

I note the issue summary still says

Use static_map process plugin to map the D7 comment / field migrations to the D8 standard comment_forum and comment comment types

But I don't see where in the patch we're mapping to comment - so perhaps the IS needs an update? Tagging for that.

masipila’s picture

Issue summary: View changes

Thanks @larowlan!

Issue summary updated.

Cheers,
Markus

masipila’s picture

maxocub’s picture

This also needed a re-roll, so here it is with the Combo -> Combination change.

heddn’s picture

Preferably #2887142: NodeType source plugin should include comment information lands before this one. If we can, let's get that one committed first.

phenaproxima’s picture

Assigned: Unassigned » phenaproxima

Assigning for review.

xjm’s picture

Priority: Major » Critical

Given the importance of Migrate to the Drupal 8 ecosystem at this point, @catch and I agreed to promote the "Migrate critical" issues to critical priority.

quietone’s picture

Status: Needs review » Needs work

I have through the issue and looks like the task is done and everyone agrees with the solution in the IS. What I would like to see is this documented on the Known issues when upgrading from Drupal 6 or 7 to Drupal 8 before this is committed.

What I have not done is review the code, that will have to be later.

Setting to NW for the doc page, just to ensure we document this.

phenaproxima’s picture

Assigned: phenaproxima » Unassigned

Un-assigning from myself, since @quietone kicked it back.

masipila’s picture

Assigned: Unassigned » masipila

I'll do the documentation update.

masipila’s picture

Assigned: masipila » Unassigned
Status: Needs work » Needs review

Known issues docs updated:
https://www.drupal.org/docs/8/upgrade/known-issues-when-upgrading-from-d...

According to #108, the D6-D8 part will be handled as a separate follow-up issue. I'm not 100% familiar with the D6-D8 status so I didn't update the Known Issues for D6-D8 part. But that is anyway out of scope of this issue.

Back to Needs Review.

maxocub’s picture

Issue tags: +Vienna2017

Adding a tag so we can see this issue on our Vienna migrate sprint kanban.

quietone’s picture

Status: Needs review » Needs work

@masipila, thanks. That documentation is very thorough and clear! I made one small change, due to an extra 'do' in a sentence. On reading the whole thing, the only concern I have is that is isn't clear if the comment field on Article should be removed for all migrations with Article content type of for only migrations using Forum that have Article content type. Does it matter? Either way, I think this should be made clear what situations the recommendation is addressing.

masipila’s picture

On reading the whole thing, the only concern I have is that is isn't clear if the comment field on Article should be removed for all migrations with Article content type of for only migrations using Forum that have Article content type. Does it matter? Either way, I think this should be made clear what situations the recommendation is addressing.

I don't completely understand this question so you'll get a bit longer answer.

1. For Forum, no special actions are needed from the site builder after this issue has been committed. The only thing that need to be understood is that we are *not* creating comment_node_forum like we are doing for all other content types. Reason being that Forum uses a hard coded comment type so we are migrating the comments to that comment type.

2. The Article topic has nothing to do with Forum.

The migrations are creating comment type comment_node_article and migrating all Article comments to this comment field. This works exactly like for all other content types (except Forum which has the special handling described above).

The thing here is that if D8 site was installed using Standard installation profile, then Article also has another comment field called comment which was there out of the box. If the site builder does not want to have two comment fields for Article content type, then the site builder needs to remove the not-wanted comment field comment. See the third screenshot in the issue summary.

Hope this helps,
Markus

masipila’s picture

Status: Needs work » Needs review

Back to needs review.

quietone’s picture

@masipila, thank you very much. I have read your comment and re-read the known issue page. It is all clear to me now and I don't think any changes are needed to known issues. I was just a bit muddled.

quietone’s picture

I have applied the patch, read the code and can't find any problems not even a nit. I did have a problem understanding the entity counts changes, but I was reading it as an increase instead of a decrease in the entity counts. maxocub kindly showed me the error of my ways. I still blame it all on the northern hemisphere air!

masipila tested the patch in #90 and reported that the 'Handling for Forum is correct now' and 'Unfortunately the suggested approach from #71 for Article is not ok.' Following discussion resolved how to handle the Article node type.

After that test, the code changes was simply a name change on a method from 'CreateNodeCommentCombo' to 'CreateNodeCommentCombination'. So, I think the manual testing done in #90 still applies.

The Known issues handbook page has been updated.

masipila, does suggest in #90 that another pair of eyes on the other migrations changed in this patch is a good idea. Those migrations are

d7_comment_entity_display.yml
d7_comment_entity_form_display_subject.yml
d7_field_formatter_settings.yml
d7_field_instance.yml
d7_field_instance_widget_settings.yml

The final question, as far as I am concerned, is are the existing test sufficient for the changes to these migrations.

And I need to take a break now.

maxocub’s picture

d7_comment_entity_display.yml

+++ b/core/modules/comment/migration_templates/d7_comment_entity_display.yml
@@ -13,7 +13,14 @@ source:
-  field_name: bundle
+  field_name:
+    -
+      plugin: migration_lookup
+      source: bundle
+      migration: d7_comment_type

The change in this file is to get the field name from the comment type migration. Beside the forum comment field name, the other field names did not change, as shown by this test:

+++ b/core/modules/comment/tests/src/Kernel/Migrate/d7/MigrateCommentEntityDisplayTest.php
@@ -53,7 +53,7 @@ public function testMigration() {
     $this->assertDisplay('node.article.default', 'comment_node_article');
     $this->assertDisplay('node.book.default', 'comment_node_book');
     $this->assertDisplay('node.blog.default', 'comment_node_blog');
-    $this->assertDisplay('node.forum.default', 'comment_node_forum');
+    $this->assertDisplay('node.forum.default', 'comment_forum');
     $this->assertDisplay('node.test_content_type.default', 'comment_node_test_content_type');

d7_comment_entity_form_display_subject.yml

+++ b/core/modules/comment/migration_templates/d7_comment_entity_form_display_subject.yml
@@ -22,7 +22,14 @@ process:
+  bundle:
+    -
+      plugin: migration_lookup
+      source: bundle
+      migration: d7_comment_type

Same as above, but with the comment bundle. And only the forum bundle is changed in the test:

+++ b/core/modules/comment/tests/src/Kernel/Migrate/d7/MigrateCommentEntityFormDisplaySubjectTest.php
@@ -48,7 +48,7 @@ public function testMigration() {
     $this->assertDisplay('comment.comment_node_article.default');
     $this->assertDisplay('comment.comment_node_book.default');
     $this->assertDisplay('comment.comment_node_blog.default');
-    $this->assertDisplay('comment.comment_node_forum.default');
+    $this->assertDisplay('comment.comment_forum.default');
     $this->assertDisplay('comment.comment_node_test_content_type.default');

And the same is true for the 3 other migrations:

  • d7_field_formatter_settings.yml
  • d7_field_instance.yml
  • d7_field_instance_widget_settings.yml
quietone’s picture

Status: Needs review » Reviewed & tested by the community

@maxocub, Absolutely! Thanks for answering while I took a break. Yes, it is obvious that those other migrations are better now that are using migration_lookup.

Woohoo!

maxocub’s picture

@quietone: Thank you! They are better in the way that we don't need to add this mapping in all of them:

+++ b/core/modules/comment/migration_templates/d7_comment_type.yml
@@ -8,7 +8,14 @@ source:
+    plugin: static_map
+    source: bundle
+    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

We do the mapping only in d7_comment_type and get the same value everywhere with the migration lookup.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

@maxocub and @phenaproxima stepped me through this one at DrupalCon Vienna. Looks like @larowlan's feedback from #111 has been addressed. The issue with Articles having two comment types is already existing, regardless of this patch. There's some talk about this in #2906470: Orphan comments and entries in comment_entity_statistics after comment field instance has been deleted, but I think we need a dedicated follow-up critical to this (since it sounds like it's a bit contentious among members of the Migrate team), because nearly everyone installs with Standard (I can find some DA stats to back this up if need be).

My only concern was whether existing D7 migrations would need to make changes here, and it sounds like existing migrations should work fine. Forum module is doing something truly wacky, so we're trying to support it properly here.

Committed and pushed to 8.5.x and back-ported to 8.4.x. Thanks!

  • webchick committed 5cfa420 on 8.5.x
    Issue #2853872 by heddn, maxocub, masipila, phenaproxima, quietone,...

  • webchick committed cb8420a on 8.4.x
    Issue #2853872 by heddn, maxocub, masipila, phenaproxima, quietone,...
masipila’s picture

@webchick, as @maxocub and @phenaproxima most probably explained (and which is clearly documented on the migrate known issues page), the current approach where this issue concluded was that the comment comment field needs to be manually deleted from the Article content type because we are migrating comments to comment_node_article comment field.

The issue #2906470: Orphan comments and entries in comment_entity_statistics after comment field instance has been deleted that you were referring to has nothing to do with migration per se. It's just that this assumption that the redundant comment field is deleted does not currently work because the deletion of the comment field leaves orphan rows in the database at the moment. Based on your comment above, I raised that issue from major to critical.

----

If you feel that we should revisit the decision that D7-D8 migration path (and D6-D8 if D6 had a content type called Article by default, I don't remember by heart) relies on the manual deletion of comment field of Article, then that should indeed be handled as a separate follow-up issue.

Cheers,
Markus

maxocub’s picture

Here is the issue for the discussion about mapping machine names to things provided by the standard installation profile: #2910600: [policy, no patch] How machine names should be migrated.

Status: Fixed » Closed (fixed)

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