Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
If one enables Responsive Images module, and tries to upgrate a Drupal 7 instance without the picture module's tables being available, an exception is thrown:
Migration failed with source plugin exception: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupal_blue.picture_mapping' doesn't exist: SELECT "p".*, "map"."sourceid1" AS "migrate_map_sourceid1", "map"."source_row_status" AS "migrate_map_source_row_status"\n
FROM\n
"picture_mapping" "p"\n
LEFT OUTER JOIN drupal_blue.migrate_map_d7_responsive_image_styles "map" ON machine_name = map.sourceid1\n
WHERE ("map"."sourceid1" IS NULL) OR ("map"."source_row_status" = :db_condition_placeholder_0); Array\n
(\n
[:db_condition_placeholder_0] => 1\n
)\n
in [docroot]/core/lib/Drupal/Core/Database/Driver/mysql/ExceptionHandler.php line 53'
This happens because the ResponsiveImageStyles
migrate source plugin extends SqlBase
instead of DrupalSqlBase, so the source module requirement aren't checked by Migrate Drupal's MigrationPluginManager.
Proposed resolution
ResponsiveImageStyles source plugin must extend DrupalSqlBase.
Remaining tasks
- Patch.
- Test.
User interface changes
Nothing.
API changes
ResponsiveImageStyles source plugin extends DrupalSqlBase (following Drupal's Migrate API best practices).
Data model changes
Nothing.
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#26 | 3250582-26-9.4.x.patch | 2.47 KB | Matroskeen |
#26 | 3250582-26-10.0.x.patch | 2.47 KB | Matroskeen |
|
Comments
Comment #2
huzookaComment #3
huzookaComment #4
MatroskeenThe fix is good and makes sense to me. I looked at the original issue #2993367: Add the migration from D7 Picture to Responsive Image trying to find the reason for it, but it wasn't discussed anywhere.
I looked at the test and noticed the following construction, which was new to me:
Looks like
MigrationDeriverTrait::getSourcePlugin()
is supposed to be used in derivers (not in tests).Here is a typical construction to test the exception and I think we should go for it:
Also, while we're here - maybe we should check other source plugins too? There is a chance that somewhere there is a plugin with the exact same issue...
Comment #5
huzookaAddressing #4.(a)
@Matroskeen
I thought a lot this weekend about how to test this in the SQL source plugin base test...
My idea: by using annotation reader, where I find a 'source_module', I would simply check whether the actual plugin class (or one of its base classes) are extending DrupalSqlBase. Ideally inside the getPlugin() method.
What do you think about it? Can you give some guidance?
Comment #6
quietone CreditAttribution: quietone at PreviousNext commentedThis is a duplicate of an earlier issue #3211510: D7 responsive image styles migration inconsistent with other migrations. Adding credit.
I agree with #4.
This is not needed. The source plugin only queries the table 'picture_mapping'.
It is not common to update the source database for a migration tests. Let's have a comment explaining why the source db must be updated.
nit: add a blank line before this line for readability.
nit: If other changes are being done, make this alphabetical.
There are not other source plugins that should be using DrupalSqlBase and aren't. This one is an oversight, mostly on me. I was working off an older patch and just didn't notice. Writing a test to prevent this from happening again is ideal but is it needed? Are there any issues for creating more DrupalSqlbase source plugins?
Comment #8
quietone CreditAttribution: quietone at PreviousNext commentedReally adding credit.
Comment #9
huzookaA bit different approach in regards of the test-only patch (extra check added to
MigrateSourceTestBase::getPlugin()
).This test revealed that of the plugins that have a test coverage, only ResponsiveImageStyles has this particular bug.
Addresses everything in #6.
Comment #11
huzookaAdded "needs subsystem maintainer review" because of the non-trivial base test change.
Comment #12
MatroskeenI like the approach in #9, and I would RTBC... but given that there are some changes to MigrateSourceTestBase I think one of the migrate maintainers should do that.
Comment #13
quietone CreditAttribution: quietone at PreviousNext commentedThis will not be true if for some other reason the fixture changes. I'd rather just, 'Ensure the 'picture' module is enabled in the source.'.
Removing setup is not needed. Let's keep this test in accord with all the other Migration Kernel tests and the patch smaller. So, just put the source db update here.
I should have mentioned this earlier, sorry.
The added test is further proof that this fix is correct and that no other source plugins are affected. That is good.
I don't think it is a good idea to commit the test though. The source_module property is defined in the annotation in \Drupal\migrate\Annotation\MigrateSource and the documentation states it is the responsibility of the source plugin how that value is used. The text of the message in this patch is not in agreement with the documentation.
Yes, in core, it is practice that source_module is used for Migrate Drupal. And there is also an issue to move source_module and destination_module to Migrate Drupal, #3009349: Move source_module from Migrate to Migrate Drupal. That will need to be done so that Migrate Drupal can be moved to core.
Comment #15
danflanagan8Here is a new patch with the updates requested in #13. Most notably, all the changes to
MigrateSourceTestBase
have been reverted. This is a fun case where the patch is considerably smaller than the interdiff.@quietone seems to be suggesting that something similar to the fail test could be considered as part of #3009349: Move source_module from Migrate to Migrate Drupal, and that sounds reasonable to me.
Comment #16
quietone CreditAttribution: quietone at PreviousNext commented@danflanagan8, thank you! This is all that is needed to fix the problem.
I am confident this is the only source plugin in core affected, and the only issue I have found in the migration component that is adding a source plugin is the one for json. (And at the latest meeting the migrate maintainers have decided to do that in contrib). It was an unfortunate situation that the we missed that this plugin did not extend from DrupalSqlBase in the original issue. On the positive side, I think this is the only one of the 110 migrate drupal source plugins in core that this has happened to, and it is the last one.
Comment #18
quietone CreditAttribution: quietone at PreviousNext commentedLooks like unrelated failures. Restoring RTBC.
Drupal\Tests\layout_builder\FunctionalJavascript\AjaxBlockTest
Drupal\Tests\layout_builder\FunctionalJavascript\ContentPreviewToggleTest
Comment #20
danflanagan8The failure was in
Drupal\Tests\layout_builder\FunctionalJavascript\AjaxBlockTest<.code>, clearly an unrelated random fail. Back to RTBC.
Comment #22
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedAdded reroll of patch #15 on Drupal 10.0.x.
Comment #23
danflanagan8@ravi.shankar, I don't understand the patch in #22. The recent failure test failure in the RTBC'd patch was clearly a random failure that had nothing to do with the patch: https://www.drupal.org/pift-ci-job/2311366
I would like to set this back to RTBC but that would set the patch in #22 to be the RTBC'd patch.
Comment #24
MatroskeenI triggered tests for Drupal 10.x on patches #15 and #22.
Patch #15 does not apply, so this is probably why have patch #22.
I assume we want bug fixes to go in both development branches, so we just need to make sure that patch #22 is correct.
Comment #25
quietone CreditAttribution: quietone at PreviousNext commented@ravi.shankar, when rerolling a patch it helps to always add a diff and a comment.
Here is a diff of between to 9.4 and 10 patch.
Comment #26
MatroskeenThere is no reason for this issue to be stuck at "needs review". Let's get this in!
I'm re-uploading patches #15 and #22 in a single comment. If you're curious about the diff - see comment #25.
I'm also reverting it back to RTBC as per #16 and my quick review.
Comment #27
quietone CreditAttribution: quietone at PreviousNext commentedStarting a test on 10.0.x
Comment #28
quietone CreditAttribution: quietone at PreviousNext commentedThis is blocking an issue with a deprecation, #3009349: Move source_module from Migrate to Migrate Drupal
Comment #29
alexpottCommitted 1e98456 and pushed to 10.0.x. Thanks!
Committed and pushed d5cb46bb07 to 9.5.x and 372bdb8789 to 9.4.x and ec8f30ee61 to 9.3.x. Thanks!
Backported to 9.3.x since this is bug fix.