Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
system_requirements() includes this:
$version = str_replace(\Drupal::CORE_COMPATIBILITY . '-', '', $required_file->info['version']);
...
'description' => t('@name requires this module and version. Currently using @required_name version @version', ['@name' => $name, '@required_name' => $required_name, '@version' => $version]),
\Drupal::CORE_COMPATIBILITY is "8.x".
So we str_replace() "8.x-" with an empty string.
However, we're not careful about doing so only at the beginning of the version string.
So, if you somehow are dealing with a version string that contains '8.x-' somewhere you don't expect, all hell breaks loose.
Reasons you might have a version string that will be wrongfully clobbered include:
- A contrib module is upto 8.x-8.x branch and you're using the -dev release.
- You applied #3093130: Between alpha1 and X.Y.0 official \Drupal::VERSION should not be "8.8.0-dev" since it doesn't work for Semver::satisfies() and core's version is 8.8.x-dev
- You're using git_deploy and a Git checkout of core, in which case core thinks its version is 8.8.x-dev (I think).
- ...
Proposed resolution
If we have to strip this off, make sure we only do it at the beginning of the version string.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | 3093789.4_5.interdiff.txt | 644 bytes | dww |
| #5 | 3093789-5.test-and-fix.patch | 3.73 KB | dww |
| #4 | 3093789-4.test-only-will-fail.patch | 3.01 KB | dww |
Comments
Comment #2
dwwSomething like this.
Comment #3
tedbow@dww good catch.
Looks like we also do this in
\Drupal\system\Form\ModulesListForm::buildRowComment #4
dwwWow, this is all kinds of broken. :/ I started trying to write a test for this, but it's just uncovering deeper problems. :( Here's a test that should pass, but fails. The fix in #2 isn't sufficient, since we also do similarly unsafe things in
core/lib/Drupal/Component/Version/Constraint.php. That code assumes that '8.x-' is optional. So it ignores it. But callers (likesystem_requirements()and what you posted in #3) are already stripping it off. So if you start with a module at 8.x-8.x-dev,system_requirements()invokesisCompatible()with '8.x-dev', which thepreg_match()insideparseConstraint()treats as 'dev'. :(tl;dr woe be unto you if you reach minor version 8.x
Comment #5
dwwBased on my findings in #4, here's a different/better fix that gets the test from #4 to pass: stop stripping core compatibility *at all* in
system_requirements(). ;) This has the side effect of fixing #3093453: [PP-1] system_requirements() prints out a bogus version string when checking newly added dependencies for free. ;)Maybe still NW for #3, but that's perhaps out of scope and better handled in a separate issue.
Comment #7
lotyrin commentedComment #8
dwwI re-queued #5 for both 8.8.x and 9.0.x.
Also, spun off #3 to #3094979: ModulesListForm::buildRow() should more carefully strip core compatibility from version strings to keep the scope small and clear in each issue.
@lotyrin re: #7 thanks for the RTBC! However, for it to "count" I invite you to provide some reasons for changing the status. Did you review the code? Did you test it? Core committers care about why you think something is RTBC.
Thanks,
-Derek
Comment #9
dwwUgh. #3094979-3: ModulesListForm::buildRow() should more carefully strip core compatibility from version strings wasn't supposed to fail, but it did. :(
Looking deeper, my investigation in #4 wasn't fully accurate.
While
Drupal\Component\Version\Constraint::parseConstraint()handles the core compatibility optionally,Drupal\Component\Version\Constraint::isCompatible()does not. It looks likeisCompatible()always expects the core compatibility to be stripped (although it's not really documented as such). It's extra confusing since theConstraintconstructor takes a core compatibility param, but it only uses it forparseConstraint()and doesn't save it for later. ButisCompatible()assumes whatever you're passing is the same core compatibility as the constraint you started with. Fun!parseConstraint()has a comment that says:But that doesn't always seem to be true.
I don't have time right now to dig deeper, but some initial manual tests instantiating various
Constraintobjects and callingisCompatible()on them isn't giving expected results when using version strings involving8.x-8.x*.We almost certainly want to add some more cases to
core/tests/Drupal/Tests/Component/Version/ConstraintTest.phpabout this.It's kinda sad, since we're trying to kill off "core compatibility" entirely. But our constraint/version handling code is definitely buggy around the edges with it, and we probably need to fix that before we can get rid of it.
Alas,
-Derek
Comment #10
berdirSomewhat related #3124892: Error message Notice: Undefined index: version in system_requirements() for custom themes that do not define a version. Possibly we could check for an empty version as well here while changing things?
Comment #17
nicxvan commentedPostponing on #3493718: Convert system_requirements() into OOP hooks and install time class
Comment #18
larowlanBlocker is in