Problem/Motivation

If one migrates data from a Drupal 7 source that does not have comment module enabled, but the destination site does have comment module enabled, every comment related migrations (which are using the d7_node_type source plugin) will be executed producing e.g. comment types for every node type in the source.

Affected migration plugins:
Drupal 6:

  • d6_comment_entity_display
  • d6_comment_entity_form_display
  • d6_comment_entity_form_display_subject
  • d6_comment_field
  • d6_comment_field_instance
  • d6_comment_type

Drupal 7:

  • d7_comment_entity_display
  • d7_comment_entity_form_display
  • d7_comment_entity_form_display_subject
  • d7_comment_field
  • d7_comment_field_instance
  • d7_comment_type

Proposed resolution

Do not migrate comment bundles when the comment module is disabled on the source Drupal 7 instance.

  • Solution A: add a new migration sources for comment configuration migrations that replaces d7_node_type and d6_node_type. This source plugin has to extend copy the corresponding d[6|9]_node_type migration source plugin, but it should define comment as its source module.
  • Solution B (if #3176393: Use SourcePluginBase::getSourceModule() in DrupalSqlBase::checkRequirements() can be committed): add a source_module: comment to the migrations listed above. This isn't enough since comment type migrations require both comment and node modules.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#23 diff-10-18.txt962 bytesquietone
#18 core-migrate_comment_config_if_comment_enabled-3176394-18--9.1.x--complete.patch26.38 KBhuzooka
#18 core-migrate_comment_config_if_comment_enabled-3176394-18--9.1.x--test-only.patch9.85 KBhuzooka
#10 interdiff-3176394-4-10.txt1.87 KBhuzooka
#10 core-do_not_migrate_comment_configurations_if_comment_wasnt_enabled_on_source-3176394-10--complete.patch26.38 KBhuzooka
#10 core-do_not_migrate_comment_configurations_if_comment_wasnt_enabled_on_source-3176394-10--test-only.patch9.85 KBhuzooka
#4 core-do_not_migrate_comment_configurations_if_comment_wasnt_enabled_on_source-3176394-4--complete.patch25.9 KBhuzooka
#4 core-do_not_migrate_comment_configurations_if_comment_wasnt_enabled_on_source-3176394-4--test-only.patch9.36 KBhuzooka
#2 core-do_not_migrate_comment_configurations_if_comment_wasnt_enabled_on_source--on-top-of-3176393--3176394-2.patch5.84 KBhuzooka
#2 core-do_not_migrate_comment_configurations_if_comment_wasnt_enabled_on_source-3176394-2.patch8.49 KBhuzooka
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

huzooka created an issue. See original summary.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented
Assigned: huzooka » Unassigned
Issue summary: View changes
Status: Active » Needs review
Related issues: +#3176393: Use SourcePluginBase::getSourceModule() in DrupalSqlBase::checkRequirements()
FileSize
core-do_not_migrate_comment_configurations_if_comment_wasnt_enabled_on_source-3176394-2.patch8.49 KB
core-do_not_migrate_comment_configurations_if_comment_wasnt_enabled_on_source--on-top-of-3176393--3176394-2.patch5.84 KB
huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented
Assigned: Unassigned » huzooka
Status: Needs review » Needs work
huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented
Assigned: huzooka » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
FileSize
core-do_not_migrate_comment_configurations_if_comment_wasnt_enabled_on_source-3176394-4--test-only.patch9.36 KB
core-do_not_migrate_comment_configurations_if_comment_wasnt_enabled_on_source-3176394-4--complete.patch25.9 KB

Added a test-only patch for comment type (comment "bundle") migration tests to verify that no comment types are migrated when comment, node or both of them are disabled on the source.

The new "complete" patch adds additional test coverage for the new CommentType (comment_type) migration source plugin.

  • \Drupal\Tests\comment\Kernel\Plugin\migrate\source\CommentTypeTest verifies the basic functionality.
  • \Drupal\Tests\comment\Kernel\Plugin\migrate\source\CommentTypeRequirementsTest tests the expected \Drupal\migrate\Exception\RequirementsException

The last submitted patch, 4: core-do_not_migrate_comment_configurations_if_comment_wasnt_enabled_on_source-3176394-4--test-only.patch, failed testing. View results

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community

Tested in detail, works great!

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Reviewed & tested by the community » Needs work

Yes, the test coverage certainly looks thorough!

The two items I found are in the d6/d7 MigrateCommentTypeTests.

  1. +++ b/core/modules/comment/tests/src/Kernel/Migrate/d6/MigrateCommentTypeTest.php
@@ -60,4 +54,85 @@ public function testMigration() {
+  public function testNoCommentTypeMigration(array $disabled_source_modules, array $expected_messages) {

    When the source fixture needs to be changed in a Migrate test the MigrateDumpAlterInterface is used for that. That would mean that this new test would move to a separate file. I am on the fence about this. It seems extra work for little gain. 

  2. +++ b/core/modules/comment/tests/src/Kernel/Migrate/d6/MigrateCommentTypeTest.php
@@ -60,4 +54,85 @@ public function testMigration() {
+   * Tests comment type migration with disabled node or comment on source.

    According to the data provider this also tests with node and comment disabled but this summary suggests, to me, that it is either one or the other that is disabled. Maybe just make the summary line generic, "Tests comment type migration".

I'm setting to NW and feel free to push back on the first point.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented
Assigned: Unassigned » huzooka
huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented
Assigned: huzooka » Unassigned
Status: Needs work » Needs review
FileSize
core-do_not_migrate_comment_configurations_if_comment_wasnt_enabled_on_source-3176394-10--test-only.patch9.85 KB
core-do_not_migrate_comment_configurations_if_comment_wasnt_enabled_on_source-3176394-10--complete.patch26.38 KB
interdiff-3176394-4-10.txt1.87 KB

Re #8:

  1. Added a comment that refers MigrateDumpAlterInterface::migrateDumpAlter() but also explains why it isn't used in this case.
  2. Method comment changed to "Tests comment type migration without node or / and comment on source."; I hope that is good enough now.
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community

  • catch committed 20895bd on 9.2.x 
    Issue #3176394 by huzooka, Wim Leers, quietone: Do not migrate comment...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 9.2.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed 20895bd and pushed to 9.2.x. Thanks!

I think this should also go into 9.1.x - while it adds a class, it's fixing a bug and we generally err on the side of backporting migration fixes especially since we're still in the rc phase, but leaving at 'to be ported' in case we need to discuss more.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

I think this should also go into 9.1.x - while it adds a class, it's fixing a bug and we generally err on the side of backporting migration fixes especially since we're still in the rc phase, but leaving at 'to be ported' in case we need to discuss more.

+1

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Patch (to be ported) » Reviewed & tested by the community

Sorry for not following through sooner here 😬

The patch in #10 in fact applies cleanly to 9.1.x — and still does today. Queuing tests to prove this…

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented
Assigned: Unassigned » huzooka
huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented
Assigned: huzooka » Unassigned
Status: Needs work » Needs review
FileSize
core-migrate_comment_config_if_comment_enabled-3176394-18--9.1.x--test-only.patch9.85 KB
core-migrate_comment_config_if_comment_enabled-3176394-18--9.1.x--complete.patch26.38 KB

Let's see (first with SQLite)!

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented
Issue tags: -Needs reroll

With SQLite #18 works as it should – let's see othere nvs...

Status: Needs review » Needs work

The last submitted patch, 18: core-migrate_comment_config_if_comment_enabled-3176394-18--9.1.x--complete.patch, failed testing. View results

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented

Failure of env PHP 7.3 & MySQL 5.7 seems to be unrelated (and I think I have already seen that before).

Drupal\BuildTests\Framework\Tests\BuildTestTest::testPortMany
RuntimeException: Unable to start the web server.

https://www.drupal.org/pift-ci-job/1958716

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented
Status: Needs work » Needs review
quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs review » Reviewed & tested by the community
FileSize
diff-10-18.txt962 bytes

I made a diff of the patches in #10 and #18 and confirmed that the reroll is fine. Patch still applies.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

We need a change record here to announce the new source migration plugin.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record

CR created: https://www.drupal.org/node/3197281

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Reviewed & tested by the community » Fixed

Let's get this into 9.1.x - in #24 I didn't realise that we'd already added to 9.2.x and this was only open for 9.1.x backport. I agree that since this is an addition, that fixes a bug, it is safe to backport.

  • alexpott committed 752c8ff on 9.1.x authored by catch 
    Issue #3176394 by huzooka, Wim Leers, quietone: Do not migrate comment...

Status: Fixed » Closed (fixed)

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