Module version dependency in .info.yml is ineffective for patch releases

Does this help?

Re: #3, that could be by design because all patch releases are backwards-compatible.

I am tagging this "documentation" because I agree this feature seems undocumented online.

Patch releases are supposed to be BC, but could fix bugs. It's entirely legit for a module to depend on "Drupal 8.1.2, because 8.1.1 had a bug in it that made this thing not work." So I wouldn't call this by-design. It's just a straight up bug.

Patch version is ignored when parsing dependencies. Attached a patch that takes it into account along with a test.

Btw Drush version parser is independent of Drupal's one. It is at http://api.drush.org/api/drush/commands!pm!pm.drush.inc/function/pm_pars...

Note also that Drupal is using major-minor nomenclature in code, while it is documented that contrib modules uses major-patch -> https://www.drupal.org/node/467026

1. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -677,12 +677,14 @@ public static function parseDependency($dependency) {
   +    // @todo@ if $p_minor is 'x', then there's no patch version for sure. Since we don't support 8.x.1 and 8.x.x is redundant with core compatibility check.
   

   What's TODO here? Also comments should wrap at 80 char line.

2. +++ b/core/modules/system/src/Tests/Module/DependencyTest.php
   @@ -82,6 +82,21 @@ function testIncompatibleModuleVersionDependency() {
   +    // Test that the system_incompatible_module_semantic_version_dependencies_test is
   +    // marked as having an incompatible dependency.
   

   Again, comments should wrap at 80 line, in this case "is" should be moved to the next line.

3. +++ b/core/modules/system/src/Tests/Module/DependencyTest.php
   @@ -82,6 +82,21 @@ function testIncompatibleModuleVersionDependency() {
   +      '@module' => 'System (>= 8.1.999)',
   +      '@version' => \Drupal::VERSION,
   
   +++ b/core/modules/system/tests/modules/system_incompatible_module_semantic_version_dependencies_test/system_incompatible_module_semantic_version_dependencies_test.info.yml
   @@ -0,0 +1,8 @@
   +name: 'System incompatible module semantic version dependencies test'
   +type: module
   +description: 'Support module for testing system dependencies.'
   +package: Testing
   +version: VERSION
   +core: 8.x
   +dependencies:
   +  - 'drupal:system (>= 8.1.999)'
   

   This tactic will break in time, meaning we will have to update this in every 6 months. It's not horrible, but would be nice if we can make a test that will just work. Either we would have to alter the \Drupal::VERSION somehow or dynamically set/alter the dependency in the from the info.yml file.

#13.1 I added this @todo@ to note that, although it works, we're not being algorithmically accurate. See comment #12

#13.2 I wonder if a shorten module name is better (system_incompatible_module_semver_dependencies_test)

#13.3 It won't fail because Drupal::VERSION is 8.0.0-dev even in 8.1.x branch - http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal.php?h=8.1.x#n81. I don't know if it is intended or a bug.
Just in case, we want to alter the dependency (via hook_system_info_alter()), not Drupal::VERSION.

Regarding #13.1

If we actually want to make a @todo, general practice is to use "@todo" not
@todo@", it should have a link to an issue containing an open task on the todo (so we know it's going to be done eventually) or it should be solved in this ticket. The comment should state what's left "to do".
In this case, if we want to algorithmically accurate, we should do it, if we feel like it's not needed or would make the code unnecessary complex, then there's no @todo here, but a comment explaining the reasoning would be needed.

Regarding #13.2 It's ok that the name spans over 80 chars when it's really long, but the "is" after it should be moved to the line after.

Regarding #13.3 It seems odd that 8.0.0-dev is used for all 8.x.x releases, but still it's just a matter of time (9.x.x) before the test will fail. We might as well make this future proof, so we don't make tests that will fail when the Drupal version number changes.

Huh, the module handler doesn't use version_compare()? And I went to all that effort testing with it in #2656994: Experimental modules should have their own version numbers. ;) It would be worth investigating if and why it's not used.

For me this is legitimately a bug, since compatibility with patch releases is something that will be needed from time to time. For example, say a certain contrib module is blocked on a patch-release-safe bug, but will destroy your data horribly before that bug is fixed. Totally sensible to require the patch release as the minimum allowed version.

ModuleHandler::parseDependency() does not use version_compare(), with this inline comment:

          // Drupal considers "2.x" to mean any version that begins with
          // "2" (e.g. 2.0, 2.9 are all "2.x"). PHP's version_compare(),
          // on the other hand, treats "x" as a string; so to
          // version_compare(), "2.x" is considered less than 2.0. This
          // means that >=2.x and <2.x are handled by version_compare()
          // as we need, but > and <= are not.

It won't fail because Drupal::VERSION is 8.0.0-dev even in 8.1.x branch - http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal.php?h=8.1.x#n81. I don't know if it is intended or a bug.

That is, or was, a bug. A release manager who rebased 8.1.x clearly did not fix this when she did so. ;) It was fixed in #2640650: Update the version number from 8.0.x to 8.1.x.

As a side note, I had to pretty much re-do parse_dependency logic for the composer façade:
http://cgit.drupalcode.org/project_composer/tree/project_composer.module...
And have a test built for it:
http://cgit.drupalcode.org/project_composer/tree/project_composer.test#n54

Perhaps this can be adapted to be something used by both core and project_composer?

#21++

We're seeing this become a problem for D8 contrib modules, that need a particular minimum version of Drupal because prior versions contained show-stopping bugs.

Examples:

1. #2730295: Remove custom cache context services after QueryArgsCacheContext can handle arguments without values, require Drupal 8.1.2
2. #2711529: File upload widget broken when using CDN module, fixed in Drupal 8.1.4: require that version

Also, the D8 docs in https://www.drupal.org/node/2000204 do not document whether it is possible to specify a minimum version for the core: property (as it is with the php: property). Is that possible?

While the hack in the OP should probably work, too – it has the UX & look & feel of a hack.

The php property maps to a composer require php:x.y.z. The core property would map to a composer require drupal/core:x.y.z. No?

In any case, I would recommend module maintainers to specify the composer.json properties, as that prevents an incompatible version from being downloaded into the working directory in the first place.

I think this is currently the most-immediate blocker to Drupal.org allowing semantic versioning for contrib releases.

Re: #21: We could move project_composer_parse_dependency() to a core component. This would allow project_composer to depend on it, and libraries as per #23.

Then we can fix this issue by using composer/semver after a conversion.

Is that somewhat what this #2677532: Move drupal_check_incompatibility() functionality to a new Dependency class and Version component is trying to do ? I mean, it moves it to 'its own class' but maybe we should make it a component from the getgo.

:-) See #2677532-6: Move drupal_check_incompatibility() functionality to a new Dependency class and Version component

@Mixologic suggested in #2807145: [policy, no patch] Allow contrib projects to specify multiple major core branches that we postpone that one on this, so I did so. Especially if we are going with #21 which would help us do the same logic in Drupal and on drupal.org.

Also we should postpone this on #2677532: Move drupal_check_incompatibility() functionality to a new Dependency class and Version component as per above which is RTBC now!

#2677532: Move drupal_check_incompatibility() functionality to a new Dependency class and Version component is now in

There's a new META in town.

This issue is blocking (at least) the Automatic Updates, Composer in Core, and Drupal 9 initiatives. After talking to @xjm, escalating to critical.

I think @webchick meant to change the priority too. :-)

