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.

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! We will then be able to do the mapping for the D6 forum comment type as we did for D7 in #2853872: Migration for forum and article comments: duplicate comment types and incorrect comment_entity_statistics.

And while we're at it, we should do the same for Drupal 7, that is get rid of the CommentType source plugin and move this functionality in the NodeType source plugin, so we'll have a common way to deal with comment types from Drupal 6 & 7.

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.

A few source plugins from Migrate Drupal are now deprecated.

Those are:

  • Drupal\comment\Plugin\migrate\source\d6\CommentVariable
  • Drupal\comment\Plugin\migrate\source\d6\CommentVariablePerCommentType
  • Drupal\comment\Plugin\migrate\source\d7\CommentType

Data model changes

None.

CommentFileSizeAuthor
#57 2887142-57.patch90.29 KBAdita
#57 interdiff-2887142-54-57.txt954 bytesAdita
#54 2887142-54.patch90.22 KBAdita
#54 interdiff-2887142-49-54.txt1.49 KBAdita
#49 interdiff-2887142-47-49.txt9.21 KBmaxocub
#49 2887142-49.patch90.14 KBmaxocub
#47 interdiff-2887142-45-47.txt9.19 KBmaxocub
#47 2887142-47.patch84.49 KBmaxocub
#45 interdiff-2887142-42-45.txt490 bytesmaxocub
#45 2887142-45.patch90.14 KBmaxocub
#42 interdiff-2887142-39-42.txt24.17 KBmaxocub
#42 2887142-42.patch90.34 KBmaxocub
#39 interdiff-2887142-37-39.txt1.24 KBmaxocub
#39 2887142-39.patch78.79 KBmaxocub
#37 interdiff-2887142-32-36.txt796 bytesmaxocub
#37 2887142-36.patch78.79 KBmaxocub
#32 interdiff-30-32.txt3.52 KBJo Fitzgerald
#32 2887142-32.patch78.45 KBJo Fitzgerald
#30 interdiff-29-30.txt681 bytesJo Fitzgerald
#30 2887142-30.patch78.45 KBJo Fitzgerald
#29 interdiff-2887142-26-29.txt2.6 KBmaxocub
#29 2887142-29.patch78.69 KBmaxocub
#26 interdiff-2887142-19-26.txt17.85 KBmaxocub
#26 2887142-26.patch78.44 KBmaxocub
#19 interdiff-2887142-17-19.txt4.34 KBmaxocub
#19 2887142-19.patch91.4 KBmaxocub
#17 interdiff-2887142-12-17.txt67.01 KBmaxocub
#17 2887142-17.patch88.59 KBmaxocub
#12 interdiff-6-12.txt4.26 KBmaxocub
#12 2887142-12.patch45.61 KBmaxocub
#6 interdiff-2887142-4-6.txt1006 bytesphenaproxima
#6 2887142-6.patch43.48 KBphenaproxima
#4 interdiff-2887142-2-4.txt6.09 KBphenaproxima
#4 2887142-4.patch42.9 KBphenaproxima
#2 2887142-2.patch38.08 KBphenaproxima
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.

maxocub’s picture

Status: Needs work » Needs review
FileSize
88.59 KB
67.01 KB

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.

Status: Needs review » Needs work

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

maxocub’s picture

Removed the test for the deleted CommentType plugin.

maxocub’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

Jo Fitzgerald’s picture

Status: Needs work » Needs review

Tests are green so returning to Needs Review.

maxocub’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update
maxocub’s picture

Issue tags: -Needs change record

Added two change records.

heddn’s picture

Status: Needs review » Needs work
  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.

maxocub’s picture

Status: Needs work » Needs review
FileSize
78.44 KB
17.85 KB

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.

heddn’s picture

Assigned: heddn » Unassigned
Issue tags: -Migrate BC break

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

andypost’s picture

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

maxocub’s picture

Status: Needs work » Needs review
FileSize
78.69 KB
2.6 KB

Oups, good catch!

Jo Fitzgerald’s picture

Coding standards correction.

heddn’s picture

Version: 8.5.x-dev » 8.4.x-dev
Category: Task » Bug report
Status: Needs review » Needs work
Issue tags: +migrate-d7-d8, +Needs issue summary update, +Needs change record

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.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
78.45 KB
3.52 KB

Updated the trigger_error messages.

heddn’s picture

Assigned: Unassigned » heddn
masipila’s picture

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

heddn’s picture

Assigned: heddn » Unassigned
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update, -Needs change record +Vienna2017

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

maxocub’s picture

maxocub’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
78.79 KB
796 bytes

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.

Status: Needs review » Needs work

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

maxocub’s picture

Status: Needs work » Needs review
FileSize
78.79 KB
1.24 KB

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.

phenaproxima’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

It's a long patch, but it's looking truly excellent. I really like the changes made here. I also like the improved and expanded test coverage, and I think the migration path that comment types and fields take will be much clearer after this lands. Bravo to all!

