DrupalSqlBase::checkRequirements should test version with $minimum_version

1. Based on the code comments, I agree with your description of the problem. Both $minimum_version and 'minimum_schema_version' were intended to do the same thing, and they should have bee named the same. But one is a class property and the other is a key in the plugin definition. (In Drupal\Component\Plugin\PluginBase::__construct(), $plugin_definition is a parameter, assigned to the class property $pluginDefinition.) They are not logically related: the results of your grep experiments show that.

2. Both SqlBase and DrupalSqlBase are base classes, so we have to consider the implications for child classes. Can we simply ignore the minimum_schema_version key if it is defined in a derived source plugin? Suppose someone writing a contrib or custom source plugin needed a version constraint. First she read the documentation and used the property $minimum_version. That had no effect. So she looked at the code and added 'minimum_schema_version' to the plugin definition. If we adopt this patch, then we are breaking the check needed for that contrib or custom module.

   I think, if we want to make this change, we have to make sure that either 'minimum_version' or 'minimum_schema_version' version will work and throw a deprecation notice in the latter case.

3. We might also want to set $this->minimum_version = $this->pluginDefinition['minimum_version'] ?? 0; in the constructor and then use the property in checkRequirements(). Or default to -1? This would have the advantage that we would not have to check isset($this->pluginDefinition['minimum_version']) in checkRequirements().

