Problem/Motivation

Originally discovered at #3093789-3: system_requirements() strips core compatibility in really fragile and dangerous way when comparing versions

core/modules/system/src/Form/ModulesListForm.php has a buildRow() method. It's currently doing this:

if (!$dependency_object->isCompatible(str_replace(\Drupal::CORE_COMPATIBILITY . '-', '', $modules[$dependency]->info['version']))) {
  ...
}

This is problematic for a few reasons:

  1. str_replace() isn't careful about where in the version string it's stripping 8.x- from.
  2. \Drupal\Core\Extension\Dependency::isCompatible() already gracefully handles version strings with a core compatibility part. Stripping anything (even if we were more careful and using preg_replace() or something) risks comparing against unintended versions.
  3. If you happen to be on version 8.x-8.x-dev of a given module, you end up with a version string of just dev which completely breaks all the intended logic.

Proposed resolution

Stop stripping the \Drupal::CORE_COMPATIBILITY part of version strings before using \Drupal\Core\Extension\Dependency::isCompatible().

Remaining tasks

  1. Do it.
  2. Make sure the test coverage is good.
  3. Review.
  4. RTBC.
  5. Commit.

User interface changes

-

API changes

-

Data model changes

-

Release notes snippet

-

CommentFileSizeAuthor
#5 3094979-5.patch1.51 KBdww
#3 3094979-2.patch1004 bytesdww

Comments

dww created an issue. See original summary.

dww credited tedbow.

dww’s picture

dww
we/he/they
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 3094979-2.patch1004 bytes

I haven't looked into the test coverage yet, but here's a patch to get us started.

Also, crediting @tedbow who ran grep in the other issue to find this bug. ;)

Status: Needs review » Needs work

The last submitted patch, 3: 3094979-2.patch, failed testing. View results

dww’s picture

dww
we/he/they
commented
Status: Needs work » Needs review
Issue tags: +Needs tests
StatusFileSize
new 3094979-5.patch1.51 KB

Ugh, I was wrong at #3093789-4. See #3093789-9: system_requirements() strips core compatibility in really fragile and dangerous way when comparing versions for more.

This will pass. Still needs improved test coverage.

The whole patch is different, interdiff would be meaningless.

dww’s picture

dww
we/he/they
commented
Title: ModulesListForm::buildRow() should not strip core compatibility from version strings » ModulesListForm::buildRow() should more carefully strip core compatibility from version strings

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

joachim’s picture

joachim commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/system/src/Form/ModulesListForm.php
@@ -335,7 +335,15 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
+        if ($dependency == 'Common Test') {
+          echo var_dump($dependency_object);
+        }

    Is this stray debug code? 

  2. +++ b/core/modules/system/src/Form/ModulesListForm.php
@@ -335,7 +335,15 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
+        $core_stripped_version = preg_replace('/^' . \Drupal::CORE_COMPATIBILITY . '-/', '', $modules[$dependency]->info['version']);

    Perhaps out of scope and better be done in a follow-up, but I think this fairly messy mucking about with version numbers would be nicer if it were wrapped up in a helper, maybe a static method on Drupal\Core\Extension\Extension?

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

dcam’s picture

dcam commented
Status: Needs work » Closed (outdated)
Issue tags: +stale-issue-cleanup

This code got moved around, but ultimately #3086845: Module constraint checks fail incorrectly due to str_replace applied the exact same proposed fix to it. This issue is outdated.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.