Problem/Motivation

If a migration using the VariableTranslation source plugin is run and the database does not have the i18n_variable table then the migration fails with an exception with the message 'Migration failed with source plugin exception'. A requirements exception would be better.

Proposed resolution

Add checkRequirements() method to VariableTranslation.

Remaining tasks

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quietone created an issue. See original summary.

quietone’s picture

Status: Active » Needs review
FileSize
1022 bytes
mikelutz’s picture

Status: Needs review » Reviewed & tested by the community

I like it.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

I thought it was okay for this to go in without an explicit test for the exception because it seemed as though it would be tested by \Drupal\Tests\migrate_drupal\Kernel\Plugin\migrate\source\d6\VariableTranslationTest() atleast implicitly. But it's not. So I think we need a test. Also whilst playing around with \Drupal\Tests\migrate\Kernel\MigrateSourceTestBase::testSource() to make it call checkRequirements() I think it reveals that the annotation for this plugin is wrong - or at least needs a comment as to why it is correct.

 * @MigrateSource(
 *   id = "d6_variable_translation",
 *   source_module = "system",
 * )

In Drupal 6 which module provides the i18n_variable table - it's not system. So perhaps source module should be something else?

quietone’s picture

I found another that should be a requirements exception. I don't see a reason why this can't be fixed in this issue.

Migration failed with source plugin exception: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'v.language' in 'field list': SELECT v.vid AS vid, v.language AS language
FROM
@vocabulary v; Array
(
)
quietone’s picture

Just noting where to get the information to update the comment per #4 See https://www.drupal.org/project/drupal/issues/2908282#comment-12274922 and https://www.drupal.org/project/drupal/issues/2908282#comment-12297415 for why source_module = 'system'

quietone’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1.71 KB
2.58 KB

Fixes for #4. A new test has been added and the source_module changed to i18n, which is the module that creates the i18n_variable table. The references in #6 helps explain why it was 'system' but alexpott is correct in that it is wrong.

And in #5 I suggested another source plugin should be changed but alas, I didn't note the source plugin and I am not sure which one it was. So, that can wait for another day.

Finally, should we add a call to checkRequirements in \MigrateSourceTestBase::testSource()? Right now, that seems like a good idea. The down side is that all the plugin tests would need to be changed to add a system table so the source_module is enabled. Still, it would be worth it for improving the tests. In any case, that needs to be explored in a separate issue.

Status: Needs review » Needs work

The last submitted patch, 7: 3081123-7.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
2.58 KB
1.84 KB

fix @group and change the exception message to make it clear that it is the source database with a missing table.

Status: Needs review » Needs work

The last submitted patch, 9: 3081123-9.patch, failed testing. View results

quietone’s picture

Fix namespace and @group error.

quietone’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

quietone’s picture

Status: Needs work » Needs review
FileSize
856 bytes
2.51 KB

Oops. Busy day here and I uploaded the patch from 9 again. This should be correct now.

quietone’s picture

Everything in #4 fixed. I made a new issue to decide if we want to call checkRequirements in the source plugin test, #3084370: Add checkRequirements to the source plugin test MigrateSqlSourceTestBase. Adding a checkRequirement for another source plugin as suggested in #5 and that is not done because I am not 100% sure which plugin it was and it is just a change of the exception being thrown so not a bug per say.

So ready for review.

quietone’s picture

Status: Needs review » Needs work
+++ b/core/modules/migrate_drupal/tests/src/Kernel/d6/VariableTranslationCheckRequirementsTest.php
@@ -0,0 +1,38 @@
+   * Tests exception in thrown when profile_fields tables does not exist.

copy paste error

quietone’s picture

Status: Needs work » Needs review
FileSize
998 bytes
2.52 KB

Correct the comments in the test.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

srjosh’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me.

alexpott’s picture

Version: 8.9.x-dev » 8.8.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed and pushed de2072c9fa to 9.0.x and c792c0421d to 8.9.x. Thanks!

I think we should backport this to 8.8.x.

  • alexpott committed de2072c on 9.0.x
    Issue #3081123 by quietone, alexpott: Add checkrequirements to...

  • alexpott committed c792c04 on 8.9.x
    Issue #3081123 by quietone, alexpott: Add checkrequirements to...
alexpott’s picture

Status: Patch (to be ported) » Fixed

Discussed with @catch we agreed to backport.

  • alexpott committed 8019463 on 8.8.x
    Issue #3081123 by quietone, alexpott: Add checkrequirements to...

Status: Fixed » Closed (fixed)

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

NiklasBr’s picture

I got the following errors when upgrading from D6.52 to D8.8:

Upgrading upgrade_d6_language_content_taxonomy_vocabulary_settings
 [error]  Migration failed with source plugin exception: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'v.language' in 'field list': SELECT v.vid AS vid, v.language AS language, map.sourceid1 AS migrate_map_sourceid1, map.source_row_status AS migrate_map_source_row_status
FROM
{vocabulary} v
LEFT OUTER JOIN d8.migrate_map_upgrade_d6_language_content_taxonomy_vocabulary_set map ON vid = map.sourceid1
WHERE (map.sourceid1 IS NULL) OR (map.source_row_status = :db_condition_placeholder_0); Array
(
    [:db_condition_placeholder_0] => 1
)
 
Upgrading d6_google_analytics_user_settings
Upgrading upgrade_d6_language_negotiation_settings
Upgrading d6_language_content_settings
Upgrading upgrade_d6_menu
Upgrading language_prefixes_and_domains
Upgrading upgrade_d6_menu_links
Upgrading d6_language_negotiation_settings
Upgrading upgrade_d6_node_revision_bild
Upgrading d6_taxonomy_vocabulary
Upgrading upgrade_d6_node_revision_bildspel
Upgrading d6_language_content_taxonomy_vocabulary_settings
 [error]  Migration failed with source plugin exception: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'v.language' in 'field list': SELECT v.vid AS vid, v.language AS language, map.sourceid1 AS migrate_map_sourceid1, map.source_row_status AS migrate_map_source_row_status
FROM
{vocabulary} v
LEFT OUTER JOIN d8.migrate_map_d6_language_content_taxonomy_vocabulary_settings map ON vid = map.sourceid1
WHERE (map.sourceid1 IS NULL) OR (map.source_row_status = :db_condition_placeholder_0); Array
(
    [:db_condition_placeholder_0] => 1
)
 

PHP 7.3.12
MySQL 5.7.28
Server nginx/1.14.0