Problem/Motivation
Drupal modules and themes need to be able to specify that they depend on specific versions of Drupal because a new minor version of Drupal may introduce new APIs and a patch version may fix bugs. Drupal modules and themes also should be able to specify if they are compatible with 2 major versions of Drupal such as 8 and 9, if they don't use any APIs that will be removed in Drupal 9. Modules, install profiles and themes should not have to create a new major branches to achieve this as per #2608062: [META] Requirements for tagging Drupal 9.0.0-alpha1.
For Drupal 8 projects, currently the core
key in info.yml
files only supports core: 8.x
which denotes that they support any version of Drupal 8. To get around this limitation many modules use the dependencies:
key in info.yml
files with a more specific version of system module. For example
- "drupal:system (>8.6)"
. But this method is not enough because it does not support patch releases either, see #2641658: Module version dependency in .info.yml is ineffective for patch releases.
While we could change the core:
key to accept semantic version constraints to allow requiring specific core patch versions, for example core: ^8.7
, or even support multiple branch compatibility (core: ^8 || ^9
), or both (core: ^8.7 || ^9
) this would cause problems for any sites using previous versions of Drupal 8 core, if they attempted to update a module they currently have installed that switched to the new format.
A Drupal 8.x.y site that has my_module 8.x-1.1 that uses core: 8.x
and then updates to my_module 8.x-1.2 that uses core: ^8
(anything beside 8.x) will have this this module silently disabled, even while the configuration for the module would still be available. There would be no message that this happened. See #138 and #2917600: update_fix_compatibility() puts sites into unrecoverable state.
Proposed resolution
As a consequence of the above, we need to introduce a new key core_version_requirement:
that would accept Composer semantic version constraints. The version constraints will be evaluated by \Composer\Semver\Semver::satisfies()
. The rules for this can be found at https://getcomposer.org/doc/articles/versions.md.
This key could be used at the same time as the core:
key. But in certain cases it would be used in place of the core:
key. To make it possible to have modules be compatible Drupal 8.7.x, 8.8.x, 8.9.x and 9.0.x and maximize the duration in which Drupal 9 readiness can be worked on and ensure that security fixes for contrib modules don't force sites still on Drupal 8.7.x to upgrade to Drupal 8.8.x (if the module doing a security release already started using core_version_requirement
). See reasoning in #179.3 and release manager approval in #207.
Assuming the 8.7 patch release in which this issue ships is 8.7.7, the following combination would not produce the expected results info.yml
file:
core: 8.x
core_version_requirement: ^8.7.7
This would not actually restrict anyone from installing it on Drupal 8.6.x, or 8.7.0 through 8.7.6 since the core_version_requirement
key would be ignored and would therefore still allow the module to be installed. We cover this case by not allowing the core:
key once a core_version_requirement
key requires Drupal 8.7.7 or later. Not having the core:
key will give a hard fail in Drupal 8.7.x sites but because it fails in \Drupal\Core\Extension\InfoParserDynamic::parse
this would not cause a problem in #138 and #2917600: update_fix_compatibility() puts sites into unrecoverable state.
Another combination that would not produce the expected results:
core: 8.x
core_version_requirement: ^8.7.4
In this case Drupal 8.7.3 would not enforce core_version_requirement: ^8.7.4
. We enforce this by not allowing restrictions that cover versions before this issue is committed because these will not actually be enforced.
In both these cases it would be hard for a contrib author to be aware for these problems if they are working on Drupal 8.8.x unless they also tested their module on previous versions of Drupal. Therefore we should add validation to \Drupal\Core\Extension\InfoParserDynamic::parse()
to make sure that module authors don't create info files with these problems.
Examples of supported core_version_requirement:
values:
core_version_requirement: value |
Evaluates to constraints | Comments |
---|---|---|
^8.8 | >= 8.8.0.0-dev < 9.0.0.0-dev | Recommended for modules that are only compatible with 8.8.x and above but not 9. |
^8 || ^9 | >= 8.0.0.0-dev < 10.0.0.0-dev | Recommended for modules that are compatible with both Drupal 8 & 9. Most modules aren't actually compatible with all versions of Drupal if removed deprecated API use and used the new replacements. |
Remaining tasks
User interface changes
Small string change for modules page.
API changes
Support core semantic versioning for Drupal core via new core_version_requirement
key.
Data model changes
None.
Release notes snippet
A new core_version_requirement
has been added to info.yml
files. This key replaces the core
key for projects that only support Drupal 8.8 onwards (including Drupal 9). It may be used in addition to the core
key for projects that want to specify compatibility with Drupal 9 as well (but remain compatible with prior versions of Drupal 8). Read more in the change record for the new core_version_requirement
key.
Comment | File | Size | Author |
---|---|---|---|
#252 | 2313917-252.patch | 5.52 KB | tedbow |
#252 | 2313917-252.patch | 5.52 KB | tedbow |
#239 | 2313917-follow-up-8.7.patch | 5.49 KB | tedbow |
#236 | 2313917-follow-up.patch | 5.61 KB | tedbow |
#226 | 2313917-226-8_7.patch | 47.87 KB | tedbow |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedComment #2
xjmCritical I think.
Comment #3
pwolanin CreditAttribution: pwolanin commentedComment #4
pwolanin CreditAttribution: pwolanin commentedComment #5
xjmA workaround for this (used in D7 in contrib) would be to declare dependencies on e.g. system > 8.1.0. Though that doesn't help with pre-8.0.0. And that might also be broken too...
Comment #6
pwolanin CreditAttribution: pwolanin commentedNew test - should fail.
Comment #8
pwolanin CreditAttribution: pwolanin commentedtest-only plus more complete patch.
Comment #11
pwolanin CreditAttribution: pwolanin commentedSilly me - gave a file the wrong name.
Comment #13
Gábor HojtsyThere are not even tests for core dealing with its own version numbers. @xjm pointed me here from #2170443-82: [meta] Create a plan for implementing semantic versioning for core but I think only as a related thing, not as saying this would resolve that.
Comment #14
Gábor HojtsyOpened #2315849: Update status does not have tests with (semantic) Drupal 8 versions for the core versions issue.
Comment #15
pwolanin CreditAttribution: pwolanin commentedIndeed - the code here should be re-factored to be more testable. it's kind of a mess and coupled to the forms.
I think the question is whether we should get this simple fix in first?
Comment #16
Gábor HojtsyIf we are to leave this todo in for commit, we need an issue opened and referenced.
Should we have a test for a 8.2.x or somesuch to test the other part of the condition?
Although that test would start failing in itself once we are on 8.1.x-dev... Not sure how best to test that then...
Needs work at least for the todo.
Comment #17
Gábor HojtsyOh, well, we can info alter the core version number in the test to be bigger than the current core version number, so it will always be passing.
Comment #18
Gábor HojtsyComment #19
xjmDiscussed with @alexpott and @effulgentsia. I think that technically this is a release blocker for 8.1.0 that we can and should but don't absolutely need to fix sooner, but not sure how to represent that in the issue queue. :P
Comment #20
star-szrminor version blocker
? :)Comment #21
jhedstromRather than moving
invalidCore()
to\Drupal
, this patch moves it to the module handler interface (I'm not sure that it matters, but since the method was specific to modules, I figured that was a starting point). I also added a unit test, but it hard-codes versions, meaning it will require updating as core versions change. This can most likely be refactored to be be always passing as Gabor suggests for the simpletests. Leaving at needs work for now.Comment #22
catchWe're supposed to have a strict six month release cycle for minor versions, so if we know something will block an 8.1.x release now, the only way we can guarantee it won't push out the six month cycle is to fix it before 8.0.0 - otherwise we start a countdown as soon as 8.0.0 is tagged. Similar problem with Drupal.org issues around 8.1.x if they come up.
In practice that's probably too strict but might as well keep things critical until they're the last things blocking, then evaluate what's remaining.
Comment #23
YesCT CreditAttribution: YesCT commented(to send #21 to the bot)
Comment #24
tstoecklerIt seems we should be updating Drupal::CORE_COMPATIBILIY as part of this issue, no?
Comment #25
dawehner@tstoeckler
Well, changing that would make stuff like
ModuleHandler::parseDependency()
not working anymore. In theory semv would not break APIs any APIsin between, so this won't be a problem.
It kinda sad that we can't reuse anything from our existing dependency system for modules.
Does someone know why we do use an
x
instead of an*
?Comment #26
Gábor Hojtsy@dawehner: as to why do we not use .*. that is at best historic IMHO.
@jhedstrom: this looks good but it should not start failing when a new core version is tagged (if it did not fail before).
Comment #27
dawehner@Gábor Hojtsy
It's primarily really confusing as the composer.json files look totally different.
Comment #28
Gábor Hojtsy@jhedstrom still want to work on this? :)
Comment #29
jhedstromThis patch fixes the unit test so it should continue to work as new versions are released. I'm not sure what to do about the test yaml files though.
Comment #30
Gábor HojtsyThe dynamic version test looks much better. Except:
$minor
in this patch is not a number becausesubstr('8.0.x', 1, 1)
is '.' no? Would be better to useexplode(\Drupal::VERSION, '.')[1]
no?Comment #31
jhedstromGood catch! This fixes that issue, and also switches to using a
dataProvider
method.Comment #32
Gábor HojtsyYay, looks great to me :)
Comment #33
alexpottWhat about this code for themes? In SystemController::themesPage() - it would be nice to handle extensions (modules, themes and install profiles) together somehow.
Comment #34
jhedstromCurrently there doesn't seem to be any commonality between ThemeHandler and ModuleHandler (and I can't find an equivalent for profiles). Would it make sense for these to all extend a common base class (ExtensionHandler perhaps) that deals with core compatibility? Alternatively, perhaps the compatibility logic could be moved to a trait?
Comment #35
markhalliwellComment #36
amateescu CreditAttribution: amateescu commentedI think this method name could be made a bit friendlier, I would have no idea what it does if it were being presented in an IDE autocomplete suggestion. How about isCoreCompatible() or isCompatibleWithCore()?
Doesn't look like a valid filename for a module file :)
Comment #37
jhedstromThis removes the erroneously added file mentioned in #36, and I think addresses the concern in #33.
Comment #38
jhedstromComment #39
amateescu CreditAttribution: amateescu commentedYep, I think #37 is the best we can do for now, until we have some common base class for modules, themes and profiles.
Comment #40
catchSorry I'm not sure whether we actually need this or not:
From #2135189: Proposal to manage the Drupal core release cycle
This means there's no valid reason to run contrib module on different minor versions of core - because only one of those versions will actually be supported.
The support makes sense in theory, but I wonder whether it could encourage bad behaviour in practice.
Comment #41
JamesOakleyBut what are "trivial BC breaks"? That strikes me as an oxymoron. (Yes, I have read #2135189: Proposal to manage the Drupal core release cycle).
You could have trivial and significant changes in a new minor version, and you can have backward-incompatible changes in a new minor version. However wouldn't any backwards-incompatible change be non-trivial by virtue of the fact that it breaks BC.
Comment #42
catch@JamesOakley. Yes it's possible when a new minor version comes out that a contrib module might not be compatible with it, however that doesn't mean that it'll need two branches, one compatible with 8.0.x and one with 8.1.x - just that it'll need updating to 8.1.x and previous versions of the module won't work any more.
Comment #43
JamesOakley@catch: In which case, you're proposing that the info (.yaml) files for modules and themes should just say "core=8.x" as at present, and (supposing a contrib module is on its 5th major release) the branch would be named 8.x-5.x, and releases of that module would be 8.x-5.1 and 8.x-5.x-dev.
In other words, what we have for Drupal 7 should just continue to work. I think I agree. If a BC change in core was particularly critical for a given contrib module, they could always fork to 8.x-6.x if the maintainer felt the need to - although, as you say, this would usually not be advised.
Comment #44
catch@JamesOakley I'm not 100% sure but if we're strict about only supporting one minor version at a time, then that seems enough to me.
Comment #45
tstoecklerI think even if discourage dependencies on patch versions a) we don't yet know how everything will pan out so not even allowing for it seems optimistic at best and short-sighted at worst and b) I still think it is confusing to specify "8.2" instead of "8.2.x" when the actual versions will be 8.2.3, 8.2.4, etc. (Note that unlike e.g. the kernel "8.2" will never actually be a release)
Comment #46
catchYes I'm inclined to agree with (a) in general (hence leaving this at RTBC), it's more about how much we encourage it or not.
Comment #47
alexpottWe don't seem to have complete test coverage here.
We don't have complete test coverage of these conditions.
Also there are lots of places where we are using
\Drupal::CORE_COMPATIBILITY
and we havedrupal_check_incompatibility()
- should these all be changing too?Comment #48
YesCT CreditAttribution: YesCT commentedupdating issue summary to make clear what the remaining tasks are. the dreditor insert tasks button is useful to making it easy to also include instructions for how to do the things. This is important to help people get involved, especially if this issue will be use for the Friday Critical office hours.
Comment #49
larowlanComment #50
Gábor Hojtsy@alexpott: I am looking into how to solve your feedback in #47. About drupal_check_incompatibility() that already uses version_compare() and not just comparing to specific compatibility values as-is. Not sure what to change there? There are two uses:
Then the core compatibility check, so for that we would need to simulate different compatibility versions of core. That means we would either need to be able to inject the core version to ModuleHandler::isCoreCompatible() or test an extended version of it with our own code. The later does not sound too sane, since then we would just test our own code. For the first variant, we can maybe introduce a class variable with a setter that would then be used to test alternate core compatibility. That is the change you had in mind?
Comment #51
xjmSo I marked this as a critical back in #5, but then suggested a workaround in the next comment. Silly xjm. Discussed with @alexpott, @effulgentsia, @catch, @Dries, and @webchick -- this is probably more properly a major, unless the proposed workaround doesn't work. (Still obviously a bug worth fixing though.)
Comment #54
pwolanin CreditAttribution: pwolanin commentedre-roll for fuzz
Comment #56
pwolanin CreditAttribution: pwolanin commentedoops, missed new info.yml files
Comment #59
MixologicI've been doing a bunch of work on processing the update stats and have noticed some things Im curious about:
If I have an 8.0.x drupal site, and have a module installed, say thingy 1.0 (1.0.0?). Thingy is going to ask for
http://updates.drupal.org/release-history/thingy/8.x?site_key=long_site_key&version=8.x-1.0
Now, if 8.1.0 comes out and thingy implements new features that take advantage of api's introduced in 8.1, and thingy releases 1.1 (1.1.0?) how should updates.drupal.org respond to that update request?
It seems like the updates request is not sending enough information -- thingy 1.1 would be incompatible with their 8.0.x drupal site, but updates are not specifying enough information to allow for a correct response.
The 8.x in the updates url comes from the CORE_COMPATIBILITY constant. Should that be updated to reflect 8.0.x, and all uses of CORE_COMPATIBILITY updated to expect the longer number?
Comment #60
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented@Mixologic, since 8.0.x is no longer supported after 8.1.0 is released, the Update Manager will probably be telling you to update core at the same time as it's telling you to update the module. If you have it configured to only warn about security updates, then maybe not (at least for a short while, until the next core security release)... but the bottom line is that your days on 8.0.x are numbered anyway. This really isn't any different from Drupal 7.
Taking a quick look at the patch, I am not sure overloading the "core" key is the way to go as it makes it inconsistent with other dependencies (and is also kind of an API change at this point)? If I'm reading the patch right, it wants to let you do this:
Where the first specifies a >= dependency for Drupal core and the second for a particular module. Why is the syntax different? I actually think the current method is easier to understand:
Also, the current method even allows things like this (at least according to https://www.drupal.org/node/542202#dependencies it's supposed to - can't say I've ever tried it):
In which case this issue could maybe just be closed?
Comment #61
Mile23You want to be able to be as specific as you want to about core versions. It might be that you discover your contrib module breaks in 8.1.2, so you specify 8.1.1 and begin work on a new release.
I disagree with the proposed resolution in the issue summary:
Specifying 8.1.5 should tie you to that version exclusively. Specifying 8.1.* should work like all the other semver systems that aren't Drupal. :-)
Patch in #56 needed a reroll, so here's a reroll of dubious quality. Setting to needs review only to trigger the testbot... Clearly this issue will need more work.
Also setting parent to #1612910: [policy, no patch] Switch to Semantic Versioning for Drupal contrib extensions (modules, themes, etc)
Comment #63
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedBut again, that should already be possible, via:
So I still don't see why we want to add a new parallel method for specifying core dependencies, and especially why it should use a different syntax than the one which would still be used for specifying contrib dependencies.
Comment #64
mondrake#63 yes it should, but see #2641658: Module version dependency in .info.yml is ineffective for patch releases
Comment #65
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThanks. Sigh :) It seems to me like fixing that is a better way to go than this issue.
Comment #67
Wim LeersThis issue and #2641658 are blocking contrib modules. See #2641658-22: Module version dependency in .info.yml is ineffective for patch releases:
Comment #68
dawehnerIMHO we should really try to put another method onto the module but rather expand the VersionChecker, which is introduced as part of #2677532: Move drupal_check_incompatibility() functionality to a new Dependency class and Version component
Comment #74
MixologicMoving this to its new Meta parent.
Comment #75
webchickThis issue is blocking (at least) the Automatic Updates, Composer in Core, and Drupal 9 initiatives. After talking to @xjm, escalating to critical.
Comment #76
dwwNot sure how this overlaps with #2807145: [policy, no patch] Allow contrib projects to specify multiple major core branches, but over there, I propose completely removing the
core:
keyword and requiring module dependencies. If we did that, we could won't fix this one and concentrate on #2641658: Module version dependency in .info.yml is ineffective for patch releases. There's lots of overlap with all 3 of these issues, not sure the best way to untangle and move forward.Comment #77
catchThe fallback here with Drupal 9 is to require all contrib modules to have a Drupal 9 branch from 9.0.0 onwards, which would then update core compatibility if nothing else. If it's a choice between missing Symfony-related support deadlines and the 9.x contrib branches, security support will win at this point, without the Symfony deadline it should otherwise probably be blocking though. However we'll have exactly the same issue again for Drupal 10 so it makes sense for the issue to be critical regardless.
Not sure about this from the other issue:
We have a multi-year effort (possibly entering its second decade at this point) to make system module optional. This is nowhere near fruition but could eventually be the case. So while contrib modules might rely on specific core modules, just a core dependency should still be an option I think.
Comment #78
tedbowHere is reroll for now with a couple of changes
I removed this test module in favor of using
system_test_system_info_alter()
to alter the core requirement ofcommon_test
. This way we don't have to update the test for every minor version of Drupal 8.This alter hook is already used in this way in
\Drupal\Tests\system\Functional\Module\VersionTest::testModuleVersions()
.I didn't change this but should we change this to by 12.x or something to buy us some time before we have to update the test.
The other option would be use the
system_test_system_info_alter()
to also use common_test to test this and not need the module\Drupal\Component\Version\Constraint
I don't see aVersionChecker
class.If so, I agree that we should use this class. It might require changes because it doesn't work with semantic versioning. See #2641658: Module version dependency in .info.yml is ineffective for patch releases which fixes this adds test coverage. I didn't make this change in this patch though.
but also the test coverage for that class doesn't cover much of dependency functionality so I created #3066448: Harden test coverage for dependency Constraint class
Comment #80
tedbowWe no longer need
$theme->info['regions']['content']
here.Since @alexpott made the suggestion to add this in #33 #2489830: Improve theme compatibility error message was committed which added this below
$theme->incompatible_region = !isset($theme->info['regions']['content']);
Comment #81
tedbowOk this patch
ModuleHandlerInterface
\Drupal\Component\Version\Constraint
instead check core compatibilityConstraint
doesn't handle semantic versioning correctly.Constraint
Comment #82
tedbowCopy and paste error should have been
$modules[$dependency]->info['core']
This would have cause a test failure but was applying to 8.6.x.
Fixed
Comment #83
tedbowOk I was looking at the proposed solution to this problem and I am not sure I agree with it.
core:
key is "8.x". There is the idea that we would also support 9.x because it would be equal to\Drupal::CORE_COMPATIBILITY
but as we haven't released 9.0.0 yet I think we are free to change that.core:
key act like thedependencies:
key using our version string constraint systembut of course if we fixed this current issue you won't need to do this because you could just use the core key.
All this makes wonder if we really want connect our
core:
key to our own version constraint system when right now the only thing we have to support for BC purposes in8.x
. Right now\Drupal\Component\Version\Constraint
is very complicated and #3004459: [PP-1] Fold in dependency parsing wisdom from project_composer only makes it more complicated because it has to include a conversion from our current version constraint strings to semantic versioning as supported by\Composer\Semver\Semver::satisfies
.What if we supported
8.x
for BC purposes and then supported semantic versioning as supported by\Composer\Semver\Semver::satisfies
directly.If we did this:
core: 8 - 9
which support all versions of Drupal 8 and 9. Or you could specify a specific versioncore: 8.6.5 - 9
Comment #84
xjm@tedbow explained his proposal for semver support in #83 to me; I think it's a great solution to use Composer's existing API to validate this so that we can support this properly in Drupal 8 while adding a smooth upgrade path to
composer.json
if we go that direction, and also giving us multiple core branch compatibility kind of "for free" if we don't end up deprecating.info.yml
.@tedbow will post a prototype for that here and then if others think it is a good idea we can use the same approach in #2641658: Module version dependency in .info.yml is ineffective for patch releases.
Comment #85
tedbowok this patch does #83
\Drupal\Component\Version\CoreSemver
with a single public static function:satisfies()
\Drupal\Component\Version\CoreSemver::satisfies()
checks for '8.x' and convert it to ^8 which covers any Drupal 8 usage and then passes it to\Composer\Semver\Semver::satisfies()
.\Drupal\Component\Version\CoreSemver::satisfies()
to prove it covers all the cases we need\Composer\Semver\Semver::satisfies()
is concerned8.x
actually equals^8
. Both of these constraints are actually parsed as>= 8.0.0.0-dev < 9.0.0.0-dev
I didn't know this and I am not 100% certain but I added test coverage to prove it>= 8.0.0.0-dev < 9.0.0.0-dev
is actually what we mean by the currentcore: 8.x
in info.yml files.\Composer\Semver\Semver::satisfies()
directlyComment #86
tedbowre #85
Using both calls where to prove that for our test cases they are exactly the same.
\Composer\Semver\VersionParser
is what\Composer\Semver\Semver::satisfies()
actually uses to parse the dependency string.We can see here that '8.x' is the same as ^8 as far as composer version parsing is concerned
\Drupal\Component\Version\CoreSemver
to do conversion for 8.x.\Drupal\Component\Version\CoreSemver
for is to wrap\Composer\Semver\Semver::satisfies()
in a try/catch. Right now if you havecore: 8.nonsense
you can still visit /admin/modules but you wouldn't be able to enable that module.With this patch you would get the fatal error
Comment #87
catchIf we're using Semver::satisfies() does that mean this issue indirectly fixes #2807145: [policy, no patch] Allow contrib projects to specify multiple major core branches too? If so there's only tests to add (which could happen in that issue possibly).
Comment #88
tedbow@catch
Yes it should allow you to specify any range for Drupal that you wanted.
This test case from #85 for
\Drupal\Component\Version\CoreSemver::satisfies()
would prove that but of course since in #86 removes the need for\Drupal\Component\Version\CoreSemver
at all this test was remove. We could have a functional test for this with a module used these ranges to at last prove they work for Drupal 8.Not sure how we fake
\Drupal::VERSION
to be 9 in a functional test.Comment #89
MixologicSo the current proposal is "The value for the core key in info.yml files will add the ability to express the value as a semver compliant constraint, and it just so happens that the current value, 8.x, also works"
Since we're not forced to support an strange BC things like the version parsing in .info.yml files for module projects, it seems like relying on bare semver is a great way forward here, vs postponing on #3004459: [PP-1] Fold in dependency parsing wisdom from project_composer and trying to leverage that.
I'd lean on putting this into
\Drupal\Component\Version\CoreSemver
because one of the reasons we want to expose our version number logic as a separate package/library is that drupal.org can then rely on that component to reuse the same logic, even while we're still on d7.There is one thing that we'll need to fix first in core if we change the allowable values in the info.yml core key: The locale module uses that key to build it's URL https://git.drupalcode.org/project/drupal/blob/8.8.x/core/modules/locale... and also https://git.drupalcode.org/project/drupal/blob/8.8.x/core/modules/locale...
We have already put some work in on the drupal.org side to make it so that the %core pattern can both be 8.x as well as '
all
', so I think thats more a matter of ignoring the core key and using 'all' to get to the right translation subdirectory. But I think that ought to happen first.Comment #90
MixologicTheres one more place where the core key gets used: in update.inc: https://git.drupalcode.org/project/drupal/blob/8.8.x/core/includes/updat... where it checks if a DB schema update hook is compatible with the current version of core.
Comment #91
Wim Leers#77 I love how delightfully British the parenthetical remark is 😋
#78.3: Wait … #3066448: Harden test coverage for dependency Constraint class was created not only for #3004459: [PP-1] Fold in dependency parsing wisdom from project_composer and #2641658: Module version dependency in .info.yml is ineffective for patch releases, but also for this issue? 😮
#83:
composer
-like and -compliant without breaking BC. Your concrete proposal here seems to strike perfect balance 👏👏 Glad to see @xjm's tentative support in #84 and @Mixologic's vote of confidence in #89.#89 + #90: Woah! Those
locale
andupdate.inc
pointers are priceless!Nit: unnecessary whitespace change.
This version is distracting from what matters here, let's use
1.0.0
?This will soon stop working. Should we make this something absurd like 99? Assuming a new major version every 5 years, this would last us nearly half a millennium. 😆💧🏰
This does not exist.
Comment #92
tedbow@Mixologic thanks for pointing out these cases where
core:
is used.\Drupal\Tests\system\Functional\Module\DependencyTest::testCoreVersionDependency()
to use the~8.6
format~8
and 1 with just thesystem.info.yml
to be easier to reviewProbably both will break a bunch of tests
Comment #93
MixologicSo there's another approach to this that I think we might consider.
We should drop the core_compatibility checks entirely, or we should only check for 6.x/7.x and respond accordingly and *accept everything else*.
If our goal is to be able to use existing d8 modules when d9 is released, that will not actually be possible *unless* the module maintainers have updated their core: key to expressly indicate that their module will work with d9.
As of the last time I ran the phpstan deprecation testing, there were about 7700 d8 supported modules. Of those, about 3000 of them have no deprecated code usages. That's 3000 modules that would work with d9 if it were released tomorrow, with nothing required of the maintainers, and nothing blocking sites that rely on those modules from upgrading. If we impose a requirement on maintainers that they must take an action before d9 comes out, we stand a good chance of that not actually happening, and preventing any sites that rely on those modules from moving to d9.
Although... when 9.1 comes out and adds a new api that a contrib module takes advantage of, we'll need some way for that module to express that it needs at least 9.1. So... perhaps "8.x" becomes the wildcard that we always allow?
Comment #95
Gábor Hojtsy@Mixologic: well, your data shows that it is MORE likely that an 8.x module running on 9.x will break than not. So assuming it will not break is not a good idea IMHO. I don't think asking people to do some manual action is bad. Also depending on how semantic versioning of contribs will be implemented, contrib maintainers may need to do some action there as well?
Commit an info.yml change and tag a new release is still easier than "now start a new branch of your module and maintain two branches from now on". I am concerned if we assume 8.x modules should just work with 9.x that will lead to lots of easily hosed sites. (That said, I think this would ultimately be a release manager decision).
Comment #96
tedbowInteresting that both patches in #92 had the same number of fails. Many if not all are in UpdateTests and they fail with
Since this service is from the system module I think somehow
\Drupal\FunctionalTests\Update\UpdatePathTestBase
is not installing the system module. Interestingly when I try to install drupal with the patch withdrush si
it also does not enable the system module but when I install with the patch throw the browser it does enable the system module. So my guess it is something with\Drupal\FunctionalTests\Update\UpdatePathTestBase::installDrupal()
. I will investigate but let me know if you have any ideas.Comment #97
MixologicHad a good chat with Gabor and #93 is really not plausible. I was hoping for some way that required no input from maintainers if possible, but we really don't have any way of knowing whether any given contrib module is truly d9 compatible without maintainers confirming that it is, in fact, compatible. As such, I fully agree that #95 is the way forward and that we'll need maintainers to express that their module is compatible, and hopefully with something as minor as a core key and not something as drastic as a 9.x branch.
Comment #98
catchfwiw this is my preference - contrib and custom modules have to make a .info.yml change and a new release, but we don't require a separate branch. It doesn't rule out a module having issues with 9.x but the opt-in means at least someone thought it was a good idea.
This is obviously a very good reason to get this change into 8.8.x asap though, so that there's plenty of time to actually do that before 9.0.x lands.
Comment #99
tedbowShould have a
!
here 😱Comment #100
tedbowComment #101
tedbowBelatedly replying to @Wim Leers comment #91. Thanks for the review
\Drupal\Component\Version\Constraint
then it would have been needed I think because there would have been changes to this class. But in #85 onward we are usingwithout this class so this issue doesn't need #3066448
Comment #102
tedbowThis patch adds
core:
key is not a valid constraint.Right now we just do
$modules[$dependency]->info['core'] != \Drupal::CORE_COMPATIBILITY
So if you have anything else we don't hard fail in the modules page or
\Drupal\system\Controller\SystemController::themesPage()
#99 would hard fail.\Drupal\Tests\system\Functional\Module\DependencyTest::testCoreVersionDependency()
specifying to major core version with a||
operator and some other cases.core: 8.x
will also be supportedComment #104
MixologicLooks like you might have accidentally renamed system_incompatible_module_version_test instead of adding a new one for the system_incompatible_core_version_test_9x
(which, since it was added to the test in 2313917-102-no-info-yml-changes.patch, but didnt add the new testing module, explains the failure)
These style of range operators are very uncommon (I had to look up why the - 9 worked for 9.1.1: https://getcomposer.org/doc/articles/versions.md#hyphenated-version-range- for the curious).
We should add the more standard pattern of using '
^8 || ^9'
(also^8.7.6 || ^9
) and reflect that in our docs. (8 - 9
is appealing due to its simplicity, but most of the existing composer/semver documentation and examples out there likely wont suggest that pattern)Comment #105
tedbowre #104. @Mixologic thanks for the review
description
which should have been done anyways and now git picks it up as new fileThe problem in 2313917-102-no-info-yml-changes.patch is I did
git diff 8.8.x ':(exclude)*.info.yml' > ../2313917-102-no-info-yml-changes.patch
which of course did not pick up the new system_incompatible_core_version_test_9x.info.yml file 😿
Then should also use DrupalSemver
Change these to use
^
to matchDrupalSemverTest
Removed this test because it seems a made up situation.
These test cases are the only ones where
DrupalSemver
acts differently then\Composer\Semver\Semver::satisfies()
. Do we need the other test cases or are we just testing\Composer\Semver\Semver::satisfies()
?Comment #106
tedbowThe fact #105 passed without any changes to the
locale
module makes me think there is actually a test for creating the url for the translation files directly from a .info.yml file since all of those files where changes to usecore: ~8
and no test failed.I don't know much about locale and the how this intergrates with d.o but looking at
\Drupal\Tests\locale\Functional\LocaleUpdateBase
$this->makePoFile('remote/8.x/contrib_module_one', 'contrib_module_one-8.x-1.1.de._po', $this->timestampNew, $translations_one);
This makes me think we still need to be sending
*/8.x/*
unless we want to wait for drupal.org infrastructure changes.Looking at
locale_translation_build_projects()
It seems like you could get a translation for a different version of then the current. But then how would have enabled the module?
Could we just also use
\Drupal::CORE_COMPATIBILITY
regardless of the core key which will be changed in this issue?Comment #107
Gábor HojtsyLocalize.drupal.org supports
/all/
as well in place of/8.x/
in the path in the directory component. I cannot find the issue and it may not be 100% permanently solved but @drumm/@mixologic and I did some experiments back in the day :DComment #108
tedbowtldr;
I am including my original comment below in which I think I figured this out. but as far as I can tell
\Drupal\Core\Utility\ProjectInfo::filterProjectInfo()
filters thecore
key out when setting the project data.locale_translation_build_projects()
the only way$data['info']['core']
can be set is it was added inhook_locale_translation_projects_alter()
.which runs after
\Drupal\Core\Utility\ProjectInfo::filterProjectInfo()
which is triggered byIn
locale_translation_project_list()
locale_translation_build_projects()
'core' => isset($data['info']['core']) ? $data['info']['core'] : \Drupal::CORE_COMPATIBILITY,
core:
key of the .info.yml files.(@see 1)/all/
instead of the value set inlocale_translation_build_projects()
because custom localization servers might not break.We could announce though that 9.x will not be support and then switch to
/all/
Original comment/debugging
@Gábor Hojtsy thanks for the info.
I chatted with @Mixologic about it and he said as far as Localize.drupal.org is considered we could switch to
/all/
instead of the current/8.x/
But the locale module also allows you to use a different localization server. I am not sure anybody does this. @Gábor Hojtsy maybe you would have a better idea? Looking at
locale_translation_default_translation_server()(
this could easily be done. So I assume we have to assume there are some out there.Right now the core value that is used in the server request to replace
%core
is set here'core' => isset($data['info']['core']) ? $data['info']['core'] : \Drupal::CORE_COMPATIBILITY,
in
locale_translation_build_projects()
This will not logger work because
$data['info']['core']
may now be^8 || ^9
.(Editorial note: This is a wrong assumption as if you read on)This a little confusing because currently you can't enable a module or theme unless the core key is equal
\Drupal::CORE_COMPATIBILITY
.So how module like this ever get installed if
$data['info']['core'] !== \Drupal::CORE_COMPATIBILITY
? So why was that check ever put in the first place?But then I looked and
$data['info']['core']
is not coming from directly from the module's info file. In\locale_translation_project_list()
we actually firehook_locale_translation_projects_alter
So it would be possible to change the 'core' key value in the hook I think? But looking at the docs in local.api.php that doesn't seem to be the intention of this alter hook. Therefore I don't think we have to worry about a BC break there
The only other case I could think of where the 'core' key would not be set is for
'project_type' == 'core'
I debuggedlocale_translation_project_list()
and do see this.Oh damm. This more and more complicated. Just found
\Drupal\Core\Utility\ProjectInfo::filterProjectInfo()
which actually filters out all but a whitelisted set of keys in the project info. For our purposes this will not have thecore
key set in the whitelist so I am not I actually don't don't know how you would havecore
set unless you added it inhook_locale_translation_projects_alter
. Very confusingSo if that is the case it seems like we should be safe to just change the code mentioned above in
locale_translation_build_projects()
from'core' => isset($data['info']['core']) ? $data['info']['core'] : \Drupal::CORE_COMPATIBILITY,
to just
'core' => \Drupal::CORE_COMPATIBILITY,
The other option if we want to be super paranoid about breaking any strange usage of
hook_locale_translation_projects_alter
would be to just leave it the way it is. From my reading of the code the only way$data['info']['core']
would be set is if it was set in this hook(because of\Drupal\Core\Utility\ProjectInfo::filterProjectInfo()
)Because of
\Drupal\Core\Utility\ProjectInfo::filterProjectInfo()
we should not have to worry aboutcore: ^8 || ^9
or any other core value directly from a .info.yml file being used in\locale_translation_build_server_pattern()
to replace%core
.Comment #109
MixologicReally good find on that weird info['core'] behavior. Definitely doesnt seem entirely intended to work that way, but thats how it is.
So, for the purposes of this patch, we don't need to concern ourselves with the locale urls. Those will be set to \Drupal::CORE_COMPATIBILTY if i'ts reaching out to ftp.drupal.org, regardles of the core version key. ( We can still eliminate that for core without any bc breaks by replacing the %core key in the default_server_pattern with all)
Regarding the contents of #105
I only found a couple more things that need adjustments:
I think we'll need the same handling here in the update module:
https://git.drupalcode.org/project/drupal/blob/8.8.x/core/modules/update... and change the language in https://git.drupalcode.org/project/drupal/blob/8.8.x/core/modules/update... to use \Drupal::VERSION
And I think we might want to reword this message to use \Drupal::VERSION and be less about the core key being wrong and more about it being incompatible:
https://git.drupalcode.org/project/drupal/blob/8.8.x/core/modules/system...
Otherwise I think this patch is the right thing to do, and we should do it soon.
Aside: I cant seem to find anywhere that the
core
key is being checked for installation profiles. ¯\_(ツ)_/¯. Another, side issue seems to be that there's a core key for views compatibility in config files as well that appears to be not used/checked anywhere.Comment #110
tedbow@Mixologic thanks for the additional information
Yep I think we should file a follow up to do that though since it is not needed in this patch.
core
key from the info.yml file is not the same core translation projects/urls is so hard to figure out, maybe unintentional, and probably untested.I made #3070354: Add a test for locale_translation_build_projects() to unensure that 'core' key does not affect translations URLs to make a test that clearly test this and it has patch that proves we could change
locale_translation_project_list()
to actually whitelistcore
so it does come from the info.yml files and no tests would fail.It would be great to get that in.
update_verify_update_archive()
check that module is compatible.I think we could add this to
\Drupal\Tests\update\Functional\UpdateUploadTest::testUploadModule()
because this would checkcore
viaupdate_verify_update_archive()
. I will make an issue.So I made this #3070401: Install profiles do not support multiple core branch compatibility is. We will probably need to fix that and then decide if we also want profiles to be able to be compatible with multiple versions of core. I think now they would be because of this bug.
Good catch!
$theme->incompatible_core
can be TRUE if the core key is missing or incompatible so I provided 2 different messages.Comment #111
tedbowTagging with
Drupal 9
since this allows #2807145: [policy, no patch] Allow contrib projects to specify multiple major core branchesComment #113
tedbowI realize now that 'core' is require key for the yml file the admin page will actually have a fatal error if 'core' is not provide so we don't need this inner if/else.
It is true that you could remove the 'core' with an alter but that is existing problem with the message and an edge case that I don't think we need to solve here.
Remove the if/else and simplifying the message to make it more similar to the current one.
Also updating related test which caused the fail and fixing the codesniffer nit in #112
Comment #114
MixologicThis is rock solid IMO, and enables multiple valuable things.
Contrib projects that must have a particular version of core will be able express that in the core key, and Contrib projects that wish to work with both d8 and d9 can explicitly state the breadth of their compatibility..
Comment #115
jibranLet's add a change record as well.
Comment #116
tedbowHere is change notice: https://www.drupal.org/node/3070687
It is tricky than I thought because
Basically most modules can't actually use this now unless they don't want to be compatible with 8.7 or before
so could use some second opinions on the wording the recommendations about when to recommend developers start to use the new format.
Also I think this issue is also important #3070354: Add a test for locale_translation_build_projects() to unensure that 'core' key does not affect translations URLs to get committed for reasons stated in #110
Comment #117
jibranThanks, for creating the change record.
Let's add some more test cases here e.g.
^8@alpha
,^8@beta
,8.x-dev
,dev-8.8.x
, and8.x-dev#ceebf041
Comment #118
jibranComposer also supports
self.version
which I assume in this case can be\Drupal::VERSION
. Do we want to suppport that as well?Comment #119
MixologicRe #117 Composer/Semver is pretty well tested. Do we really need to prove that the underlying implementation works? (https://github.com/composer/semver/tree/master/tests). The primary thing we should be proving is that '8.x' still works as intended, and will work with 8.8.0. If we introduce something radical into DrupalSemVer beyond just catching an exception, then yeah, we'd want to verify all those other permutations/combinations, but as it stands I don't think it makes sense to test dependencies.
Re #118This is the core: key, so it wouldnt ever make sense to use self.version as the module versions do not have any relationship to the version of core, so no, I do not believe we should support that.
Comment #120
Mixologic8.9 comes out at the same time as 9.0. That means that this doesn't actually allow module developers to use this prior to 9.0 unless they drop 8.7 support. Does that achieve our goals here?
I guess the same sort of issue stands with any deprecations introduced in 8.8.0. A module cannot simultaneously remove those deprecations and be d9 compatible as well as maintain compatibility with 8.7.
Comment #121
larowlanShould we keep behind one test .info file with 8.x to validate that it still works?
Comment #122
BerdirDidn't really follow the whole discussion, but to address the when-can-I-use-this, we could maybe backport a small change to 8.7 that converts anything containing "~8" in the core string to "8.x" so it would essentially be backwards compatible?
You'd still need to wait if you are using anything that was deprecated only in 8.8, but it might still be enough to get started.
And even without that, contrib modules can already have a patch that adds 9.x compatibility as soon as the branch really exists to start running tests against that, and commit/release that on the day that 8.9/9.0 is released, if they wish to do so.
Comment #123
xjm@Berdir Yep good call, we (me, @catch, @larowlan, @Mixologic, @tedbow) discussed today that we need to backport a lightweight version of this to 8.7 and probably 8.6 (without all the info file conversions and etc.). Ted's going to work on that minimal version of the patch and we'll commit it first.
We also want a test module that has the old format just so we can verify it keeps working.
Comment #124
xjmComment #125
xjmOh and we also discussed that for due diligence we should test scenarios where a module is updated but not core, and vice versa. We should have manual testing to document what happens for all the various relevant permuations of:
Comment #126
tedbowok here is the minimal version with info.yml changes
There were some things in the previous patch that there was not test coverage added for but since no modules are going to be using
core: ^8
I added some test forthe interdiff with 113 does not include the info.yml changes to keep it small so it is just the test changes.
Comment #128
tedbowfixing test failure and documenting test helper assert method
Comment #129
tedbowComment #130
tedbowre #125 @xjm thanks for pointing out these test areas
I tested:
It didn't respect the core key because we hadn't actually update
\Drupal\Core\Extension\ModuleInstaller::install()
. I added a check to this for the module being installed and the core key for the dependencies if they are being abled. Also add test coverage.Now drush will not install if the core key is not compatible. So now drush works with 8.x or anything else supported in semantic version constraints.
Before I made the changes to
\Drupal\Core\Extension\ModuleInstaller::install()
the install would not check for the core key. So even if a module in the profile had a incompatible core version it would still install.If a module doesn't have the core key at all fail because it is require key the info.yml files. This is true with or without the patch.
After I made the change to
\Drupal\Core\Extension\ModuleInstaller::install()
if you have a module with an incompatible core key that is required by your profile the install will fail. This is the same as if you have a module required by your profile that is that has missing dependencies.The "old format" is 8.x is covered by our new format and means the same thing to
\Composer\Semver\Semver::satisfies
that we mean so it is find. I have manually test this but also since in #128 we don't change any info.yml files except for a couple tests all our existing test cover this.If they use anything but "8.x" you won't be able to install the module on the modules page. But it will be able to be installed by drush or the profile installer because both don't check the value of the core key.
If the module is already installed it will remain installed but it will not run updates for that module.
works fine with the patch
core installs fine
update.php works fine for module with 8.x or ^8 in core.
\Drupal\FunctionalTests\Update\UpdatePathTestBaseTest::testUpdateHookN()
also covers this now.I have tested this but also covered now
\Drupal\Tests\system\Functional\Module\DependencyTest
Sorry just providing the 8.8.x patch for now. if this passes will fix a 8.7.x patch
Comment #132
Mixologicew, gross. I guess the simpleTestTest was built around expectations of the extension system.
Comment #133
tedbowSo the error #131 was because SimpleTestTest was checking installing
non_existent_module
. But the new core was checking for 'core' before checking if it existed. So only checking if it actually exists.But because of existing
We don't actually check if modules are missing unless you
$enable_dependencies == TRUE
. Not sure if that is intentional but it is existing issue. If we want to change that it should be another issue.Comment #134
tedbowand here is the 8.7 version
Comment #135
tedbowWe have 7 instance of
!DrupalSemver::satisfies(\Drupal::VERSION
should we replace this with something like\Drupal::isCoreCompatible($version)
If you look at the quote in #116
this can now be changed to
This means module developers don't have to wait till Drupal 9 is actually released.
Comment #136
Wim LeersI read all comments after #99.
was a major point of confusion (see #106, #108). It was added in #1627006: Collect project information for translation update. Specifically, in the first commit of that issue (there's two — the one you're looking for happened in #56, that committed the #53 patch). No explicit discussion about "core" or "version". I think this was just an accidental oversight that happened somewhere along the refactoring. +1 for the solution in #108 that is simultaneously a simplification. 🙇♂️👏 for creating #3070354: Add a test for locale_translation_build_projects() to unensure that 'core' key does not affect translations URLs to remove all ambiguity! I reviewed that patch too (and RTBC'd it).
core
key toModuleInstaller::install()
, which is what Drush uses. Great! 👍This can be seen at https://github.com/drush-ops/drush/blob/9.7.1/src/Drupal/Commands/pm/PmC....I was worried for a moment about https://github.com/drush-ops/drush/blob/9.7.1/src/Drupal/Commands/pm/PmC..., which says it duplicates portions of
ModuleInstaller::install()
(and would hence perhaps bypass thecore
key validation you added). But fortunately, this is used just to determine which additional modules would be installed, which Drush asks you to confirm, and then the full set of module is passed into core'sModuleInstaller::install()
. So we're good to go 👍foo
with a security release8.x-2.23
and in8.x-2.21
it updated itscore
key tocore: ^8 || ^9
. Now any site still on Drupal 8.6 won't be able to do this update. There isn't anything we can do to fix this.The only solution is to communicate that this is only supported as of Drupal 8.7.6, and that hence no contrib module should ever specify
^8 || ^9
, they should always be willing to specify the stricter^8.7.6
. I think that in a follow-up we should consider enforcing this: if acore
key is set to^8
, Drupal core could throw a fatal error, with an error message explaining that this risks breaking sites on Drupal 8.6 or 8.7 before 8.7.6. But even then, if a site on Drupal 8.6.x or even on 8.7.5 updates a contrib module to a version that hascore: ^8.7.6
, updates still will not be installed.*.info.yml
files in a follow-up, and that follow-up already exists: #3072702: Core extensions should not need to specify the new core_version_requirement in *.info.yml files so that 9.0.x can be installed.Patch review
👍 This looks good. The reasoning for adding this class is solid. Variable naming is identical to the decorated code.
🤷♂️ I find this string strange, but it is identical to the pre-existing exception messages in
\Drupal\Core\Extension\ModuleInstaller::install()
. So … ✅🤓 Nit: unnecessary blank line. Fixed. ✅
👎
@module
is not used. Removed. ✅👎
@core_constraint
is not used. Removed. ✅🤓 Nit: This is not mocking the
update_test_schema
module being installed, butupdate_test_semver_update_n()
. Fixed. ✅This could use a comment.
Missed opportunity for a Ted joke 😁
🤔 This puzzled me initially. So I dug deeper into this. I was able to confirm my hunch: this is effectively doing the opposite of
$assert_session_>fieldDisabled()
. But because there a::fieldEnabled()
assertion method does not exist, @tedbow must have opted to do this.IMHO this makes it much harder to understand the patch. So I'm adding
::fieldEnabled()
. Perhaps core committers disagree with this. But without that addition, this test is just so much more obscure.👍 This is not changing the test coverage, it's just updating the expected error message for the
test_invalid_core
theme, since we now fail in more explicit ways.🤓 Nit: comments are outdated. Fixed. ✅
Übernit: the blank line can be removed now. Fixed. ✅
👍 This assertion method was added to be able to assert the same error message for two different themes. This is necessary to reliably test the different possible causes for showing an error.
🤓 Nit: s/an invalid core/an invalid core version constraint/ Fixed. ✅
👍 IMHO this adequately tests the current and future scenarios we will encounter.
The only remaining risk that I see here is the scenario where a site that is still on Drupal 8.7.5 is notified of a contrib module security release that has
core: ^8.7.6
. If that security release contains anhook_update_N()
, then that won't be picked up. That is a manageable, isolated risk though. We should be able to prevent that from happening by having the security team requiring that no security release is allowed to contain anything else thancore: 8.x
until Drupal 8.8 is released (and Drupal 8.6 is deprecated).Comment #137
larowlannit: should we use single quotes here to aid readability
I queued a test run against 8.8.x as this doesn't apply to 8.7.x (but we want to backport it, so will need either a re-roll or two patches, one per version)
Comment #138
tedbow@Wim Leers thanks for the review and fixes.
I found another problem.
update_fix_compatibility()
is called from\Drupal\system\Controller\DbUpdateController::handle()
.update_check_incompatibility()
core: 8.x
will disable the module. The user will get no message that module has been disabled.I am not sure what to do except to ask developers to only use it for
~8.7.6
or the like. But figured we should discuss it at leastComment #139
Wim LeersReproduced #138. Well-spotted! The consequence is that this module gets automagically uninstalled. (While its configuration still is installed — this is literally just updating the
core.extension
config, nothing else happens.)I don't think there's anything we can do about this. The same is true for modules specifying
php: 7
in their*.info.yml
. We can't protect against that either.#233091: Move code from update.php into includes/update.inc added this a decade ago next week 😲. It has not changed since then. One key difference is that in Drupal 7, it used to just disable a module instead of uninstalling it. As of #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed (change record: https://www.drupal.org/node/2193013), that's no longer possible. Hence the result of
update_fix_compatibility()
used to be something more actionable ("fix the module, then just re-enable it"). Today, this results in a hard-to-escape place: you can't reinstall the module (because the config already exists, which Drupal complains about when you try to reinstall), reverting the module update won't help either (it's still uninstalled). The only solutions you have available are:core.extension
config manuallyhook_uninstall()
never ran, and hence ifhook_install()
is not careful it could up doing the same thing multiple times.I would propose we re-RTBC this and create a follow-up for improving
update_fix_compatibility()
.Comment #140
MixologicHmm. I hate to say it but I dont see how to get around this due to the number of people who could potentially update a module to one that an older version of drupal doesnt know how to handle.
How difficult would it be to allow for an *additional* key like 'corev2' or 'coresemver', and then when reading in the info.yml files, override what we consider to be the core key if the newer one is there?
Comment #141
MixologicSo yeah, looks like InfoParserDynamic is where we'd notice the additional key.
That way, contrib modules could future proof and work with 9.x, but could still keep the 'core: 8.x' key too so as to not break BC.
Comment #142
tedbowI think another option to would be enforce that you can't use any Semver that would match 8.7.5(assuming this would catch everybody doing ^8 or ~8.7.x)
that means to make it compatible with drupal 9 you would have to do `
^8.7.5 || ^9
` or^8.8 || ^9
.I feel like you would want to do this anyways because if your module is d9 compatible then you would have had to remove at least some deprectations.
This non developer would do
^8 || ^9
unless they simply add the git commit and didn't check anything at all either in tests or manually.Here is work in progress. I haven't tested it much just the start of the idea.
Comment #143
MixologicRight, but doesnt that still mean:
A site owner, on any version of Drupal less than 8.7.6, trying to upgrade to a version of a module that uses anything other than '8.x' as the core key is going to have their module uninstalled?
Comment #144
Wim Leers#140: IMHO that would be a huge WTF :|
#142: That's what I already said in #136 ( ), except that you're proposing to enforce this. I like it 👍
#143: Yes, it does. There's nothing we can change about how already installed Drupals handle this. The nice thing about #142 is that this massively reduces the probability that contrib module authors change it until it's absolutely intentional. There's no way to perfectly fix this; the only way to achieve that would be to time travel to before Drupal 8.0.0 shipped 🤓
Comment #145
MixologicHmm. IMO thats a hard BC break that puts site owners at risk of having a huge WTF, regardless of what value a contrib maintainer puts into their module.
There's no way to fix this using the existing
core:
key, but we *can* fix this by introducing an additional key. Older versions of core would ignore the key as superfluous, newer versions of core can detect that key and override the value.The only thing we would have to add is checks into InfoParserDynamic https://git.drupalcode.org/project/drupal/blob/8.8.x/core/lib/Drupal/Cor... and it would allow for a seamless transition for older sites.
The only challenge with this approach is documenting/explaining this for maintainers.
Comment #146
tedbowI didn't see #145 when I started this but this think the below should be considered:
I chatted with @Wim Leers some more about this.
Here is where I think we stand.(below considers this gets backported to 8.7.7 but could be a different 8.7 release)
\Drupal\Core\Extension\InfoParserDynamic::parse()
(or somewhere else) to make sure that no module usescore: ^8.7.6
or anything else that would suggest that this value would work for anything before this issue is committed. We could fail hard and have them fix it.core: ^8
orcore: ^8.7.6
.core: 8.x
on<=8.7.7
"core: 8.x
to put something in there release notes for site owners for with core
<=8.7.7
core:
key before they update a module so they probably won't. It would hard to get all contrib authors to include this in their release notes and many site owners won't read themAll this makes me think if we do the current method even with the approach in #142(fixed obviously) the problem in #138 is going to affect many, many sites and because of the nature of the problem it won't really be clear to the site owners how this happened. So agree with @Mixologic in #143 that this will be a pretty problem.
I agree with @Wim Leers point in #144 that this would be very confusing.
If we kept the
core:
and added a new one,core_requirement
for example. that would mean that any module that want to be compatible with 8 and 9 would needHaving 2 keys that are basically for the same purpose but 1 being ignored for any site above 8.7.7 seems strange. If I am a d9 site owner I would likely be confused as to whether I should use this module.
What if...
core:
key\Drupal\Core\Extension\InfoParserDynamic::parse()
to throw an exception ifcore:
andcore_requirement
were both present\Drupal\Core\Extension\InfoParserDynamic::parse()
we also check to make sure that we don't havecore_requirement: ^8 || ^9
because we know it won't work for earlier version because it will be missingcore:
core_requirement
will fail hard because their version of\Drupal\Core\Extension\InfoParserDynamic::parse()
will require thecore:
key. Because it would fail on the parser they will not get into the problem documented in #138 byupdate_fix_compatibility()
core_requirement
until you are ready to stop support for earlier version of Drupal core. but this needs to happen anyway at some point if you are using replacements for deprecations that weren't in earlier versionscore_requirement: ^8 || ^9
orcore_requirement: ^8.7.4 || ^9
this should obvious to developers that they are ending support for earlier versions.Basically for any developer using
core_requirement
we would force them to use a constraint that didn't include version of drupal before this was committedComment #147
MixologicOkay, so if
key doesnt exist at all, then the module they are attempting to upgrade to wont be possible, but they wont get stuck in that terribad limbo of "fake disabled".
I think that works, but I still think that having both keys has an advantage. Site owners, by and large, are not looking at that key. Its machine data. they are either grabbing a tarball from d.o. or using composer or using drush.
About half of the modules in the ecosystem work with *every version of d8 out there*. What this means is that if, for some strange reason, somebody on an old, 'working' version of 8.1.0 happens to see a grave RCE in one of their modules, and wants to upgrade, they cannot.
So, making the key either/or effectively puts a hard line that means "when a module decides to declare that it is compatible with d9, it must also declare that it is no longer compatible with anything less than < 8.7.6."
Allowing for supporting both allows a maintainer to *add* support for d9, without *removing* support for < 8.7.6 at the cost of 'two keys that mean the same thing, but with different allowable datatypes'
Comment #148
tedbowIs that true because
If it is 2) then I could see the case for the 2 keys. If it is 1) it seems will have to remove support for older versions of d8 anyways. @Mixologic maybe you have better insight into this?
Comment #149
MixologicIts 2. Theres a lot of modules that have no deprecations (at least as far as our phpstan is able to detect). Granted, we're testing against 8.8.x, which means that maybe they were deprecated in 8.6.0 and already fixed.)
But in any case, I feel like having two keys isn't really that complicated or difficult to explain, since the only people we'll be explaining it to for the most part are contrib maintainers, and that we should allow for all three scenarios: just core, just core_requirement, or both.
Comment #150
tedbowI am still on the fence
My vote would be that
core_requirements:
would take precedence. Maybe the developer meant to remove Drupal 8 support but forgot remove the core key.Of course if
core_requirements:
did take precedence this would still be compatible with any version of Drupal core before this issue was committed because it would ignorecore_requirements:
Could we somehow check to make sure if
core:
was used thatcore_requirements
constraint must pass for at least 1 Drupal 8 version? If not either throw an exception or at least put it on the status report.core:
key?It seems at least by 10.0.0 we should not support it.
It seems that by 8.9.0 we should recommend that
core_requirement
be the only key. At that point 8.7.x will be totally supportedComment #151
tedbowOk here is a try
\Drupal\Core\Extension\InfoParserDynamic::parse()
to make surecore
orcore_dependency
is providedcore_dependency
doesn't try to specify a constraint for version <=8.7.x(because it would be ignored by those versions). Not perfectcore:
only uses[MAJOR].X
. I limited to a single digit think we would deprecatecore:
before Drupal 10🙏Comment #152
tedbowFixed
\Drupal\Tests\Core\Extension\InfoParserUnitTest
and added test cases.There were a lot of fails in #151 which I can't reproduce locally. Maybe it is a Drupalci error?
I changed the issue to 8.8.x because if is an optional key we don't have to backport it. But we still could.
Comment #154
Wim LeersThe comment says 8.7.7, the condition says 8.7.6. Updated the condition.
Comment #155
Wim Leers#146++ Thanks again for that excellent idea!
@Mixologic keeps making good points, keeps seeing things others did not! 👏
#147: somebody on an old "working" version of Drupal 8.1.0 already has many security holes to worry about. We already don't support it. This would not actively break it. Plenty of Drupal modules are updated over time to rely on e.g. Drupal 8.4.3 or 8.5.2 and then they get a security release. Which means that security release will also work only on 8.4.3 or 8.5.2. Don't get me wrong, I get what you're saying. But somebody on such an ancient version of Drupal 8 we cannot really support anyway.
That being said, #149 is pretty convincing that we must allow both keys simultaneously to maximize the smooth update from Drupal 8 to 9. Consider me convinced 😊
"just core" or "just core_requirement" or "both"
(Note 8.7.6 was released the other day, so what's below assumes 8.7.7 is the patch release in which this will be backported.)
The end of #149 says there are three possible scenarios: "just core", "just core_requirement" or "both". Let's break down when you would use which scenario. Because #146 kept it simple: it's either "just core" or "just core_requirement". Now deciding what to do became much more complex, and almost needs a decision tree (EDIT: after writing the list below, I think it does need that):
Summarized, it's either
Correct? (This information is what we'd need to update the change record with.)
Deprecating "core"
Removing
core
in Drupal 9 would be impossible because that'd remove the ability to smoothly update from Drupal 8 to 9. Allowing both keys may be an acceptable and required cost (in the form of a mild DrupalWTF) in order to enable a smooth update path. So I think @tedbow is right in saying that we'd deprecatecore
in 9 and remove it in 10.On ensuring the ecosystem does the right thing
I'm kinda proud I thought of this one 🤓🤓😅 We could add a
git
pre-commit hook that checks if the top-level*.info.yml
file containscore: 8.x
before the commit but not after the commit; if the module has >0 sites using it according to our usage statistics, then the commit should be refused.This would ensure that not a single Drupal contrib module that is in use today would stop working on Drupal versions older than 8.7.7. It means that modules can only go from "just core" to "both", and not to "just core_requirement". 🥳
Thoughts?
Comment #156
Wim LeersThis is definitely the lowest-priority thing in this issue. Let's first ensure the technical solution is sound before we start bikeshedding the name of the new key :)
@tedbow proposed
core_requirement
. I proposedrupal
. An example:versus
Comment #157
MixologicOn ensuring the ecosystem does the right thing
Re #155 I've been thinking that maybe its not the responsibility of the runtime to ensure that the maintainer didn't make a mistake and have disagreements between the two keys. It seems more like the job of a static analysis tool, somewhat like pareview.sh or similar, or yes, something like a post commit hook could work as well. It'd basically result in a bug/issue being filed with the module maintainer.
This is already the case for every module out there that leverages an API feature that wasn't in a previous version, and as far as I know the issue queue/support forums don't have many instances of people trying to install 'new' modules on older versions of drupal and running into missing API's. (but then again, I dont really know if thats been a problem or not)
While it'd be great to have something bulletproof and impossible for people to install a module they can't use, Im not sure we need to put a lot of effort into protecting against this sort of 'maintainer mistake' edge case.
Deprecating "core"
The plan is still to replace these keys with the optional composer.json, and then yes, deprecate both core and core_requirement in 9 for removal in 10.
Im not sure if we would/should still allow
core: 9.x
or if we should deprecatecore:
in d8 for removal in d9 and only have thecore_requirement:
key in d9. There's probably good arguments for both options.Decision Tree
You've got a couple of scenarios incorrect.
1. you *must* use 'core' if your module should still work with versions of core below 8.7.7 (or, now 8.8.0)
2. you *must* use 'core_requirement' if you wish to add d9 compatibility
3. you *may* use 'core_requirement' to specify requirements on versions 8.7.7 (8.8.0) and above. If you do, you *must not* use the 'core:' key.
Scenario 1 is 'just core', and is the default, scenario 2 + 1 is "both" and scenario three is "just core_requirement".
Naming
What about
drupal/core: ^8.7.7 || ^9
As thats what they'll have to put into their composer.json eventually anyhow...
Comment #158
Wim LeersTrue. But the difference is that at this stage of Drupal 8, we want to ensure every Drupal 8 module that exists today if compatible with Drupal 8 and 9 if at all possible.
Hm. Interesting that you say that. Because this seems to be counter to what you wrote in #140, #145 etc.. You argued to add a second key instead of making the
core
key more capable precisely to avoid contrib modules from releasing updates that wouldn't work on older versions of Drupal.I am probably missing something. Why is it different for the
core
key*.info.yml
API than it is for other APIs?RE: decision tree: that is exactly what I wrote, I just phrased it differently — you phrased it more clearly :) (Although you omitted one scenario: the one in my 4th scenario bullet.)
EDIT: no, your second bullet is wrong. We can't require
core_requirement
to be added if we want Drupal 8 modules to work in Drupal 9 unchanged. At least, that's what #3069795: [meta] Improve dependency management for extensions's first use case says:Or perhaps I misinterpreted that too?
(In writing my previous comment, I specifically tried to verify that we were decided to require contrib module maintainers to do something to declare Drupal 9 compatibility. This was surprisingly hard to find, and the above quote is what led me to conclude that we chose to not require any action.)
Let's make this super clear once and for all in the issue summary!
RE:
drupal/core
: 😀 Works for me 👍Comment #159
markhalliwell"just core"
I really don't understand why a new "core_requirements" or "core_dependency" field is needed. This seems like this is overcomplicating matters; way more than it needs to.
Can't we just add a check for
8.x
here since it's a known and likely expected value? It should honestly just trigger a deprecation anyway I think.All this talk about supporting both 8 and 9.
If that's truly the case then a project can use the new semver constraint.
This means that it wouldn't be installable for versions older than 8.7.7, but I think that should be "works as designed".
Take Drupal Bootstrap's 8.x-3.x branch as an example. Due to the fact that themes (still) can't have dependencies, we never had the option to do the system dependency "hack".
Technically, the project has to support all 8.x core versions, which effectively makes it >= 8.0.0.0-dev < 9.0.0.0-dev (as the IS states). We can't even use newer APIs that replace procedural ones (i.e. drupal_set_message vs. messenger service) in fear of breaking existing installs.
This means that even if we wanted to support both 8.x and 9.x, we'd still have to create a new branch anyway just for the deprecated APIs that were removed in 9.x. At which point we could easily still support "8.x" but only the latest versions.
IMO, that's fine. I don't think core needs to worry so much about the duality of supporting multiple versions. All it should really care about is if the semver constraint passes the current version.
Just the fact that this finally gives projects the ability to specify which version of core is massive.
This means that the responsibility of what is or isn't "installable" falls back to the project itself... not core.
How that project manages its compatibility/dependencies should remain at a project level, whether they choose to use git hooks, sniffers, a Noonian Soong android... whatever lol
Comment #160
tedbowre #159
@markcarver sorry we haven't had to update the issue summary, explain this better.
We could fix it for any site that has this commit but for any site that is still running 8.7.5, which is the latest security release, we can't fix it.
The bigger problem is not whether it is installable. the problem is that Drupal 8.7.5 has
my_module 8.x-1.1
that usescore: 8.x
and then updates tomy_module 8.x-1.1
that usescore: ^8
(anything beside 8.x) will have this this module disabled, Drupal 7 style not uninstalled, when they visit update.php(they don't even have to run updates). They will not be notified of this in anyway. I detailed why this happens in #146. It is related to #2917600: update_fix_compatibility() puts sites into unrecoverable stateIf we use
core: ^8
we can't prevent this for any site that doesn't have the core update.re #155
Yeah but the problem existing with someone on 8.7.5 which is the last security release.
New patch
This patch adds more validation to
\Drupal\Core\Extension\InfoParserDynamic::parse()
It makes sure you can't do
'
We know that this module would be able to be installed on versions before 8.8.0 because we know 8.7.6 will not check
core_dependency
So we should not validate this unless the info.yml file remove
core:
Comment #161
MixologicYes, we determined that the only real way that we can determine whether or not a module is 'ready' for d9 is to require a module maintainer express that in some fashion. Updating their core compatibility key is the mechanism we're planning to do that. Therefore d9 wont enable a module that only has 'core: 8.x'
And yeah, Im not totally 100% either way on whether or not core should try and protect users from invalid modules. My main issue that I was talking about in 140/145 was in response to #138. Basically yes, we should definitely not *break* sites, and that situation seems like it would be really, really common. Maintainers accidentally having both keys not agree with each other seemed like it was considerably less likely to happen, and wouldnt be as grave of an impact.
But Im fine if we want to put in some checks into core for additional validation. (I just don't want things to get super complicated with lot of code paths to validate to prevent uncommon edge cases - but its probably not as big of a deal as Im making it)
Because if we dont, we will break, hard, existing sites if they upgrade a module and there is anything other than '8.x' as the value in that field. See #138
Comment #162
markhalliwellIMO, that kind of change in a project should be considered a BC break and they should have created a new branch (major version) that supports the new semver syntax.
Comment #163
MixologicYes, that would be a BC break, which is why adding a new key is a way to avoid having that break. We do not want to require every d8 module to release a new major version to be d9 compatible. See point #3 on here: https://www.drupal.org/project/drupal/issues/2608062
Comment #164
tedbowComment #165
markhalliwellExcept that any project that would want to support both 8.x and 9.x would mean that the only version of 8.x they could technically support is the very last one due to API deprecations being removed in 9.x... right?
So that means that if their project wants to support anything less than the latest version of 8.x it would still have to have a separate branch (major) that includes the deprecated APIs.
This decision (to support both 8.x and 9.x) is something that happens at a project level and explicitly requires it to remove BC support from previous versions, thus new branch (major).
No project will ever be able to support both with anything less than the latest 8.x release as well as 9.x using the same codebase.
This is why Drupal Bootstrap 8.x-3.x will never work for both, regardless if there's some new 9.x only
core_dependency
key. That's because it uses deprecated APIs that have been removed in 9.x (i.e. drupal_set_message()).Even if it kept
core: 8.x
and addedcore_dependency: ^8.8 || ^9
, it would only ever work on 8.x... never 9.x because of said nonexisting APIs.The project is then forced to create a new branch (major) that supports only the latest 8.x release (where it doesn't use any deprecated APIs) and 9.x. Thus, specifying
core: ^8.8 || ^9
would work because this issue would be committed to the latest 8.x code and of course 9.x.Using the core key is sufficient. I don't see how adding some special "9.x only" semver key actually solves anything in the long run with how current project versioning and workflows actually work.
Comment #166
Berdir> Except that any project that would want to support both 8.x and 9.x would
mean that the only version of 8.x they could technically support is the very
last one due to API deprecations being removed in 9.x... right?
That is exactly how it works, with all deprecations. Additionally, 8.9 will have no or only minimal new deprecations, so projects should be able to compatible with 8.8+ and 9.0. Or maybe even 8.7+ or 8.6+ and 9.0 if it happens to not use code that was deprecated in newer versions.
This is by design.
> The project is then forced to create a new branch (major) that supports only the latest 8.x release (where it doesn't use any deprecated APIs) and 9.x
No, that's not how semver works, as long as your module defines its dependency it's acceptable and normal for contrib modules to continuously or in a bigger jump require new 8.x minor versions in a regular new release. Or require a higher PHP version (just like 8.7/8.8 do), as long as your own API doesn't introduce BC breaks.
That said, this isn't the place to have that discussion,
Comment #167
MixologicNo, that is incorrect.
Only if those projects actually have any deprecations. Right now there are thousands of modules that don't have any usages of deprecated code.
Please refer to #147 - #149
Comment #168
markhalliwellContrib isn't semver (yet):
https://www.drupal.org/docs/8/understanding-drupal-8/understanding-drupa...
No, this has just been a dependency hack for the lack of this issue.
Also, to reiterate, themes are not able to do this. When a theme specifies "8.x", it effectively MUST mean >=8.0.0.
This is why we haven't been able to remove any deprecated APIs from our codebase, simply note them with @todos.
This issue should treat all of 8.x contrib like that though.
If a project, of any kind, never used any of the deprecated APIs that will be removed in 9.x, then sure... they don't have to create a new branch (major) and could easily change to the semver constraint.
That does seem, however, extremely unlikely.
Then what's the point of using semver then?
I mean seriously, that is literally the point of it.
Just because a project could, theoretically, work in both doesn't mean it should be "automatic".
Nor are the chances of this actually happening automatically in contrib without requiring at least some minor removal of deprecated code.
This is just adding another Drupalism key to bypass this process.
If someone has the time to add this additional key, they can easily create a new branch (major) in the same process.
No need to shoot ourselves in the foot by making it easier for others to shoot themselves in the foot too.
Comment #169
markhalliwellI get the desire to have our cake an eat it too.
I'm just saying that we shouldn't be cutting corners just to deal with our own Drupalism.
This is a chance to start us on the right path, can we actually start it... doing it correctly?
Comment #170
MixologicI have data that we've already ran on over 7000k modules that show that its true, for about 3500 modules.
Mark, here's the plan:
https://www.drupal.org/project/drupal/issues/3069795#comment-13200882
Please note point #2, which specifically is #3005229: Provide optional support for using composer.json for dependency metadata
And specifically this
https://www.drupal.org/project/drupal/issues/3005229#comment-13203895
There is a 'right path' which is using composer.json for dependency management, and not using the .info.yml but we're too close to the end of the road for drupal 9 to *require* that path.
This issue is a shim until we have a better way, but this is definitely a smaller, easier to consume, easier to communicate to maintainers vs how much we'll need to do in the other issue.
Comment #171
Berdir> Also, to reiterate, themes are not able to do this. When a theme specifies "8.x", it effectively MUST mean >=8.0.0.
As I said, this discussion is off-topi, so this will be my last comment this.
I don't know why themes would be different from modules, but depending on a specific minor core version is done in all major contrib modules and it is how they more or less keep up with removing their deprecated code usage.
token requires 8.5, pathauto and paragraphs too and I'll likely bump paragraphs to 8.7 after the next release.
This isn't something that needs to be discussed, it has been done like that for years now. Core only supports the last two minor versions, and the older only for security updates, anyone on older versions is on their own anyway. What would be the point of contrib still supporting 8.1? See https://semver.org/#what-should-i-do-if-i-update-my-own-dependencies-wit.... Contrib modules can't really make a decision on what a minor vs patch version is but composer exposes 8.x-1.2 as 1.2.0, so IMHO, each release of a contrib module is a minor release anyway.
> This is why we haven't been able to remove any deprecated APIs from our codebase, simply note them with @todos.
No, that's not the same at all. That is about ensuring that a contrib module that hasn't been updated since 8.0 still works with 8.8, it's the opposite direction. But any contrib is free to use a new API added in 8.6, otherwise there wouldn't really a be a point in doing minor releases at all, only custom code would then be allowed to use it.
In regards to whether 9.x requires a new release or not, I'm not 100% sure, in the end, if nothing else, then the d.org infrastructure might require that for now. But even if, this would still mean that e.g. 8.x-1.9 and 9.x-1.0 (or you could also release that as 9.x-1.9, might actually make more sense) are almost identical, maybe minus your own deprecated code, and that would make continued maintenance of the 8.x releases easier.
Comment #172
markhalliwellRight, ok.
Comment #173
dwwProbably unhelpful, but I'll repeat this from #2807145: [policy, no patch] Allow contrib projects to specify multiple major core branches:
I still believe that expanding
core
support in .info.yml files is the wrong approach. I think D9 should completely ignore 'core' (and we shouldn't add a new key under any name). If there's no composer.json file (per #3005229: Provide optional support for using composer.json for dependency metadata) D9 should only look atdependencies
in the .info.yml file. Contribs that want to support 8.8.x+ and 9.0.x should say something like:I haven't tested exactly what "older" versions of core do if
dependencies
isn't in a recognizable format, but I suspect it's less severe than the hell scenario from #138.Yes, @catch said (in #77) that we've got a multi-decade "project" to not require system module. Good luck with that. ;) Especially not before 9.0. If all we're talking about is the "easy" way to get this case working without composer.json, I fail to understand the benefit of keeping 3 possible dependency mechanisms working (
(drupal/)?core
,dependencies
and composer.json) in D9. Let's killcore
and rely ondependencies
.Thanks,
-Derek
p.s. And unless someone's planning to fix a lot of other problems in a relatively short amount of time, those contrib releases are still going to have version numbers with some version of core embedded in them. :( Is it more or less of a WTF to have some "8.x-2.2" release that's actually also compatible with 9.0.0? I know composer doesn't care about any of that, and will do whatever the constraints say, but if we're not requiring composer, and humans are still downloading tarballs from d.o, I fail to see how they're going to make sense of this crap when looking at a project page. I understand (as well as anyone) that big changes require many small steps, and this is part of a larger effort to get us to pure semver for contribs. Yay. But as an interim "solution", are we actually doing anyone any favors trying to support a single 8.x-2.2 release on both 8.8+ and 9.0 if we still let humans download stuff themselves?
Comment #174
tedbowthis a performance improvement that i meant to include in #160
so not commenting on #160 and beyond right now
Making this is a const
This method can use a static var so it doesn't evaluate the same constraints multiple times.
there will probably be a lot of modules with
core_dependency: "^8.8 || ^9"
and in Drupal 9 core modules will probably all have
core_dependency: "^9"
So this will improve the performance of parse the info.yml files.
Also changing the names and making it protected
Comment #175
xjmThanks everyone for all your feedback on these issues!
As mentioned in #3069795-14: [meta] Improve dependency management for extensions, @tedbow, @catch and I (release managers), @Gábor Hojtsy (product manager), @larowlan (framework manager), and @Mixologic (Drupal infra team member and Composer initiative contributor) met to discuss this set of issues as they relate to Drupal 8.9/9.0. In that meeting, we finalized/reconfirmed a couple decisions:
It is a requirement for our continuous upgrade path that a contributed or custom project must be able to work with both Drupal 8.9 and 9.0 using the same codebase (i.e., without requiring the maintainer to create a new branch). This requirement is also documented in #2608062: [META] Requirements for tagging Drupal 9.0.0-alpha1 (point 3 of the proposed resolution).
Our long-term goal is to replace the dependency management functionality of
.info.yml
files withcomposer.json
, as per #3005229: Provide optional support for using composer.json for dependency metadata. However, we agreed that it would be too disruptive in too short a timeframe to deprecate.info.yml
dependency management for removal before 9.0.0. For the multiple branch compatibility, we prefer a solution that requires the smallest change possible of contrib developers (ideally a one-line fix they can easily add).We also want to implement semantic versioning for contrib on Drupal.org as a longterm improvement to dependency management, but that's a big project and we can't risk blocking the release of Drupal 9 on it. @Mixologic has a plan for handling the transition to semver and the deprecation of the branch prefix, but that plan is beyond the scope of this core bug.
Finally, this issue itself describes a long-standing critical bug that needs to be fixed in Drupal 8 (including 8.7 and 8.8) as non-disruptively as possible, regardless of long-term API additions.
Regarding #173, @tedbow has done a good bit of research related to the
dependencies
key, and he, @Mixologic, and I have also talked about it a bit yesterday and today. We've found that the existing dependency key implementation has the following issues:system.module
in order to declare core version requirements, as this isn't actually what the module developer needs/means. (The decade-long quest to removesystem.module
isn't in danger of being completed soon, but we also don't want to make it take even longer than it otherwise would.) ;).info.yml
dependency functionality. (As per point 2 above.)Given all the above things, we prefer a single one-line API addition for the core version constraint (and a plan to deprecate the old core key later, which we're outlining in #2807145: [policy, no patch] Allow contrib projects to specify multiple major core branches) over an implementation to fix it with the dependency key.
Phew, lots of words! Thanks to @tedbow and @Mixologic for helping make sure we have all this documented here on the issue.
We'll also update the issue summary of #2807145: [policy, no patch] Allow contrib projects to specify multiple major core branches to incorporate the high-level information and the longer-term plans.
Comment #176
tedbowI was thinking about removing this check because existing sites could have modules that are not enabled and have invalid values in
core:
, maybe they were started and never finished. This currently will not cause a parsing error so the modules page will still load.This seems like very edge case though. Also since we are asking developers to use a new
core_dependency
key we should be careful that if they accidentally place the new value incore:
while they are making this change it doesn't mess up their site with #138So I think this new check is good. Adding new test coverage for it.
Comment #177
MixologicLets refactor all instances of the semver check into the parser, and add a flag for "compatibility" that we check instead. That way we only have one place where drupalsemver gets used.
This should probably be 8.8.0, unless we plan on backporting this to 8.7.x ?
Comment #178
catchJust catching up on the past 30+ comments, but to me the new core_dependency (regardless of eventual naming) seems like a good compromise. And more than anything I'm extremely glad we realised about the silent-quasi-disable issue before rather than after commit.
Comment #179
tedbowRight now Drupal.org's packaging has some old code that adds the
core:
key if it is missing in info.yml files. Right now there should always be acore:
key but this issue makes thecore:
optional or evenNot allowed in this case. If d.o packaging added the
core
back in then you could enable a module that hadcore: ^8.8
because Drupal 8.7.x would ignorecore_dependency
This would change current functionality. Right now as long as you have the
core:
\Drupal\Core\Extension\ExtensionList::getList
will not throw an exception when you get the list of modules. So the modules page and update.php will not throw an error. On the update.php this does lead to #2917600: update_fix_compatibility() puts sites into unrecoverable state so maybe we be would better off throwing the exception in\Drupal\Core\Extension\InfoParserDynamic::parse()
. This would be an even more common problem if developers start to try to usecore: 9.x
whether we actively support it or not.I just wanted to point this out before we change this behavior. Right now we have explicit tests that a module shows up on the modules page even it has the incorrect value in
core:
.I think we need some clarification from Release management on whether we want to backport this to 8.7.x.
From the comment in #3069795-14: [meta] Improve dependency management for extensions about the call with @catch + @xjm(release managers)/@larowlan/@Gábor Hojtsy/@tedbow/@Mixologic we were going to backport this to 8.7.x. But that is before we figured out we can't use the existing
core:
key.But if we don't backport this to 8.7.x then you can't do
When 8.8.0 is released 8.7.7 will still be supported as far as security coverage. Hopefully we will have a lot of modules using the new
core_dependency
key to denote that their module is compatible with Drupal 9. Probably a lot of these modules will not have used the things we have deprecated in 8.8.x but used the things we deprecated in 8.7.x so it would be good if they could mark modules compatible with > 8.7.7. That way all those sites could use the new modules(because probably they would also have other fixes not related tocore_dependency
But of course it depends on whether we think this is too disruptive to backport
Comment #180
MixologicRe: 177.1 I dont think this should change any functionality. Easier to express what I mean with an interdiff/patch
Comment #181
tedbow@Mixologic ok that makes sense I thought you wanted to throw an exception in the parser.
We can just check
!empty($module_data[$module]->info['core_incompatible'])
We don't actually need to check
core_dependency
at all.Don't need to test
core_dependency
at all.Comment #182
tedbowComment #184
tedbowOk
\Drupal\Tests\system\Functional\Module\DependencyTest::testCoreVersionDependency()
failed because we were using thesystem_test_system_info_alter()
to changecore_dependency
. Of course nowcore_dependency
already evaluated to setcore_incompatible
so that won't affect whether a module can be enabled.I think using
hook_system_info_alter()
to alter thecore
orcore_dependency
keys is something that probably doesn't happen outside of core testing.I wonder if we should even check for this in
\Drupal\Core\Extension\ExtensionList::doList()
where the alter is called and throw an exception.The other thing we could do is actually set
core_incompatible
after the hook. Since it really isn't parsing the info file sincecore_incompatible
would never be in the info file itself. Doing this.Comment #186
tedbowThe update module tests failed because they only call
\Drupal\Core\Extension\InfoParserDynamic::parse()
and not\Drupal\Core\Extension\ExtensionList::doList()
socore_incompatible
was not set.\Drupal\Tests\system\Functional\Module\DependencyTest::testCoreVersionDependency()
mentioned in #184 we can add more comprehensive test coverage toInfoParserUnitTest
because that is wherecore_incompatible
is not being set. Everything else is responding to that.So I moved the dynamic test cases from
\Drupal\Tests\system\Functional\Module\DependencyTest::testCoreVersionDependency()
where it was testing different major minor combinations to a new\Drupal\Tests\Core\Extension\InfoParserUnitTest::testCoreIncompatibility()
with dataProvider for these cases make surecore_incompatible
is set correctly in these cases.I also renamed
\Drupal\Tests\system\Functional\Module\DependencyTest::testCoreVersionDependency()
totestCoreCompatibility()
and different modules instead ofsystem_test_system_info_alter()
to alter the info.Comment #187
MixologicThis appears to me that we're now requiring core_dependency in order to run updates. It also seems like there's maybe there's test coverage missing for update_check_incompatibility(), but that might just be that its covered some other way. But probably that check can be removed entirely, as it could either be
core
, orcore_dependency
Otherwise there's the two minor phpcs errors (a vestigal use for drupalsemver and an extra blank line ).
I think if we address those things, this is pretty much RTBC AFAICT, IMO. YMMV.
Comment #188
claudiu.cristeaFrom IS:
Why not supporting a special value in
dependencies
?Comment #189
MixologicRe #188 Please refer to #175 as we've already exhaustively covered that.
Comment #190
tedbowThis is the only place we are actually using
DrupalSemver now besides it's own test. We can remove it from the patch and just catch the exception here from
\Composer\Semver\Semver::satisfies
Comment #191
eelkeblokRe #188 and #189, to be fair, I don't think using a special core entry in dependencies was discussed before. What was discussed is using drupal:system as a hack/workaround to depend on a certain core version. Although I quite like that idea from a semantic POV, I think it will come with its own set of downsides. The only downside I "feel" for the current approach is that the name of the key - core_dependency - is less than elegant, but I don't have a better suggestion.
Comment #192
tedbowre #187
core_dependency
this didn't cause a test failure because in\Drupal\Core\Extension\InfoParserDynamic::parse
There was
$parsed_info['core_dependency'] = $parsed_info['core'];
This was needed so that we could just check
core_dependency
in multiple places and worked because8.x
is valid for\Composer\Semver\Semver::satisfies()
But now we are just setting
core_incompatible
we don't needcore_dependency
after the parser.re #190
re #188 and #191
it is true that is slightly different suggestion but I think it still runs into the same existing problem we have with our current logic that evaluates our
dependencies
values documented in #175.dependencies
because it does not correspond to a module but also that it accepts different values. Unless we want to run it through our existing broken logic we would have to not accept values that other dependencies accept such as>= 8.7.x
. That value would throw an exception in\Composer\Semver\Semver::satisfies()
but not for our existing dependencies.^
or||
would be valid inbut in no other key under
dependencies
-drupal:system (8.6.x)
that is very similar but again actually would use the existing constraint format.So we would have
We could change this to be
But I think the other problems are confusing enough not to use this.
Other problems fixed in self-review
This was a copy/paste error. The reason it didn't cause a test failure was because
\Drupal\Tests\Core\Extension\InfoParserUnitTest::testCoreDependencyInvalid()
wasn't testing this with a release with a suffix. So adding a dataProvider to test this.Adding check for alpha2, beta2 to rc20.
The reason the test method here accepts a file name is if the test is called multiple times and uses the same file path with a vfsStream somehow the file is cached and it picks the same file contents. I have tried just deleting the file at the end of the test method but it doesn't work. Using a different filename ensures each call of the test method is dealing with a unique file.
Comment #193
Wim LeersMy last comment is now from 10 days ago, in #158. Caught up.
#160: that interdiff is a very valuable safeguard and clarity improvement! 👍 (Clarity improvement because it means that the "both" scenario in #155 and #157 ceases to exist.)
#175: Thanks for this super helpful summary of the current status, considerations and issue relationships! 🙏
#176.1: Very thoughtful 👍
#180: that's piggybacking on what is just a parsed
*.info.yml
file to carry derivative data as if it were parsed too. Weird and smelly, but perhaps justified in this case.Patch review
👍 This decouples
update.inc
from the currentcore
*.info.yml
key.But…
🤔 See the piggybacking remark above.
Nit: missing comment.
👍 This is now a
protected
method rather than a public method on a non-internal class, which would've meant an increased API surface. Yay for not exposing additional API surface!Nit: this could be
static
, which would guarantee this is a side effect free function.Nit: extraneous blank line.
Nit:
</span>
is in a weird place. Shouldn't that have been just before the closing parenthesis)
?Nit: mismatch.
Nit:
string
typehint👍
Comment #194
tedbow@Wim Leers thanks for the review!
core_dependency
.I think another option would to be to set
core_incompatible
in\Drupal\Core\Extension\ExtensionList::getList()
and then also do the same logic inupdate_verify_update_archive()
to determine if it is compatible, you won't need to setcore_incompatible
here.incompatible with
with the span is done in the existing core compatibility message and for a module not being compatible with another module.a couple other things
This can be determined in Drupal 9.
I think
core_version_requiement
is more explicit.\Drupal\Core\Extension\InfoParserDynamic::FIRST_CORE_DEPENDENCY_SUPPORTED_VERSION
will have to be updated accordinglyComment #195
Mixologicre #194.1 / #193 Piggybacking - it does look like we're shoehorning some logic into what ought to be just be a parser, but also, it kinda feels like the internal API for this is somewhat all over the place, and could probably use a redesign of sorts.
Remaining tasks:
1. ++ agree
2. @drumm was looking at that possibly today
3.
core_version_requiement
is superior. ++4. Above my paygrade
Comment #196
Wim LeersAgreed wrt piggybacking/shoehorning versus "already all over the place". That's why I'm not feeling strongly about it, but I think we should at least be conscious about it. I think this means we are consciously choosing to do this, since we don't see an obviously better way with the same level of low disruption.
drupal: …
instead ofcore_dependency: …
orcore_version_requirement: …
. But I defer to the majority's opinion. I think the best way forward here is to just let core committers (release managers + framework managers) decide this.For points 3 and 4, marking this RTBC and adding necessary tags to ensure this issue can move forward quickly, since it's in the critical path for Drupal 9.
Comment #197
tedbow@Wim Leers thanks for RTBC'ing this.
I going to do hopefully 1 more patch.
core_version_requirement
is better changing. Most of the interdiff is thisstatic and protected are out of order
This was for
\Drupal\Tests\system\Functional\Module\DependencyTest::testCoreCompatibility()
that was changed in #184. Since that test no longer relies onsystem_test_system_info_alter()
to alter this(it uses actual test modules now and expanded coverage in\Drupal\Tests\Core\Extension\InfoParserUnitTest::testCoreIncompatibility()
) this is no longer neededThis should also assert that 'core' is not set
Wrong comment. This is testing for invalid
core
value.This existing test just uses core. I going to add asserts to show that 'core_version_requirement' is not set since earlier in the patch
core_dependency
, if missing was being set to the corecore
value. Since we are settingcore_version_requirement
in the parser now we don't need to do this and should test for it.Comment #198
Wim Leerscore_version_requirement: …
instead ofcore_dependency: …
. That's the current majority. I'm fine with that.I grepped the patch for "core dependency" and still found some variable names that used that:
These all contain 'core_dependency' or 'CoreDependency'. I think we should update those too.
Marking
for point 1. Then back to RTBC per #196.Comment #199
tedbow@Wim Leers thanks. Fixed
Comment #200
Wim LeersComment #201
tedbow@Wim Leers thanks again but sorry to kick my own patch from RTBC twice in 1 day.
I found a weird bug when testing.
I was changing my test module to use
This should work but I was getting the exception
I figured out that this only happened if the 2nd module that used the exception not the first the problem is coming from:
Setting this to FALSE should actually happen before the bracket. Because it should after the foreach loops for all the version possibilities but outside of the if statement
if (!isset($evaluated_constraints[$constraint])) {
Basically any constraint that was sent to this method 2x would always return FALSE after the first. Of course this would not cause problems that would have returned FALSE anyways, just ones that should return TRUE.
This is the test that would fail if we ran it 2x.
I will fix this.
I am including a patch with just the test fix to show it fails
There a couple other things I want to fix with that test related to this but I won't to keep the interdiff as small as possible now.(i.e. it is not RTBC ready I think😜)
Comment #203
tedbowExplanation of test changes
We have to create a duplicate file because
\Drupal\Core\Extension\InfoParser::parse()
will cache by file names.expectException()
won't work for us here because we want to make sure it gets thrown on subsequent calls too.We need to nest the second call inside the first one's catch to make sure the exception is thrown each time.
We have a fail at the end of the test. If the exception is thrown for both calls and the message matches it will return earlier.
Comment #204
tedbowActually
expectException()
andexpectExceptionMessage()
only don't work for the 1st call toparse()
.Changing to use these for 2nd call because this also allows not using the explicit call to
fail()
to determine that one or both calls toparse()
did not cause an exception. This also allows the tests to only 1 try/catch and not a nested try/catchexpectException()
andexpectExceptionMessage()
have to be called before the try/catch to make sure they aren't called because the firstparse()
doesn't throw an exception.$evaluated_constraints[$constraint]
was set to FALSE. If it was set to TRUE then it would have been different bug. For that reason I am changing the other new test methods to always callparse()
2 times. This proves that in all these cases that the parser will parse the same content the same way called multiple times on different files. This protects use against other bugs that could happen from the fact that\Drupal\Core\Extension\InfoParserDynamic::isConstraintSatisfiedByPreCoreVersionRequirementCoreVersion()
caches the evaluations.There is actually no need to set
$filename
here as 1) it is not actually a file name and this value can just used to callparse()
direcltyThe parameter $file_name is actually miss named. It is used to make the file name is unique but it should really be named $test_case.
Comment #205
tedbowI update the change record https://www.drupal.org/node/3070687
Currently it assumes the patch will be back ported to 8.7.x and released in 8.7.7
Comment #206
MixologicAll of the additional test coverage is solid, catches additional weird subtle bugs, and in general makes this patch feel like its gold plated.
Moving back to RTBC
Comment #207
catchI'm +1 on backport to 8.7 and I think both me and xjm have been involved enough in this issue to remove the 'needs release manager review' tag. I'm about a week+ behind on the actual patches here so no comment on those yet.
Comment #208
Wim Leers@catch in #207 answered point 4 in #195/#196/#197. That means this issue needs work: it needs both an 8.8 patch (RTBC'd in #206) and an 8.7 patch (newly needed, the current patch only applies to 8.8). The change record was already updated in #205, because @tedbow correctly predicted that this is what release managers would say 👍
Once that 8.7 patch exists, that leaves only point 3 in #194/#195/#196: the name of the key aka bikeshedding 🚲🏠🎨🎨🎨🎨🎨. The current patch is going with
core_version_requirement
. We'll see what others think 🤓So once the 8.7 patch exists, this should be RTBC again.
Comment #209
tedbowHere is a 8.7.x patch. The interdiff just has the only change that wasn't part of the roll but an actual needed change because
This test case won't pass because the constraint would be
~8.7
which would include versions before 8.7.7.Every minor after this won't have this problem so it passes 8.8.x and all branches moving forward.
Comment #210
Wim LeersPer my comment in #208. #209 is the 8.7 patch, #204 is the 8.8 patch.
Comment #211
larowlanCode review
We need an issue summary update here, because it references the old name
We can't have string typehints in 8.7 because php 5?
same here re php7 features
can we get some comments here as to what this code is doing, out future selves will thank us - this is the bit of the code that worries me most from the framework manager review POV, but mostly because I'm not certain what it's doing. Also, I think we should be profiling this change.
in one instance we use empty, in another we don't - should we be consistent? the empty check is more defensive
any reason we can't use the existing key?
php 7 features again
Comment #212
Wim Leers#211 pointed out many occurrences of PHP 7-only syntax. (👍👏— sorry about that!) I wanted to queue PHP 5 testing for the patch in #209. But I'm being met with a 403 :( I left a message in the
#drupal-infrastructure
Slack channel about this.Comment #213
tedbow@larowlan thanks for the review.
Re #211.4
I have an idea how to make this simpler. Will work on it in about an hour.
Comment #214
tedbowre #211
but that is only in 8.8.x not backported to 8.7.x. Does it need to be fixed anyways?
getAllPreCoreVersionRequirementCoreVersions()
now gets all possible versions and does not evaluation of the constraint. It has more comments and uses a static variable so the versions array is only created once no matter how many modules you have.isConstraintSatisfiedByPreCoreVersionRequirementCoreVersion()
now simply loops through the versions and breaks when it finds the first 1 that satisfies the constraint.Agreed with the profiling I will do that after this patch.
But a couple reasons I don't think will be a performance problem.
core_version_requirement: ^8
since this the same constraint stringsisConstraintSatisfiedByPreCoreVersionRequirementCoreVersion()
will only evaluate 1 time for all for core modules when\Drupal\Core\Extension\ExtensionList::doList()
is called and all info files are parsed.- drupal:system (>8.6)
constraint so probably not a ton of modules will use custom constraints. The most common would probably be^8
,^8 || ^9
and^8.8 || ^9
(the last D8 version with deprecations). That still would only result inisConstraintSatisfiedByPreCoreVersionRequirementCoreVersion
being evaluated 3x for all modules that use 1 of these^9.1
and fix a major bug in 8.8.10 we will get a bunch of^8.8.10
These points made think of another optimization. One reason we want this check in is avoid developers doing
core_version_requirement: ^8.7.3
because this won't work for 8.7.3. For this case if we start checking version with 8.0.x we will have to go through a lot more versions to find one that satisfies the constraint than if we start checking versions at 8.7.6 and then go down.In the other common cases of
^8
or^8 || ^9
we get version that satisfies in the first version evaluation whether we start from the most recent or not.Therefore in the new
getAllPreCoreVersionRequirementCoreVersions()
I doing$versions = array_reverse($versions);
before I return the versions. But again this reverse only will every happen 1 time in a request.$module_data[$module]
itself might not be set. We can't actually throw an exception in that case($module_list contain an entry that doesn't exist) because that would change current functionality. We skip modules that don't exist that are sent to\Drupal\Core\Extension\ModuleInstaller::install()
. that doesn't seem like good idea but changing it would be a BC break.In the 2nd case
$module_data[$dependency]
will always be set because above itwhich was pre-existing logic.
in either case if we have module data
->info['core_incompatible']
should always be set because it is always set in the parser.Comment #217
tedbowincompatible_core
was used somewhere else. Replaced.Comment #219
tedbowThe 8.7.x patch for #217 failed because of failing test in 8.7.x branch. Retesting
Comment #220
Wim LeersRE: performance. I think actual profiling of this may be overkill.
\Drupal\Core\Extension\InfoParserDynamic::getAllPreCoreVersionRequirementCoreVersions()
which computes the list of all pre-8.7.7 versions to compare against later. This runs at most once per request, only on requests that are parsing*.info.yml
files. Per https://3v4l.org/FvUJA/perf#output it takes <=50 ms to compute the list of 611 versions to compare against.Semver::satisfies()
logic and its dependencies, plus the version comparision logic thatInfoParser
runs.On my machine:
it takes 1.7 seconds in an insane worst case scenario and only dozens of milliseconds in common scenarios.
So IMHO this is fine performance-wise. Especially considering that PHP 7.2 is the slowest version of PHP that still has security support in a few months. On PHP 7.3 and 7.4 this is going to be even faster!
Comment #221
MixologicisConstraintSatisfiedByPreCoreVersionRequirementCoreVersion
still feels like an overly complex solution to shield us from edge case maintainer mistakes. We seem to be doing a lot of work to see if the maintainer attempted to put something *earlier* than 8.7.7.I don't think the complexity is worth it.
What if we checked to see if they have *both* a
core
key and acore_version_requirement
key set, then the *only* acceptable value forcore_version_requirement
is^8 || ^9
in that case? ^8 by itself is redundant and unnecessary.The only scenario that I can imagine happening, but one that I do not believe needs any sort of defense, is if a maintainer puts something like
core_version_requirement: ^8.5
with nocore:
keyIn this case, it wouldn't be installable on 8.5/8.6/8.7.6, but would on any later versions.
I believe that's perfectly fine, somebody trying to upgrade would open an issue with the maintainers to correct the mistake and it would sort itself out.
Comment #222
tedbow@Mixologic
As you sadi in #147
This would mean for these modules they couldn't specify that type only work with limited versions in Drupal 9. I know that we try to make sure our minor versions don't break BC but 💩 happens.
A lot of modules really on things we mark as
@internal
or that aren't marked as such but according our BC policy our internal. Allows we consider form or render arrays internal but a lot of modules alter them so would break if we changed them. If we do that a module should be able to specify they are compatible with all version of Drupal 8 and limited version of Drupal 9(or all of 9 but limited 10 🙀)So even if they have the
core: 8.x
or values besidescore_version_requirement: ^8 || ^9
would be useful.re
I believe we don't prevent this it will be very common. Because for a module maintainer this is very reasonable thing to want to do if you know you remove all your deprecations and they removed in 8.4 or really more likely 8.6 or 8.5 or before. If don't read the change log carefully(i almost never did) then you probably could think you could do this.
I think if we can prevent all those issue from being created with something like
isConstraintSatisfiedByPreCoreVersionRequirementCoreVersion
then we should.Also in #3005229-82: Provide optional support for using composer.json for dependency metadata which is being worked on top of this I refactored this to a more generic
isConstraintSatisfiedByPreviousVersion ()
because we are going no need something this to make sure if the module is compatible with versions before 8.8.0 they need to specify core in the info file.Comment #223
Wim LeersI would usually agree. But in this issue we're dealing with a potential ecosystem disruption. In such a case, and anything we can do to A) minimize that disruption and B) maximize our confidence is worth it IMHO.
Comment #224
tedbowI chatted with @Mixologic more about this issue.
isConstraintSatisfiedByPreCoreVersionRequirementCoreVersion()
for the purposes it used for #3005229-82: Provide optional support for using composer.json for dependency metadata than it is in this issue. I will comment on that issue with scenarios that we discussedThe "611 unique constraints to check" would mean you have 611 modules in a site code base and every single 1 had a unique value for
core_version_requirement
. That would me there would be no 2 modules that the same value forcore_version_requirement
. So for all 611 modules only 1 could havecore_version_requirement: ^8 || ^9
and only 1 could havecore_version_requirement: ^8.8 || ^9
. Just with those 2 constraints that would be probably most modules.So tested with
I think only 50 and 25 are probably going to be at all common scenarios and those numbers seem fine.
Uploading a updated test.php
This changes
isConstraintSatisfiedByPreCoreVersionRequirementCoreVersion($constraint)
toisConstraintSatisfiedByPreviousVersion($constraint, $version)
in #3005229 this allows using the same logic for checking for pre 8.8.0 support(because composer.json changes probably not back ported to 8.7.x) that is used for checking pre 8.7.7 support here.
I also think it is clearer because the functions don't need to be written around a specific version, 8.7.7.
Comment #225
Gábor HojtsyCleaned up the issue summary as it was a very hard read for me with many verbs missing. Please double check that it is still accurate. Also removed a completed d.o dependency and added a release notes snippet.
Comment #226
tedbow@Gábor Hojtsy thanks for updating the summary
doing a couple other updates
core_version_requirement
From remaining tasks. This got a plus 1 from @Mixologic. obviously if someone has better idea it would great to hear it. But I think the current name is fine.
FIRST_CORE_VERSION_REQUIREMENT_SUPPORTED_VERSION
in the patch already references 8.7.7(obviously will need updating if this doesn't get in till 8.7.8 or later)Currently the core key is totally ignored
I previously created this follow up: #3070401: Install profiles do not support multiple core branch compatibility
With this patch both the
core
key and the newcore_version_requirment
would be ignore for installing profiles except The validation in the parser forcore_version_requirment
andcore
combinations would still happen. So you could still get a parsing error if you have invalid values but it won't actually stop you from installing a profile withcore_version_requirment: ^8.9
orcore: 7.x
(this is an existing problem).My vote would be that we fix this is in #3070401: Install profiles do not support multiple core branch compatibility and add a logic to the parser to throw an error if
core_version_requirment
is used in a profile .info.yml file.Adding that to the patch.
Comment #227
Wim Leersinfo.yml
files are processed is certainly out of scope here!This was un-RTBC'd in #211 for profiling (done: see #220 and #224.3) and nits (fixed). After that, @Mixologic raised complexity concerns in #221. @tedbow and @Mixologic had a very long chat in Slack on August 26 about this, which resulted in @Mixologic becoming convinced too, then just concluding we'd need a better method name. @tedbow addressed that in #224.
AFAICT everything has been addressed. Back to RTBC! 🤞
Comment #228
catchNit: stray 'the' or missing 'key', probably
the 'core' key
is better, fixable on commit.Nit: missing '[specify compatibility] for a [specific version]' - fixable on commit.
We should have a follow-up to remove this logic. I know there are plans to deprecate .info.yml for version dependencies altogether, but I think we could remove pre-8.7.7 support in 9.2 when Drupal 8 EOL is reached.
Comment #229
Wim Leers#228.3++ — I thought about that at some point but failed to write it. Great point!
Comment #230
catchHere's that follow-up #3077703: Remove pre-8.7.7 core compatibility checks in extension parsing.
Comment #233
catchFixed #228.2 and #228.3 on commit and pushed to 8.8.x and 8.7.x, yayyyy!
There was some discussion here whether we should attempt to backport this to 8.6.x. I'm neutral on this (would tend towards it not being worth the effort) but leaving the issue open so that the discussion can happen.
Comment #234
catchOh we also need a follow-up for updating all the core modules to be in line with this now that the supporting code is in 8.8 and 8.7, I think earlier versions of the patch had those changes already.
Comment #235
tedbow@catch thanks for committing this! 🎉
Also thanks to @Mixologic, @larowlan and @Wim Leers for the all the reviews! And for everyone else you helped out especially @drumm for fixing #3075062: Remove core key overriding in .info.yml packaging which had to be fixed before this.
re #234
I will repurpose this issue #3072702: Core extensions should not need to specify the new core_version_requirement in *.info.yml files so that 9.0.x can be installed for that.
Comment #236
tedbow😫
Ok I was looking at doing #3072702: Core extensions should not need to specify the new core_version_requirement in *.info.yml files so that 9.0.x can be installed
which would now mean using the new
core_version_requirement
key in all module/theme info.yml files.so my first thought was to use
With no core key which actually works. but then I released this would not be a good example because any developer that was creating/updating a module and looking at core for an example might use this.
Of course first looking at this you would think
core_version_requirement: ^8
means that this is the same ascore: 8.x
.But of course as we have seen in this issue this would actually not work on versions of Drupal before this issue committed because it would throw an
InfoParserException
.Of course if you were developing on 8.8.x or 8.7.x or 8.7.7(after it is released) this would be hard to figure out(and we missed it here 😿)
So I think we need a follow up to fix this(or revert ). We should throw an exception if you do
with no core
Just as we throw exception if you do
if the first case it would make you think it would work on all versions of Drupal 8(it won't)
In the second case would think it only works after 8.8.0(it actually works with all Drupal 8 versions) and we do throw an error.
Anyways here is what the fix would look like to explain better.
Comment #237
tedbowre #233
I was thinking about this and this also be complicated.
Right now
Right now this the "first" and earliest version but also because no 8.8.x release have been made yet all the versions above it would support the
core_version_requirement
key.If we backported this to 8.6.x then the earliest release with support would be 8.6.18 but not all release above that would support the key, for instances 8.7.0.
It would make when we throw
InfoParserException
much more complicated I think.Comment #238
catchThat's a very good argument not to backport to 8.6.x
On #236 this seems like a sensible fix, even though core modules can't be used with different versions of core, they should be able to set a good example for contrib.
Comment #239
tedbowHere is the 8.7.x version of #236. It is exactly the same except for
\Drupal\Tests\Core\Extension\InfoParserUnitTest::providerCoreIncompatibility()
because of the change mentioned in #209.I updated #3072702-6: Core extensions should not need to specify the new core_version_requirement in *.info.yml files so that 9.0.x can be installed but I am not actually sure we want to make any changes there. Updated issue with why.
Comment #240
catchGiven #239 doesn't block #3072702: Core extensions should not need to specify the new core_version_requirement in *.info.yml files so that 9.0.x can be installed any more, I think we should move this to a follow-up and mark this issue fixed. It's not causing a new problem that we'll need to rollback for, it's just one more case we can test for to help contrib get things right.
Comment #241
Wim LeersRE: commit. Wonderful! 🥳
RE: core being an example for contrib modules. This is usually true, but not in this case. That is indeed tricky. I think @Berdir posted a great analysis and proposal in #3072702-9: Core extensions should not need to specify the new core_version_requirement in *.info.yml files so that 9.0.x can be installed, that can alleviate the concerns here.
EDIT: cross-posted with #240. Agreed. Assigning to @tedbow for that.
Comment #242
Gábor HojtsyOpened #3077942: If 'core_version_requirement' specifies compatibility with all versions of Drupal 8 like '^8' the 'core: 8.x' must also be set and transported @tedbow's patches (and issue credit) there :) (Also cross-posted:)
Comment #243
tedbowCreated another follow-up #3078001: Don't catch exception for invalid 'core_version_requirement' in info.yml files
Comment #244
Gábor HojtsyI opened a d.o followup at #3078111: Project browsing/search compatibility filter should work off dependency data since now we need a way to be able to find 9.x compatible projects that are not 9.x branched :)
Comment #245
mondrakeFollow up needed?
I tried changing the key in a contrib project that I'm maintaining, making it to be supported only on 8.8.x or 9, https://git.drupalcode.org/project/imagemagick/blob/8.x-3.x/imagemagick.....
Then run its tests with Drupal 8.7.x.
FunctionalTests correctly fail with the incompatibility message, https://www.drupal.org/pift-ci-job/1389844, however it looks like KernelTests are not making any check and module gets installed in the SUT.
Comment #246
tedbow@mondrake thanks for reporting this.
This is an existing problem. I am not sure if this is by design as far as kernel tests.
I reverted back to the commit right before this 1.
I tested
\Drupal\Tests\node\Kernel\NodeAccessLanguageTest::testNodeAccess()
node_access_test
node_access_test.info.yml
I did get an exception from\Drupal\Core\Extension\InfoParserDynamic::parse()
about the missingcore
key7.x
I don't get an error\Drupal\simpletest\KernelTestBase::enableModules()
not the module installer so they don't check for core compatibility but also probably could be enabled without their dependencies.In fact it says the comment about "Dependencies are not resolved" in the comment.
I am not sure if we need to fix this I am sure kernel tests are meant to do this check.
Comment #247
mondrake@tedbow yeah, maybe the current state is on purpose. Nevermind. FWIW I just found #1273478: Implement @requires and @dependencies within TestBase, mark tests as skipped.
Thanks
Comment #248
Gábor HojtsyComment #249
MixologicThe backport introduced some php5.5 and php5.6 regressions:
expectException doesnt exist in phpunit 4, so the backport tests fail.
https://www.drupal.org/pift-ci-job/1397533
Comment #250
MixologicI see a lot of this pattern in core currently:
Which seems to be the correct workaround to address those.
Comment #251
xjmLet's hotfix this in 8.7.x only since 8.8.x won't be affected. (8.7.x is even on PHP 7 https://www.drupal.org/pift-ci-job/1397539 because of the way the PHPUnit bridge works I guess.)
Comment #252
tedbowI didn't want to say anything before but I was really wishing this issue would get to over 250 comments. Wish fulfilled.
Here is the patch
Comment #253
xjmThis looks good to me. @Mixologic pointed out that the test regression for this issue will cause the other to fail on 5.6 and vice versa, so I opened #3079805: expectedException() usage in two pre-8.7.7 commits has broken PHP 5 testing for 8.7.x to fix them both at the same time. I think the approach used here is the best one but we can discuss both on that issue. :)
Comment #255
catchComment #256
xjmComment #257
xjmAdjusting the release note to make the CR link accessible. Looks good otherwise, thanks!