Use SourcePluginBase::getSourceModule() in DrupalSqlBase::checkRequirements()

Just some thoughts.

1. +++ b/core/modules/migrate/src/Plugin/MigrateSourceInterface.php
   @@ -116,4 +116,13 @@ public function getIds();
   +  public function getMinimumSchemaVersion();
   

   I think that since this is specific to a Drupal database it should be in DrupslSqlBase. I know that this interface also has getSourceModule which is also specific to Drupal database, it shouldn't be here either. I'd rather not compound the error.

2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/DrupalSqlBase.php
   @@ -103,14 +103,16 @@ public static function create(ContainerInterface $container, array $configuratio
   +          $minimum_schema_version = $this->getMinimumSchemaVersion();
   +          $module_schema_version = (int) $this->getModuleSchemaVersion($source_module);
   +          if ($minimum_schema_version && !($module_schema_version < $minimum_schema_version)) {
   

   It is nice to have the comparison done on integers.

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/DrupalSqlBase.php
@@ -103,14 +103,16 @@ public static function create(ContainerInterface $container, array $configuratio
-          if (isset($this->pluginDefinition['minimum_schema_version']) && !$this->getModuleSchemaVersion($this->pluginDefinition['source_module']) < $this->pluginDefinition['minimum_schema_version']) {
-            throw new RequirementsException('Required minimum schema version ' . $this->pluginDefinition['minimum_schema_version'], ['minimum_schema_version' => $this->pluginDefinition['minimum_schema_version']]);

I didn't even know the migration system supported this! Very good to know 🤓

Reroll and remove the source_module declaration from d6_field.yml and let's see what breaks.

Ah, those tests are nice proof that this is working as intended.

Do we need the tags since this in the migration system component?

Meant to change the name of the dblog test method. It looked like an old copy/paste error.

I wanted to review this issue, but I noticed that last patch doesn't match the issue summary.

Introduce a new ::getMinimumSchemaVersion() method in MigrateSourceInterface, implement the new method in SourcePluginBase that applies the same logic as SourcePluginBase::getSourceModule() (but for minimum_schema_version) and refactor DrupalSqlBase::checkRequirements() to use these methods.

I don't see these changes in the patch. If the proposed resolution was changed, we should probably reflect this in the issue summary. Or it still needs more work. In any case, I'm moving this to NR.

IS updated and new patch with just comment changes.

#3283795: ComposerHooksTest is broken on latest DrupalCI PHP container fixed that (unrelated) fail on D10.

+++ b/core/modules/field/migrations/d6_field.yml
@@ -11,8 +11,6 @@ source:
-  # Phone is here since it does not use a migrate field plugin.
-  source_module: phone

Why are we removing this? I can't see how this change makes it not necessary more. I checked the issue comments and I couldn't see an explanation. It was added in #2936365-203: Migrate UI - allow modules to declare the state of their migrations maybe the comment #203 on that issue will shed some light as to whether this will break something.

I looked back at the issue that added the 'source_module: phone' and I am convinced it was an error. The correct entry is in core/modules/field/migrations/state/field.migrate_drupal.ymlcheckRequirementscheckRequirements.

Regarding #20, #21:

I checked the issue comments and I couldn't see an explanation.

The change is mentioned in #7. You will not see it in the interdiff attached to that comment, maybe because that patch was also a reroll.

Looking at the final patch from #2936365-309: Migrate UI - allow modules to declare the state of their migrations, I see this duplication:

    diff --git a/core/modules/field/migrations/d6_field.yml b/core/modules/field/migrations/d6_field.yml
    index e25d7ae0c2..2c64f21ea0 100644
    --- a/core/modules/field/migrations/d6_field.yml
    +++ b/core/modules/field/migrations/d6_field.yml
    @@ -10,6 +10,8 @@ source:
       constants:
         entity_type: node
         langcode: en
    +  # Phone is here since it does not use a migrate field plugin.
    +  source_module: phone
     process:
       entity_type: 'constants/entity_type'
       status: active
    diff --git a/core/modules/field/migrations/state/field.migrate_drupal.yml b/core/modules/field/migrations/state/field.migrate_drupal.yml
    new file mode 100644
    index 0000000000..53c6eb61e2
    --- /dev/null
    +++ b/core/modules/field/migrations/state/field.migrate_drupal.yml
    @@ -0,0 +1,12 @@
    +finished:
    +  6:
    +    content: field
    +    email: core
    +    # Phone does not use a migrate field plugin so it is added here.
    +    phone: field
    +  7:
    +    email: core
    +    entityreference: core
    +    field: field
    +    field_sql_storage: field
    +    number: core

I will mention the disputed change in the issue summary.

I also cannot think of any reason why the field module’s plugin, which includes handling for several other modules (nodereference, number_integer, email, …) in the process section, should declare one of them as its source module.

I was not sure whether this change was related to this issue or an unrelated fix, like the fix mentioned in #10. I tried restoring the two lines to d6_field.yml, and several of the kernel tests in the field module failed. This change is in scope.

In short, I do not see any reason to disagree with #21. Of course, @quietone is far more familiar with both issues than I am.

Back to RTBC.

Committed and pushed 12df2086f0 to 10.1.x and 397b8d5959 to 10.0.x and 8ca4362795 to 9.5.x. Thanks!

Backported this to 9.5.x since this task is low risk and improves the migration system.