Problem/Motivation

This is a problem similar to that in #2859315: SQL error from profile_fields when migrating d6 (or d7) to d8 without Profile module. It was introduced in #2981000: Migrate Drupal 7 comment entity translations data to Drupal 8.

Steps to reproduce

Drupal 7 site without comment module.

Drupal 8|9 site with comment module and content_translation module.

Attempting to load the list of migrations results in the following fatal error:

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupal.comment' doesn't exist: SELECT COUNT(*) AS "expression" FROM (SELECT 1 AS "expression" FROM {entity_translation} "et" INNER JOIN {comment} "c" ON c.cid = et.entity_id INNER JOIN {node} "n" ON n.nid = c.nid WHERE ("et"."entity_type" = :db_condition_placeholder_0) AND ("et"."source" <> :db_condition_placeholder_1)) "subquery"; Array ( [:db_condition_placeholder_0] => comment [:db_condition_placeholder_1] => ) in Drupal\migrate\Plugin\migrate\source\SqlBase->doCount() (line 389 of core/modules/migrate/src/Plugin/migrate/source/SqlBase.php).
Drupal\Core\Database\Statement->execute(Array, Array) (Line: 740)
Drupal\Core\Database\Connection->query('SELECT COUNT(*) AS "expression"
FROM
(SELECT 1 AS "expression"
FROM
{entity_translation} "et"
INNER JOIN {comment} "c" ON c.cid = et.entity_id
INNER JOIN {node} "n" ON n.nid = c.nid
WHERE ("et"."entity_type" = :db_condition_placeholder_0) AND ("et"."source" &lt;&gt; :db_condition_placeholder_1)) "subquery"', Array, Array) (Line: 91)
Drupal\Core\Database\Driver\mysql\Connection->query('SELECT COUNT(*) AS "expression"
FROM
(SELECT 1 AS "expression"
FROM
{entity_translation} "et"
INNER JOIN {comment} "c" ON c.cid = et.entity_id
INNER JOIN {node} "n" ON n.nid = c.nid
WHERE ("et"."entity_type" = :db_condition_placeholder_0) AND ("et"."source" &lt;&gt; :db_condition_placeholder_1)) "subquery"', Array, Array) (Line: 510)
Drupal\Core\Database\Query\Select->execute() (Line: 389)

Proposed resolution

Add CommentEntityTranslation::checkRequirements() similar to #2859315: SQL error from profile_fields when migrating d6 (or d7) to d8 without Profile module.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

None.

CommentFileSizeAuthor
#22 3178966-22.patch2.92 KBquietone
#22 interdiff-20-22.txt824 bytesquietone
#21 3178966-21.patch2.92 KBquietone
#21 interdiff-20-21.txt847 bytesquietone
#20 3178966-20.patch2.94 KBquietone
#20 interdiff-16-20.txt1.59 KBquietone
#16 interdiff-3178966-11-16.txt1004 byteshuzooka
#16 core-comment_entity_translation_migrate_source_requirements-3178966-16.patch2.68 KBhuzooka
#11 3178966-11.patch2.47 KBquietone
#11 interdiff-8-11.txt824 bytesquietone
#8 3178966-8.patch2.46 KBquietone
#8 3178966-8-fail.patch1.49 KBquietone
#8 interdiff-7-8.txt1.98 KBquietone
#7 interdiff_3-7.txt1.2 KBravi.shankar
#7 3178966-7.patch987 bytesravi.shankar
#3 3178966-3.patch1.04 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: Fatal error in \Drupal\comment\Plugin\migrate\source\d7\CommentEntityTranslation when source site does not have » Fatal error in CommentEntityTranslation @MigrationSource when source site does not have comment module installed

Better title.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Active » Needs review
FileSize
3178966-3.patch1.04 KB
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Component: comment.module » migration system

I wonder if this will be noticed faster in the migration system component 🤔

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented

@Wim Leers, yes it will. :-)

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs review » Needs work
Issue tags: +Needs tests 
  1. +++ b/core/modules/comment/src/Plugin/migrate/source/d7/CommentEntityTranslation.php
@@ -15,6 +16,17 @@
+  public function checkRequirements() {

    I really prefer having the query method as the first method in a source plugin, can we move this to the bottom of the source plugin?

  2. +++ b/core/modules/comment/src/Plugin/migrate/source/d7/CommentEntityTranslation.php
@@ -15,6 +16,17 @@
+    if (!$this->getDatabase()->schema()->tableExists('comment')) {

    If we are going to report that comment is not installed then we should check if it is installed, not just for the presence of the comment table.
    if (!$this->moduleExists('comment')) {

As always, this needs a test.

ravi.shankar’s picture

ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commented
FileSize
3178966-7.patch987 bytes
interdiff_3-7.txt1.2 KB

Addressed #6.1 and #6.2 of comment #6, still need works for tests.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
interdiff-7-8.txt1.98 KB
3178966-8-fail.patch1.49 KB
3178966-8.patch2.46 KB

Adding test.

The last submitted patch, 8: 3178966-8-fail.patch, failed testing. View results

huzooka’s picture

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

I've found only one minor thing:

+++ b/core/modules/comment/tests/src/Kernel/Migrate/d7/CommentEntityTranslationCheckRequirementsTest.php
@@ -0,0 +1,42 @@
+  /**
+   * Tests exception is thrown when profile_fields tables do not exist.
+   */
+  public function testCheckRequirements() {

C/P leftover

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs work » Needs review
FileSize
interdiff-8-11.txt824 bytes
3178966-11.patch2.47 KB

Oops. This should fix the comment.

huzooka’s picture

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

This is RTBC when the test passes (it will pass). I see no conflicts with core 9.2.x.

Status: Needs review » Needs work

The last submitted patch, 11: 3178966-11.patch, failed testing. View results

quietone’s picture

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

Setting to RTBC based on the comment in #12.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented
Status: Reviewed & tested by the community » Needs work

Based on the query of this plugin (line #31 and #33), this source plugin also requires the node module. Same situation as in #3176394: Do not migrate comment related configurations if "comment" wasn't enabled on the source site. (This is because Drupal core only migrates node comments.)

So we require three source module:

  1. entity_translation - reflected in the annotation
  2. comment –this will be added now
  3. node – this is missing.
huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented
Issue tags: +Needs tests
FileSize
core-comment_entity_translation_migrate_source_requirements-3178966-16.patch2.68 KB
interdiff-3178966-11-16.txt1004 bytes

Test(s?) still need(s) to be updated.

Wim Leers’s picture

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

#6.2: this thoroughly confused me for a moment — IMHO it's a huge DX problem that the method moduleExists() checks the source site, not the destination site. sourceModuleExists() or moduleExistsInSource() would've been 10x clearer!

This is very close now! 🥳

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented

@Wim Leers, yes, I agree.

Re #15. \Drupal\comment\Plugin\migrate\source\d7\Comment::query does use the node module but that code won't be reached in the test, the checkRequirements will run first. The only modules that need to be installed are those that are needed to create an instance of 'd7_comment_entity_translation' and it's dependent migrations.

Edit: refer to #15

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

Drupal 9.0.10 was released on December 3, 2020 and is the final full bugfix release for the Drupal 9.0.x series. Drupal 9.0.x will not receive any further development aside from security fixes. Sites should update to Drupal 9.1.0 to continue receiving regular bugfixes.

Drupal-9-only bug reports should be targeted for the 9.1.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.2.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Version: 9.1.x-dev » 9.2.x-dev
Status: Needs work » Needs review
FileSize
interdiff-16-20.txt1.59 KB
3178966-20.patch2.94 KB

Adding a test for when the node module is not enabled.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
FileSize
interdiff-20-21.txt847 bytes
3178966-21.patch2.92 KB

Small improvement to remove a local variable.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Issue tags: -Needs tests
FileSize
interdiff-20-22.txt824 bytes
3178966-22.patch2.92 KB

The patch in #21 can be ignored.

Forgot to update the comment.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: Fatal error in CommentEntityTranslation @MigrationSource when source site does not have comment module installed » Fatal error in CommentEntityTranslation @MigrationSource when source site does not have comment or node module installed
Status: Needs review » Reviewed & tested by the community

Übernit after the rework done since #15 (which also should've updated the issue title — did that just now).

+++ b/core/modules/comment/tests/src/Kernel/Migrate/d7/CommentEntityTranslationCheckRequirementsTest.php
@@ -0,0 +1,56 @@
+   * Tests exception is thrown when comment module is not enabled in the source.

s/comment/the given/

Note that this should also be committed to 9.1.x, so queued a test against that too.

Re-RTBC'ing given the earlier RTBC, plus the fact that the current patch looks almost nothing like my original patch anymore :)

  • larowlan committed 08350ae on 9.2.x 
    Issue #3178966 by quietone, huzooka, ravi.shankar, Wim Leers: Fatal...

  • larowlan committed 15956e3 on 9.1.x 
    Issue #3178966 by quietone, huzooka, ravi.shankar, Wim Leers: Fatal...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan as a volunteer and at PreviousNext commented
Version: 9.2.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: +Bug Smash Initiative

Committed 08350ae and pushed to 9.2.x. Thanks!

Fixed on commit per #23

@@ -22,7 +22,7 @@ class CommentEntityTranslationCheckRequirementsTest extends MigrateDrupal7TestBa
   ];

   /**
-   * Tests exception is thrown when comment module is not enabled in the source.
+   * Tests exception thrown when the given module is not enabled in the source.
    *
    * @dataProvider providerTestCheckRequirements
    *

Backported to 9.1.x because this is a major bug and the risk of disruption is low.

Tagging Bug Smash Initiative because I'm checking over RTBC bugs as part of the initiative.

Status: Fixed » Closed (fixed)

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