Re-roll of patch

The test only patch, btw it looks tricky because require to point exact version

So what about #21?

+++ b/core/lib/Drupal/Component/Version/Constraint.php
@@ -108,8 +108,10 @@ private function parseConstraint($constraint_string, $core_compatibility) {
+    // @todo@ if $p_minor is 'x', then there's no patch version for sure. Since we don't support 8.x.1 and 8.x.x is redundant with core compatibility check.

What is to do about this? The comment seem to list facts only.

1. re #44 in #14 @jonhattan refers to #12 to explain this
   maybe referring to

   Note also that Drupal is using major-minor nomenclature in code, while it is documented that contrib modules uses major-patch

   But although we don't support "support" 8.x.1 or 8.x.x basically semantic versions with "x" in minor and a patch supplied this doe actually work in \Drupal\Component\Version\Constraint::isCompatible().

   I am providing test-only patch for \Drupal\Tests\Component\Version\ConstraintTest to prove this.

   I don't think the current patch will change this.

   Should we make a follow up to enforce validation?

2. I have added these tests to the new patch to prove this doesn't change with the new regex
3. Added tests cases for semantic versioning to \Drupal\Tests\Component\Version\ConstraintTest::providerIsCompatible() because even though we are calling PHP's version_compare if we got the regex wrong this could cause unexpected results so this should be well tested.
4. Re #42
   

   The test only patch, btw it looks tricky because require to point exact version

   Yes since it relies on \Drupal::VERSION we would have to update the test module and test with every patch version.
   To avoid this we should do something like \Drupal\Tests\system\Functional\Module\VersionTest::testModuleVersions() and use system_test_system_info_alter to alter the version number of the system module and the dependency string. This allows removing the system_incompatible_module_semantic_version_dependencies_test because we can just alter the dependency of the common_test module like VersionTest does

5. +++ b/core/modules/system/tests/src/Functional/Module/DependencyTest.php
   @@ -78,6 +78,21 @@ public function testIncompatibleModuleVersionDependency() {
   +    $this->assertRaw(t('@module (<span class="admin-missing">incompatible with</span> version @version)', [
   +      '@module' => 'System (>= 8.7.1)',
   +      '@version' => \Drupal::VERSION,
   +    ]), 'A module that depends on an incompatible semantic version of a module is marked as such.');
   

   We use elementContains() here to make sure the correct module has this message in the off chance we add a test module in the future that have a similar message and produce a false positive for this test.

6. +++ b/core/modules/system/tests/src/Functional/Module/DependencyTest.php
   @@ -78,6 +78,21 @@ public function testIncompatibleModuleVersionDependency() {
   +    $checkbox = $this->xpath('//input[@type="checkbox" and @disabled="disabled" and @name="modules[system_incompatible_module_semantic_version_dependencies_test][enable]"]');
   +    $this->assert(count($checkbox) == 1, 'Checkbox for the module is disabled.');
   

   This is copied from other test methods in the class but we can now just replace this with $this->assertSession()->fieldDisabled('edit-modules-common-test-enable'); which is more readable.

The drupalci changes to the test-only patch were wrong.

Wanted to bring in couple related issues

1. #3066448: Harden test coverage for dependency Constraint class I don't think we should actually be making changes to \Drupal\Component\Version\Constraint because test coverage in \Drupal\Tests\Component\Version\ConstraintTest is so thin we could easily break functionality without knowing it.
   It would be great to get that in first
2. #3004459: [PP-1] Fold in dependency parsing wisdom from project_composer(postponed on above) This change would actually also fix all the test cases added in this issue but in different. We need either this fix or that one, not both. That one relies on \Composer\Semver\Semver::satisfies() instead of version_compare() which requires parsing our constraint strings into a Composer format so that is bigger(more risky?) change. But it does get rid of a lot of own regex

I think @sun's observation in #28 belongs in the issue summary:

The php property maps to a composer require php:x.y.z. The core property would map to a composer require drupal/core:x.y.z.

That would make it crystal clear and gets us on a path where we can migrate towards composer more easily in the future!

I haven't updated the issue summary yet because I don't know if there's consensus about this.

#45: 👏 Thanks for getting this issue back on track!

#47:

1. 👍
2. Sounds like we need consensus on this too. It seems prudent to use #3004459, because that's been proven in production over the past few years? And in reading related issues, I was going to ask: "why are we even writing our own logic if we can just reuse the logic Composer has, and you mention #3004459 relies on \Composer\Semver\Semver::satisfies(). So that answers my question. And makes the choice pretty clear to me: we should land #3004459: [PP-1] Fold in dependency parsing wisdom from project_composer, which uses Composer's logic, which removes the need for the fixes in this issue.

Hah, now reviewing this to assist @tedbow, and in reading the entire issue I come across a comment of mine from three years ago 😁 (#22)

@Wim Leers thanks for the review

1. re #48.2 I don't #3004459: [PP-1] Fold in dependency parsing wisdom from project_composer is the better option because it introduces a complex method for converting our constraint strings into composer constraint strings so even though it uses \Composer\Semver\Semver::satisfies() it introduces more complexity to get to that simplicity.
2. This patch tries a different approach. Basically it first tries uses \Composer\Semver\Semver::satisfies on the constraint string directly if it is not valid this will throw a UnexpectedValueException and then we use existing logic. This requires no changes to \Drupal\Component\Version\Constraint::parseConstraint()

   It would make the upgrade to #3005229: Provide optional support for using composer.json for dependency metadata as modules could start to use constraints that could be moved to composer.json directly.

   Also it doesn't try to support semantic versioning and our drupal version format in the same logic.

1. +++ b/core/lib/Drupal/Component/Version/Constraint.php
   @@ -82,12 +84,19 @@ public function toArray() {
   +    catch (\UnexpectedValueException $exception) {
   +      foreach ($this->constraintArray as $constraint) {
   +        if (!version_compare($version, $constraint['version'], $constraint['op'])) {
   +          return FALSE;
   +        }
          }
   +      return TRUE;
        }
   

   One option to get away from own parsing would be to deprecate our own format which would basically mean just returning with in this catch.

   If at some point we deprecated our Drupal specific format we could display a message about being incompatible or just do a scan on the status report page for any constraints that thrown an exception in (new VersionParser())->parseConstraints($constraint).

2. +++ b/core/tests/Drupal/Tests/Component/Version/ConstraintTest.php
   @@ -88,6 +88,62 @@ public function providerIsCompatible() {
   +    // Test semantic minor with "x" which we should NOT support.
   +    $semantic_less_or_equal_minor_x = new Constraint('>= 8.x.x', '8.x');
   +    $tests['>= 8.x.x-8.7.x'] = [$semantic_less_or_equal_minor_x, '8.7.0', TRUE];
   +
   +    // Test semantic minor with "x" which we should NOT support.
   +    $semantic_less_or_equal_minor_x = new Constraint('>= 8.x.1', '8.x');
   +    $tests['>= 8.x.1-8.7.x'] = [$semantic_less_or_equal_minor_x, '8.7.0', TRUE];
   +
   +    // Test semantic minor with "x" which we should NOT support.
   +    $semantic_greater_or_equal_minor_x = new Constraint('<= 8.x.x', '8.x');
   +    $tests['<= 8.x.x-8.7.x'] = [$semantic_greater_or_equal_minor_x, '8.7.0', TRUE];
   +
   +    // Test semantic minor with "x" which we should NOT support.
   +    $semantic_greater_or_equal_minor_x = new Constraint('<= 8.x.1', '8.x');
   +    $tests['<= 8.x.1-8.7.x'] = [$semantic_greater_or_equal_minor_x, '8.7.0', TRUE];
   

   Removing these test cases. I am actually sure if this is problem. We generally seem to return TRUE if the constraint is nonsense which seems like the real problem.

   It think this just points to the fact that we should get away from our own parsing.

3. +++ b/core/modules/system/tests/src/Functional/Module/DependencyTest.php
   @@ -78,6 +78,26 @@ public function testIncompatibleModuleVersionDependency() {
   +    // Test that the
   +    // system_incompatible_module_semantic_version_dependencies_test is marked
   

   Incorrect module name

4. +++ b/core/modules/system/tests/src/Functional/Module/DependencyTest.php
   @@ -78,6 +78,26 @@ public function testIncompatibleModuleVersionDependency() {
   +    $this->assertSession()->fieldDisabled('edit-modules-common-test-enable');
   

   Adding a test where we have semantic version constraint that passes.

@Mixologic commented on #3069795-11: [meta] Improve dependency management for extensions as to why the approach in #49 would be a problem

This is a weird case of "the strings and their formats in the dependencies key are a defacto API that packages.drupal.org depends on, please don't change whats possible there".

If we start adding new things to .info.yml, we'll need to adjust the project_composer/drupal.org to expect any/all of these new possibilities and handle them all properly, without a clear indicator as to which .info.yml's are in the deprecated format and which ones should be just passed through.

+++ b/core/lib/Drupal/Component/Version/Constraint.php
@@ -82,12 +84,19 @@ public function toArray() {
+      // If the constraint given is a valid composer semantic version constraint
+      // then use it directly.
+      return Semver::satisfies($version, $this->constraint);

This would introduce the possibility to use ~/^ and probably other composer format changes that I am not thinking of right now.

So we don't want to do this in Drupal core unless it is conjunction with changes on drupal.org/project_composer.

There a few options then

1. Don't actually fix this issue. #2313917: Core version key in module's .info.yml doesn't respect core semantic versioning will solve the problem by making it - drupal:system (>= 8.0.1) actually not needed because you could just use the core: key. Others semantic versioning in dependencies is not actually needed until #3054391: [META] Support semantic versioning on drupal.org for contributed projects and if by that time #3005229: Provide optional support for using composer.json for dependency metadata is committed we could rely on that.
2. Use the approach in #49 but do something like
   

   $this->validateConstraintOnlyUsesBCCompitableComposerFormat();
   return Semver::satisfies($version, $this->constraint);
   

3. Just use fix in #45 and before. This is probably the least disruptive change.

Related, I wanted to see how many contrib `dependencies` constraints actually would throw an error if used with `\Composer\Semver\VersionParser::parseConstraints()`. So I made https://github.com/tedbow/drupal_contrib_dependencies. tldr, 99% don't throw an error.
.

re #50.2

Created #3071150: Dependency version constraints evaluate true even the constraint strings aren't valid

Now this is duplicate of #2313917: Core version key in module's .info.yml doesn't respect core semantic versioning. See #3069795-14: [meta] Improve dependency management for extensions for details and related issues

See #51.1 for details here