ResponsiveImageStyles source plugin must extend DrupalSqlBase

The 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:

$image_style_source = MigrationDeriverTrait::getSourcePlugin('d7_responsive_image_styles');
image_style_source->checkRequirements();

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:

$this->expectException(RequirementsException::class);
$this->expectExceptionMessage('Profile module not enabled on source site');
$this->getMigration('user_profile_field')
  ->getSourcePlugin()
  ->checkRequirements();

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...

This is a duplicate of an earlier issue #3211510: D7 responsive image styles migration inconsistent with other migrations. Adding credit.

I agree with #4.

1. +++ b/core/modules/responsive_image/tests/src/Kernel/Plugin/migrate/source/d7/ResponsiveImageStylesTest.php
   @@ -24,6 +27,9 @@ public function providerSource() {
   +    $tests[0]['source_data']['system'] = [
   +      ['name' => 'picture', 'type' => 'module', 'status' => 1],
   +    ];
   

   This is not needed. The source plugin only queries the table 'picture_mapping'.

2. +++ b/core/modules/responsive_image/tests/src/Kernel/Migrate/d7/MigrateResponsiveImageStylesTest.php
   @@ -18,17 +20,25 @@ class MigrateResponsiveImageStylesTest extends MigrateDrupal7TestBase {
   +    $this->sourceDatabase->update('system')
   

   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.

3. +++ b/core/modules/responsive_image/tests/src/Kernel/Migrate/d7/MigrateResponsiveImageStylesTest.php
   @@ -18,17 +20,25 @@ class MigrateResponsiveImageStylesTest extends MigrateDrupal7TestBase {
        $expected_image_style_mappings = [
   

   nit: add a blank line before this line for readability.

4. +++ b/core/modules/responsive_image/tests/src/Kernel/Plugin/migrate/source/d7/ResponsiveImageStylesTest.php
   @@ -15,7 +15,10 @@ class ResponsiveImageStylesTest extends MigrateSqlSourceTestBase {
   +    'responsive_image',
   +    'migrate_drupal',
   

   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?

$ grep -ri 'extends SqlBase' core | awk -F: '{p
rint $1}' | sort -u | nl
     1  core/modules/migrate_drupal/src/Plugin/migrate/source/DrupalSqlBase.php
     2  core/modules/migrate/tests/modules/migrate_high_water_test/src/Plugin/migrate/source/Hig
hWaterTest.php
     3  core/modules/migrate/tests/modules/migrate_query_batch_test/src/Plugin/migrate/source/Qu
eryBatchTest.php
     4  core/modules/migrate/tests/modules/migrate_sql_count_cache_test/src/Plugin/migrate/sourc
e/SqlCountCache.php
     5  core/modules/migrate/tests/modules/migrate_track_changes_test/src/Plugin/migrate/source/
TrackChangesTest.php
     6  core/modules/migrate/tests/src/Kernel/SqlBaseTest.php
     7  core/modules/migrate/tests/src/Unit/SqlBaseTest.php
     8  core/modules/responsive_image/src/Plugin/migrate/source/d7/ResponsiveImageStyles.php
     9  core/modules/views/src/Plugin/views/pager/Full.php
    10  core/modules/views/src/Plugin/views/pager/Mini.php

Really adding credit.

I 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.

1. +++ b/core/modules/responsive_image/tests/src/Kernel/Migrate/d7/MigrateResponsiveImageStylesTest.php
   @@ -17,18 +17,19 @@ class MigrateResponsiveImageStylesTest extends MigrateDrupal7TestBase {
   +    // The 'picture' module must be enabled in the source (it is disabled in
   +    // Drupal 7 migration DB fixture).
   

   This 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.'.

2. +++ b/core/modules/responsive_image/tests/src/Kernel/Migrate/d7/MigrateResponsiveImageStylesTest.php
   @@ -17,18 +17,19 @@ class MigrateResponsiveImageStylesTest extends MigrateDrupal7TestBase {
   -  public function setUp(): void {
   

   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.

Here 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.

@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.

Looks like unrelated failures. Restoring RTBC.

Drupal\Tests\layout_builder\FunctionalJavascript\AjaxBlockTest
Drupal\Tests\layout_builder\FunctionalJavascript\ContentPreviewToggleTest

The failure was in Drupal\Tests\layout_builder\FunctionalJavascript\AjaxBlockTest<.code>, clearly an unrelated random fail. Back to RTBC.

Added reroll of patch #15 on Drupal 10.0.x.

@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.

I 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.

@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.

There 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.

Starting a test on 10.0.x

This is blocking an issue with a deprecation, #3009349: Move source_module from Migrate to Migrate Drupal

Committed 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.