4. Thanks for the explanation of the multiple changes in this line:

    +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/DrupalSqlBase.php
    @@ -105,8 +105,8 @@ public function checkRequirements() {
    ...
    -          if (isset($this->pluginDefinition['minimum_schema_version']) && !$this->getModuleSchemaVersion($this->pluginDefinition['source_module']) < $this->pluginDefinition['minimum_schema_version']) {
    +          if (isset($this->pluginDefinition['minimum_version']) && !($this->getModuleSchemaVersion($this->pluginDefinition['source_module']) <= $this->pluginDefinition['minimum_version'])) {

   I often ask for shorter lines of code, and this illustrates one of the reasons. When you make three changes to the same line of code, it is hard to review. (I am counting as one change s/minimum_schema_version/minimum_version/g even though the string appears twice. The other two changes are adding parentheses and changing < to <=.)

   Can we replace !(expr1 <= expr2) with expr 1 > expr2? This is not a trivial question, since getModuleSchemaVersion() can return FALSE. Assuming that the minimum version is an integer (if it is set at all), I think that FALSE will be cast to 0, so the answer is that we can.

   Am I confused, or is the inequality backwards? Looking at the test you added, I think I have it right. The test sets a minimum version of 7000 and an actual version of 7001 and expects a RequirementsException to be thrown. This does not make sense.

   Given all those questions, I consider it in scope for this issue to rewrite this block of code so that it has more, shorter lines that are easier to understand and maintain. The final conditional could be something like if ($actual_version < $minimum_version). At the same time, I think we can simplify the code by rearranging the cases and exiting early.

5. The two test methods testMinimumVersionFail() and testMinimumVersionSuccess() are identical except for some extra blank lines in the second and the last 1 or 3 lines. Can we combine them into one test with a parameter for success/fail? Alternatively, add a helper method that is called by both test methods.

On second thought: I care less about backward compatibility (#5.2) given that the current behavior is so badly broken (#5.4).

1. This should work, and you can leave it like this if you prefer:

    +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/DrupalSqlBase.php
    @@ -105,8 +105,10 @@ public function checkRequirements() {
    ...
    +          // Check that the minimum schema version is not greater than the
    +          // current schema for the source module.
    +          if (isset($this->pluginDefinition['minimum_version']) && ($this->pluginDefinition['minimum_version'] > $this->getModuleSchemaVersion($this->pluginDefinition['source_module']))) {
    +            throw new RequirementsException('Required minimum version ' . $this->pluginDefinition['minimum_version'], ['minimum_version' => $this->pluginDefinition['minimum_version']]);

   But what I was suggesting at the end of #5.4 was something like this (inside enough if statements to make it safe):

    $minimum_version = $this->pluginDefinition['minimum_version'];
    $installed_version = $this->getModuleSchemaVersion($this->pluginDefinition['source_module']);
    if ($minimum_version > $installed_version) {
      throw ...
    }

   If we do that, then I think the code is clear enough that we do not need the comment. I think the reason this bug got here in the first place is that the line was so long and confusing that no one noticed the backwards comparison until now. The shorter lines of code also mean that we are unlikely to need an explanation like the one in #2 about multiple changes to one line of code. That is why I describe the shorter lines as "easier to understand and maintain".

2. There are already two test methods in this class. Now that we are adding a third, we could decide that DRY is in scope. If you would rather defer that to a follow-up issue, we can. I want a follow-up issue anyway to remove the second namespace line and put all the use statements at the top. These lines could be moved to a helper method:

    +++ b/core/modules/migrate_drupal/tests/src/Unit/source/DrupalSqlBaseTest.php
    @@ -79,6 +79,83 @@ public function testSourceDatabaseError() {
    ...
    +  public function testMinimumVersion($success, $minimum_version, $schema_version) {
    ...
    +    /** @var \Drupal\Core\State\StateInterface $state */
    +    $state = $this->createMock('Drupal\Core\State\StateInterface');
    +    /** @var \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager */
    +    $entity_type_manager = $this->createMock('Drupal\Core\Entity\EntityTypeManagerInterface');
    +    $plugin = new TestDrupalSqlBase([], 'test', $plugin_definition, $this->getMigration(), $state, $entity_type_manager);

   Maybe we can even use a setUp() method, if the plugin definition can be set with a public method.

3. Can we use MigrateTestCase::getDatabase(), same as testSourceProviderNotActive()?

    +    // Setup a connection object.
    +    $source_connection = $this->getMockBuilder('Drupal\Core\Database\Connection')
    +      ->disableOriginalConstructor()
    +      ->getMock();
    +    $source_options = ['driver' => 'mysql'];
    +    $source_connection->expects($this->never())
    +      ->method('getConnectionOptions')
    +      ->willReturn($source_options);
    +    $plugin->setDatabase($source_connection);

4. Add a blank line after that block to separate it from the next lines:

    +    if (!$success) {
    +      $this->expectException(RequirementsException::class);
    +      $this->expectExceptionMessage("Required minimum version $minimum_version");
    +    }
    +    $plugin->checkRequirements();

5. We cannot use {@inheritdoc} here:

    @@ -123,4 +200,11 @@ public function getIds() {
    ...
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function setSystemData($data) {
    +    $this->systemData = $data;
    +  }

@quietone, thanks for the updates. I think both the test code and the "real" code are easier to read now.

I added a follow-up issue: #3191990: Simplify code in DrupalSqlBaseTest. I will also add a draft change record.

Getting back to #5.1, #5.3: there is no connection between the class property $minimum_version and the 'mimimum_version' key in the plugin definition. In #5.3, I suggested setting the property in the constructor based on the plugin definition. If we do that, then we can save one test in checkRequirements(). If we do not do that, then maybe we should deprecate the class property, since we do not use it anywhere. Any child class that wants to use it can declare it.

Can you upload a test-only patch? I am not sure how to test this issue, so I would like to see the failures in the test-only patch before calling it RTBC.

In #5.1 and #10, I was mixed up: minimum_version is declared in the annotation class. I was thinking it was a property of MigrateSource. I straightened myself out while drafting a CR.

Still NW for a test-only patch.

@quietone:

Sorry, I did not notice this until I was reviewing the failing tests in the test-only patch: we need some @param comments for testMinimumVersion().

Looks good. Thanks again!

Committed/pushed to 9.2.x and cherry-picked to 9.1.x, thanks!

Published CR. Updated CR to say it shipped not only with https://www.drupal.org/project/drupal/releases/9.2.0 but also https://www.drupal.org/project/drupal/releases/9.1.4.