Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
Comment | File | Size | Author |
---|---|---|---|
#17 | 3081123-17.patch | 2.52 KB | quietone |
#17 | interdiff-14-17.txt | 998 bytes | quietone |
#14 | 3081123-14.patch | 2.51 KB | quietone |
#14 | interdiff-9-14.txt | 856 bytes | quietone |
#11 | 3081123-9.patch | 2.58 KB | quietone |
Comments
Comment #2
quietone CreditAttribution: quietone as a volunteer commentedComment #3
mikelutzI like it.
Comment #4
alexpottI 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.
In Drupal 6 which module provides the i18n_variable table - it's not system. So perhaps source module should be something else?
Comment #5
quietone CreditAttribution: quietone as a volunteer commentedI found another that should be a requirements exception. I don't see a reason why this can't be fixed in this issue.
Comment #6
quietone CreditAttribution: quietone as a volunteer commentedJust 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'
Comment #7
quietone CreditAttribution: quietone as a volunteer commentedFixes 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.
Comment #9
quietone CreditAttribution: quietone as a volunteer commentedfix @group and change the exception message to make it clear that it is the source database with a missing table.
Comment #11
quietone CreditAttribution: quietone as a volunteer commentedFix namespace and @group error.
Comment #12
quietone CreditAttribution: quietone as a volunteer commentedComment #14
quietone CreditAttribution: quietone as a volunteer commentedOops. Busy day here and I uploaded the patch from 9 again. This should be correct now.
Comment #15
quietone CreditAttribution: quietone as a volunteer commentedEverything 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.
Comment #16
quietone CreditAttribution: quietone as a volunteer commentedcopy paste error
Comment #17
quietone CreditAttribution: quietone as a volunteer commentedCorrect the comments in the test.
Comment #19
srjoshThis looks good to me.
Comment #20
alexpottCommitted and pushed de2072c9fa to 9.0.x and c792c0421d to 8.9.x. Thanks!
I think we should backport this to 8.8.x.
Comment #23
alexpottDiscussed with @catch we agreed to backport.
Comment #26
NiklasBr CreditAttribution: NiklasBr commentedI got the following errors when upgrading from D6.52 to D8.8:
PHP 7.3.12
MySQL 5.7.28
Server nginx/1.14.0