Since it's me, I found many nitpicks -- almost all doc comment things. I do want to add more test coverage for the NodeType plugins, so I'm kicking this back as needing tests, but I think that will be doable in a jif.

  1. +++ b/core/modules/comment/migration_templates/d7_comment.yml
    @@ -42,3 +42,5 @@ migration_dependencies:
    +    - d7_comment_entity_display
    +    - d7_comment_entity_form_display
    

    Why were these dependencies added?

  2. +++ b/core/modules/comment/tests/src/Kernel/Migrate/d6/MigrateCommentEntityDisplayTest.php
    @@ -0,0 +1,70 @@
    +/**
    + * Tests the migration of comment entity displays from Drupal 6.
    + *
    + * @group comment
    + */
    +class MigrateCommentEntityDisplayTest extends MigrateDrupal6TestBase {
    

    This should also be in the migrate_drupal_6 test group.

  3. +++ b/core/modules/comment/tests/src/Kernel/Migrate/d6/MigrateCommentEntityDisplayTest.php
    @@ -0,0 +1,70 @@
    +   * Asserts a display entity.
    

    We can improve this. How about "Asserts various aspects of a comment component in an entity view display"?

  4. +++ b/core/modules/comment/tests/src/Kernel/Migrate/d6/MigrateCommentEntityDisplayTest.php
    @@ -0,0 +1,70 @@
    +    $this->assertTrue(is_array($component));
    

    Let's use assertInternalType('array', $component) here.

  5. +++ b/core/modules/comment/tests/src/Kernel/Migrate/d6/MigrateCommentEntityFormDisplaySubjectTest.php
    @@ -0,0 +1,72 @@
    +/**
    + * Tests the migration of comment form's subject display from Drupal 6.
    + *
    + * @group comment
    + */
    +class MigrateCommentEntityFormDisplaySubjectTest extends MigrateDrupal6TestBase {
    

    I think this should also have the migrate_drupal_6 group.

  6. +++ b/core/modules/comment/tests/src/Kernel/Migrate/d6/MigrateCommentEntityFormDisplaySubjectTest.php
    @@ -0,0 +1,72 @@
    +  /**
    +   * Asserts that the comment subject field is visible for a node type.
    +   *
    +   * @param string $id
    +   *   The entity ID.
    +   */
    +  protected function assertSubjectVisible($id) {
    

    Let's change $id's description to "The entity form display ID."

  7. +++ b/core/modules/comment/tests/src/Kernel/Migrate/d6/MigrateCommentEntityFormDisplaySubjectTest.php
    @@ -0,0 +1,72 @@
    +  /**
    +   * Asserts that the comment subject field is not visible for a node type.
    +   *
    +   * @param string $id
    +   *   The entity ID.
    +   */
    +  protected function assertSubjectNotVisible($id) {
    

    Same here.

  8. +++ b/core/modules/comment/tests/src/Kernel/Migrate/d6/MigrateCommentEntityFormDisplayTest.php
    @@ -0,0 +1,68 @@
    +/**
    + * Tests the migration of comment form display from Drupal 6.
    + *
    + * @group comment
    + */
    +class MigrateCommentEntityFormDisplayTest extends MigrateDrupal6TestBase {
    

    Again, let's have this in the migrate_drupal_6 group too.

  9. +++ b/core/modules/comment/tests/src/Kernel/Migrate/d6/MigrateCommentEntityFormDisplayTest.php
    @@ -0,0 +1,68 @@
    +  /**
    +   * Asserts a display entity.
    +   *
    +   * @param string $id
    +   *   The entity ID.
    +   * @param string $component_id
    +   *   The ID of the form component.
    +   */
    +  protected function assertDisplay($id, $component_id) {
    

    As before, we can improve this doc comment. Just reuse my previous suggestions :)

  10. +++ b/core/modules/comment/tests/src/Kernel/Migrate/d6/MigrateCommentEntityFormDisplayTest.php
    @@ -0,0 +1,68 @@
    +    $this->assertTrue(is_array($component));
    

    assertInternalType() is preferable here.

  11. +++ b/core/modules/comment/tests/src/Kernel/Migrate/d6/MigrateCommentFieldInstanceTest.php
    @@ -0,0 +1,88 @@
    +
    +/**
    + * Tests the migration of comment field instances from Drupal 6.
    + *
    + * @group comment
    + */
    +class MigrateCommentFieldInstanceTest extends MigrateDrupal6TestBase {
    

    Let's add this test to migrate_drupal_6 group.

  12. +++ b/core/modules/comment/tests/src/Kernel/Migrate/d6/MigrateCommentFieldTest.php
    @@ -0,0 +1,64 @@
    +class MigrateCommentFieldTest extends MigrateDrupal6TestBase {
    

    And this one.

  13. +++ b/core/modules/comment/tests/src/Kernel/Migrate/d6/MigrateCommentTest.php
    @@ -2,13 +2,15 @@
    - * @group migrate_drupal_6
    + * @group comment
    

    We want to keep this in both test groups.

  14. +++ b/core/modules/comment/tests/src/Kernel/Migrate/d6/MigrateCommentTypeTest.php
    @@ -6,9 +6,9 @@
    - * @group migrate_drupal_6
    + * @group comment
    

    The test should be in both groups.

  15. +++ b/core/modules/comment/tests/src/Kernel/Migrate/d7/MigrateCommentEntityDisplayTest.php
    @@ -6,12 +6,15 @@
      * @group comment
    

    Let's add this test to migrate_drupal_7 group as well.

  16. +++ b/core/modules/comment/tests/src/Kernel/Migrate/d7/MigrateCommentEntityFormDisplaySubjectTest.php
    @@ -19,37 +22,47 @@ class MigrateCommentEntityFormDisplaySubjectTest extends MigrateDrupal7TestBase
    +   * @param string $id
    +   *   The entity ID.
    +   */
    +  protected function assertSubjectVisible($id) {
    

    $id's description should be "The entity form display ID."

  17. +++ b/core/modules/comment/tests/src/Kernel/Migrate/d7/MigrateCommentEntityFormDisplaySubjectTest.php
    @@ -19,37 +22,47 @@ class MigrateCommentEntityFormDisplaySubjectTest extends MigrateDrupal7TestBase
    +   * Asserts that the comment subject field is not visible for a node type.
        *
        * @param string $id
        *   The entity ID.
    

    Same here.

  18. +++ b/core/modules/comment/tests/src/Kernel/Migrate/d7/MigrateCommentFieldInstanceTest.php
    @@ -2,19 +2,19 @@
      * @group comment
      */
     class MigrateCommentFieldInstanceTest extends MigrateDrupal7TestBase {
    

    Let's add this test to migrate_drupal_7 as well.

  19. +++ b/core/modules/migrate_drupal/tests/fixtures/drupal7.php
    @@ -46146,7 +46146,7 @@
       'name' => 'comment_subject_field_test_content_type',
    -  'value' => 'i:1;',
    +  'value' => 'i:0;',
    

    How did this change come to be?

  20. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MigrateUpgrade6Test.php
    @@ -40,12 +40,12 @@ protected function getEntityCounts() {
    -      'comment_type' => 3,
    +      'comment_type' => 13,
    

    Why do we suddenly have ten more comment types? I'm not opposed to this, but we may need to explain that to the committers so it will be good to document it here.

  21. +++ b/core/modules/node/src/Plugin/migrate/source/d7/NodeType.php
    @@ -107,6 +129,13 @@ public function prepareRow(Row $row) {
    +    if ($this->moduleExists('comment')) {
    +      foreach (array_keys($this->getCommentFields()) as $field) {
    +        $row->setSourceProperty($field, $this->variableGet($field . '_' . $type, NULL));
    +      }
    +    }
    

    We should probably expand the unit tests of the NodeType plugins to ensure that they return this data properly. It's pretty clear that it works, but more test coverage never hurts.

maxocub’s picture

Assigned: Unassigned » maxocub

On it.

maxocub’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
90.34 KB
24.17 KB
  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.
maxocub’s picture

Assigned: maxocub » Unassigned

Unassigning.

Status: Needs review » Needs work

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

maxocub’s picture

Status: Needs work » Needs review
FileSize
90.14 KB
490 bytes

Fixing the failing test.

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

heddn’s picture

Status: Needs review » Needs work
  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.

maxocub’s picture

Status: Needs work » Needs review
FileSize
84.49 KB
9.19 KB

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.

Status: Needs review » Needs work

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

maxocub’s picture

Status: Needs work » Needs review
FileSize
90.14 KB
9.21 KB

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?

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Maybe I'm being dense, but I don't see a problem here. All the affected migrations still have passing integration test coverage. So I'm not sure what else we're missing.

maxocub’s picture

Priority: Normal » Critical
Issue summary: View changes
Issue tags: +Migrate critical

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.

catch’s picture

Status: Reviewed & tested by the community » Needs review
+++ 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.

phenaproxima’s picture

Status: Needs review » Needs work

I think that's a good idea. Let's get that done, then I think this is back to RTBC.

Adita’s picture

Added @deprecated method

Adita’s picture

Status: Needs work » Needs review
phenaproxima’s picture

Status: Needs review » Needs work

Thanks, @Adita! Only one thing remains.

+++ b/core/modules/comment/src/Plugin/migrate/source/d6/Comment.php
@@ -34,8 +34,18 @@ public function query() {
+   * @param \Drupal\migrate\Row $row
+   * @return \Drupal\migrate\Row

Each of these need a short, one-line description, and should come before the @deprecated tag.

Adita’s picture

Status: Needs work » Needs review
FileSize
954 bytes
90.29 KB

thanks!, description added/sorted

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Glorious. RTBC once Drupal CI passes it.

  • catch committed 5b3fdce on 8.5.x
    Issue #2887142 by maxocub, phenaproxima, Jo Fitzgerald, Adita, heddn,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

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

Status: Fixed » Closed (fixed)

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