NodeType source plugin should include comment information

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.

This is postponing a Migrate critical (#2853872: Migration for forum and article comments: duplicate comment types and incorrect comment_entity_statistics), so adding the tag here too.

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.

Assigning to myself for review this week.

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.

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.

Here's a patch that does the same thing for D7.
If it makes the patch too big for review, I can always split it for D6 & D7 and create a second issue.

Removed the test for the deleted CommentType plugin.

Tests are green so returning to Needs Review.

Added two change records.

1. +++ b/core/modules/comment/migration_templates/d6_comment.yml
   @@ -16,10 +16,15 @@ process:
   +  comment_type:
   +    -
   +      plugin: migration_lookup
   +      source: type
   +      migration: d6_comment_type
   +    -
   +      plugin: skip_on_empty
   +      method: row
   +  field_name: '@comment_type'
   

   What happens if comment type is null? I guess we skip the whole comment then. That seems to make sense to me.

2. +++ b/core/modules/comment/migration_templates/d6_comment_type.yml
   @@ -3,13 +3,23 @@ label: Comment type
   +    id_prefix: 'comment_node_'
   +    label_suffix: 'comment'
   ...
   +    plugin: concat
   +    source:
   +      - 'constants/id_prefix'
   +      - type
   

   Reviewed #2853872: Migration for forum and article comments: duplicate comment types and incorrect comment_entity_statistics and we are doing the same thing there. Good.

3. +++ b/core/modules/comment/src/Plugin/migrate/source/d6/Comment.php
   @@ -33,15 +33,6 @@ public function query() {
   -    if ($this->variableGet('comment_subject_field_' . $row->getSourceProperty('type'), 1)) {
   -      // Comment subject visible.
   -      $row->setSourceProperty('field_name', 'comment');
   -      $row->setSourceProperty('comment_type', 'comment');
   -    }
   -    else {
   -      $row->setSourceProperty('field_name', 'comment_no_subject');
   -      $row->setSourceProperty('comment_type', 'comment_no_subject');
   -    }
   
   +++ /dev/null
   @@ -1,109 +0,0 @@
   -/**
   - * @MigrateSource(
   - *   id = "d6_comment_variable",
   - *   source_module = "comment"
   - * )
   - */
   -class CommentVariable extends DrupalSqlBase {
   
   +++ /dev/null
   @@ -1,63 +0,0 @@
   -/**
   - * @MigrateSource(
   - *   id = "d6_comment_variable_per_comment_type",
   - *   source_module = "comment"
   - * )
   - */
   
   +++ /dev/null
   @@ -1,98 +0,0 @@
   - * Drupal 7 comment type source from database.
   - *
   - * @MigrateSource(
   - *   id = "d7_comment_type",
   - *   source_module = "comment"
   - * )
   - */
   -class CommentType extends DrupalSqlBase {
   

   I think we need or could add a BC layer here.

Re #25:
1. Yes, the row is skipped if the comment type is null.
2. There's some overlap between the two issues, we'll need a re-roll once one of them gets commited.
3. Added back the source plugin and added deprecation notices. Also added back the tests for those deprecated source plugins.

You're the man @maxocub. I've removed the BC break tag and will review this again.

Looks impressive, found only typo

+++ b/core/modules/comment/src/Plugin/migrate/source/d6/CommentVariable.php
@@ -2,6 +2,8 @@
+@trigger_error('CommentVariable is deprecated in Drupal 8.5.x and will be removed before Drupal 9.0.x. Use \Drupal\node\Plugin\migrate\source\d6\ModeType instead.', E_USER_DEPRECATED);

@@ -10,6 +12,9 @@
+ * @deprecated in Drupal 8.5.x, to be removed before Drupal 9.0.x. Use
+ * \Drupal\node\Plugin\migrate\source\d6\NodeType instead.

+++ b/core/modules/comment/src/Plugin/migrate/source/d6/CommentVariablePerCommentType.php
@@ -2,11 +2,16 @@
+@trigger_error('CommentVariablePerCommentType is deprecated in Drupal 8.5.x and will be removed before Drupal 9.0.x. Use \Drupal\node\Plugin\migrate\source\d6\ModeType instead.', E_USER_DEPRECATED);
...
+ * @deprecated in Drupal 8.5.x, to be removed before Drupal 9.0.x. Use
+ * \Drupal\node\Plugin\migrate\source\d6\NodeType instead.

+++ b/core/modules/comment/src/Plugin/migrate/source/d7/CommentType.php
@@ -2,6 +2,8 @@
+@trigger_error('CommentType is deprecated in Drupal 8.5.x and will be removed before Drupal 9.0.x. Use \Drupal\node\Plugin\migrate\source\d7\ModeType instead.', E_USER_DEPRECATED);

@@ -13,6 +15,9 @@
+ * @deprecated in Drupal 8.5.x, to be removed before Drupal 9.0.x. Use
+ * \Drupal\node\Plugin\migrate\source\d7\NodeType instead.

typo s/ModeType/NodeType

Oups, good catch!

Coding standards correction.

Let's move this back to 8.4 since this is really a bug, not a task. So that means we'll need to update the trigger_error message and probably add a Change Record.

Updated the trigger_error messages.

heddn, you asked in IRC for comments if this issue and #2853872: Migration for forum and article comments: duplicate comment types and incorrect comment_entity_statistics are so closely related that one of these needs to be committed first.

--clips--
I discussed this with maxocub on IRC:
21:51 < masipila> heddn asked two days ago here for my opinion if https://www.drupal.org/node/2853872 or
https://www.drupal.org/node/2887142 should be committed first
21:57 < maxocub> Hi masipila, personally, I don't think it matters which one gets in first. I'd be happy to
make the re-roll and I don't think it would be that complicated. Logically though, I think
https://www.drupal.org/node/2887142 should be commited first.
21:57 < masipila> maxocub: I would tend to agree
--clips--

Cheers,
Markus

IS seems updated. All feedback addressed. Let's get this merged.

This needed a re-roll since #2853872: Migration for forum and article comments: duplicate comment types and incorrect comment_entity_statistics landed.

Oups, forgot the files.

Also putting it back to need review so someone else can confirm everything is still good to go.

The only thing added, as seen in the interdiff, is the static map dealing with forum comment type in D6, mirroring what was done for D7 in #2853872: Migration for forum and article comments: duplicate comment types and incorrect comment_entity_statistics.

The D6 entity count needed updating because of the forum comment type migration. We are not creating new comment type, field config, etc. for forum but using the ones already provided by the forum module, like we did in #2853872: Migration for forum and article comments: duplicate comment types and incorrect comment_entity_statistics for D7.

On it.

1. I don't know, they are now removed.
2. Done.
3. Done.
4. Done.
5. Done.
6. Done.
7. Done.
8. Done.
9. Done.
10. Done.
11. Done.
12. Done.
13. Done.
14. Done.
15. Done.
16. Done.
17. Done.
18. Done.
19. This change is to have at least one comment type with no subject field for the tests.
20. We have now one comment type per node type, instead of only having the comment and comment_no_subject comment types.
21. Done.

Unassigning.

Fixing the failing test.

I don't see why those migration are dependencies, but since they are in a test, I leave them there.

1. +++ b/core/modules/comment/migration_templates/d6_comment_type.yml
   @@ -3,13 +3,31 @@ label: Comment type
   +    label_suffix: 'comment'
   ...
   +  label:
   +    plugin: concat
   +    source:
   +      - name
   +      - 'constants/label_suffix'
   
   +++ b/core/modules/comment/migration_templates/d7_comment_type.yml
   @@ -3,22 +3,31 @@ label: Comment type
   +  label:
   +    plugin: concat
   +    source:
   +      - name
   +      - 'constants/label_suffix'
   

   Very mild question/nit. Should we use callback and UC the first word too? If you disagree, ignore.

2. +++ /dev/null
   @@ -1,31 +0,0 @@
   -abstract class MigrateCommentVariableDisplayBase extends MigrateDrupal6TestBase {
   
   +++ /dev/null
   @@ -1,39 +0,0 @@
   -class MigrateCommentVariableEntityDisplayTest extends MigrateCommentVariableDisplayBase {
   
   +++ /dev/null
   @@ -1,45 +0,0 @@
   -class MigrateCommentVariableEntityFormDisplaySubjectTest extends MigrateDrupal6TestBase {
   
   +++ /dev/null
   @@ -1,39 +0,0 @@
   -class MigrateCommentVariableEntityFormDisplayTest extends MigrateCommentVariableDisplayBase {
   
   +++ /dev/null
   @@ -1,37 +0,0 @@
   -class MigrateCommentVariableFieldTest extends MigrateDrupal6TestBase {
   
   +++ /dev/null
   @@ -1,60 +0,0 @@
   -class MigrateCommentVariableInstanceTest extends MigrateDrupal6TestBase {
   

   This seems like we are loosing coverage over the deprecated functionality. Yeah, I don't see any @legacy tags in the patch.

Re #46:

1. The source is the name of the content type which is usually already uppercased, like "Page" and "Article".
2. You're right, I forgot to add them back when we deprecated the classes instead of removing them.

Re #46.2:
Hmm, since these tests are integration tests and not unit tests, it's hard to keep them without modifying them so much that they do exactly what the new tests do.

The yaml files are now using the node_type source plugin, so I don't see how we can keep them and tests those deprecated source plugins.

We could add unit tests for the deprecated classes, but this patch is already big enough. Could we do that in a follow-up if that's really necessary?

This issue was postponing a Migrate critical, #2909752: [D6] Migration for comments: duplicate comment types and incorrect comment_entity_statistics, and is now fixing it right here in this patch.

I closed #2909752: [D6] Migration for comments: duplicate comment types and incorrect comment_entity_statistics as a duplicate, but that means this issue should now be marked as a Critical since it fixes the forum comment type bug.

+++ b/core/modules/comment/src/Plugin/migrate/source/d6/Comment.php
@@ -34,6 +34,8 @@ public function query() {
   public function prepareRow(Row $row) {
+    // This is a backward compatibility layer for the deprecated migrate source
+    // plugins d6_comment_variable and d6_comment_variable_per_comment_type.
     if ($this->variableGet('comment_subject_field_' . $row->getSourceProperty('type'), 1)) {
       // Comment subject visible.
       $row->setSourceProperty('field_name', 'comment');
@@ -43,6 +45,7 @@ public function prepareRow(Row $row) {

@@ -43,6 +45,7 @@ public function prepareRow(Row $row) {
       $row->setSourceProperty('field_name', 'comment_no_subject');
       $row->setSourceProperty('comment_type', 'comment_no_subject');
     }
+

Could we move this to a protected method that we can mark @deprecated? That will make it easier to find for removal in 9.x.

Added @deprecated method

thanks!, description added/sorted

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

tests are incomplete and probably broken so filed follow-up #3007436: Fix d7 comment migration

+++ b/core/modules/node/src/Plugin/migrate/source/d6/NodeType.php
@@ -63,7 +63,7 @@ public function query() {
   public function fields() {
-    return [
+    $fields = [

@@ -79,6 +79,28 @@ public function fields() {
+    if ($this->moduleExists('comment')) {

+++ b/core/modules/node/src/Plugin/migrate/source/d7/NodeType.php
@@ -54,6 +54,28 @@ public function fields() {
+    if ($this->moduleExists('comment')) {
+      $fields += $this->getCommentFields();
+    }
+    return $fields;

d7 source plugin is not fixed as d6 one