Problem/Motivation

The way we migrate comment types from Drupal 6 to Drupal 8 is no longer going to cut it, as we have discovered over in #2853872: Migration for forum and article comments: duplicate comment types and incorrect comment_entity_statistics. That issue is against the D7 migration path, but D6 is affected too because its comment type migration only creates two comment types -- comment and comment_no_subject. This can result in conflicts and problems when the Forum module is introduced, or the Standard profile is installed.

Proposed resolution

The d6_comment_type migration should behave exactly like the d7_comment_type migration and create a new comment type for each node type. To facilitate this, it would be a LOT easier and cleaner to remove the comment type-related source plugins from the D6 migration path, and have the D6 NodeType source plugin include the comment settings for each node type. This will be a million times simpler and cleaner!

Remaining tasks

Write a patch, bikeshed, review, and commit. Not necessarily in that order.

User interface changes

None.

API changes

A few source plugins from Migrate Drupal (which is experimental!) will probably be removed. Therefore, this constitutes a BC break, although it's in an experimental meta-module and therefore within policy.

Data model changes

None.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

phenaproxima created an issue. See original summary.

phenaproxima’s picture

Status: Active » Needs review
FileSize
38.08 KB

Here's a first attempt to ice the variable-based comment settings plugins in favor of merging that information into the d6_node_type plugin...

Status: Needs review » Needs work

The last submitted patch, 2: 2887142-2.patch, failed testing. View results

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
42.9 KB
6.09 KB

This oughta do the trick.

Status: Needs review » Needs work

The last submitted patch, 4: 2887142-4.patch, failed testing. View results

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
43.48 KB
1006 bytes

Okay, THIS should fix all the entity counts. Man...that test takes forever.

maxocub’s picture

Status: Needs review » Needs work
Issue tags: +Needs change record

I think that this makes a lot of sense and that it is much easier to understand what is going on.

Nevertheless, I tested it manually and it doesn't seem to be working properly yet. When I go to a migrated node which is supposed to have comments, I don't see them. It seems like there's no comment fields on the node types.

Maybe the tests should try to reproduce that, like load a node and see if the comments are there.

phenaproxima’s picture

Issue tags: +Needs tests

Well...dammit. Thanks for testing, @maxocub! I will try to reproduce the problem, and expand the test coverage to account for the hole, when I trace it.

maxocub’s picture

maxocub’s picture

Assigned: Unassigned » maxocub
maxocub’s picture

maxocub’s picture

Assigned: maxocub » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
45.61 KB
4.26 KB

I found the problem. Some field instances were not migrated because the comment variables for some content types on the D6 site were not set and they were using default values.
I added the default values in the d6_field_instance migration and modified the comment field instance test so it would also tests those field instances on those content types that are using default values.

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.

heddn’s picture

Assigned: Unassigned » heddn

Assigning to myself for review this week.

maxocub’s picture

Priority: Normal » Critical

This is postponing a critical: #2853872: Migration for forum and article comments: duplicate comment types and incorrect comment_entity_statistics so this should also be critical.
I'll work on the CR so this can be completed.

maxocub’s picture

Priority: Critical » Normal
Status: Needs review » Needs work
Issue tags: -Migrate critical

Hmm, reading the IS and seeing the the issue that is postponed on this is related to D7, I don't think this is really a blocker.
Also, I think it would be nice to do the same thing to the D7 comment type source plugin, just to have a common way to deal with comment type migrations.