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

  • Determine if profiles .info.yml files should be handled in a follow up issue.
  • 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.

    CommentFileSizeAuthor
    #252 2313917-252.patch5.52 KBtedbow
    #252 2313917-252.patch5.52 KBtedbow
    #239 2313917-follow-up-8.7.patch5.49 KBtedbow
    #236 2313917-follow-up.patch5.61 KBtedbow
    #226 2313917-226-8_7.patch47.87 KBtedbow
    #226 2313917-226.patch48.36 KBtedbow
    #226 interdiff-224-226.txt2.25 KBtedbow
    #224 test.php_.txt31.37 KBtedbow
    #224 2313917-224.patch47.27 KBtedbow
    #224 interdiff-217-224.txt5.29 KBtedbow
    #220 test.php_.txt30.86 KBWim Leers
    #217 2313917-217-8.7.patch46.18 KBtedbow
    #217 interdiff-217.txt804 bytestedbow
    #217 2313917-217.patch46.68 KBtedbow
    #214 2313917-214.patch46.62 KBtedbow
    #214 2313917-8.7.patch46.13 KBtedbow
    #214 interdiff-214.txt6.58 KBtedbow
    #204 2313917-204.patch45.24 KBtedbow
    #204 interdiff-201-204.txt12.04 KBtedbow
    #201 2313917-201.patch41.52 KBtedbow
    #201 interdiff-199-201.txt2.55 KBtedbow
    #201 2313917-201-test-fix-only.patch43.33 KBtedbow
    #199 2313917-199.patch40.81 KBtedbow
    #199 interdiff-197-199.txt3.78 KBtedbow
    #197 2313917-197.patch40.74 KBtedbow
    #197 interdiff-194-197.txt22.76 KBtedbow
    #194 interdiff-192-194.txt3.3 KBtedbow
    #194 2313917-194.patch40.21 KBtedbow
    #192 interdiff-186-192.txt12.72 KBtedbow
    #192 2313917-192.patch40.11 KBtedbow
    #186 2313917-186.patch42.93 KBtedbow
    #186 interdiff-184-186.txt9.08 KBtedbow
    #184 2313917-184.patch41.44 KBtedbow
    #184 interdiff-181-184.txt1.57 KBtedbow
    #181 2313917-181.patch40.65 KBtedbow
    #181 interdiff-180-181.txt2.28 KBtedbow
    #180 interdiff.txt6.57 KBMixologic
    #180 2313917-180.patch32.76 KBMixologic
    #176 2313917-176.patch41.86 KBtedbow
    #176 interdiff-174-176.txt9.25 KBtedbow
    #174 2313917-174.patch40.97 KBtedbow
    #174 interdiff-160-174.txt4.29 KBtedbow
    #160 2313917-160.patch40.18 KBtedbow
    #160 interdiff-152-160.txt7.67 KBtedbow
    #154 2313917-154.patch37.37 KBWim Leers
    #154 interdiff.txt2.09 KBWim Leers
    #152 interdiff-151-152.txt4.31 KBtedbow
    #152 2313917-152.patch36.45 KBtedbow
    #151 2313917-151.patch32.15 KBtedbow
    #151 interdiff-136-151.txt16.28 KBtedbow
    #142 interdiff-142.txt3.63 KBtedbow
    #142 2313917-142-wip.patch30.67 KBtedbow
    #136 2313917-136.patch29.96 KBWim Leers
    #136 interdiff.txt8.9 KBWim Leers
    #134 2313917-134-8.7.patch28.37 KBtedbow
    #134 interdiff-128-134.txt4.49 KBtedbow
    #133 interdiff-130-133.txt893 bytestedbow
    #133 2313917-133.patch28.81 KBtedbow
    #130 2313917-130.patch28.81 KBtedbow
    #130 interdiff-128-130.txt4.88 KBtedbow
    #128 interdiff-126-128.txt2.38 KBtedbow
    #128 2313917-128-8_8.patch23.93 KBtedbow
    #128 2313917-128-8_7.patch23.88 KBtedbow
    #126 2313917-126-no-info-changes.patch23.78 KBtedbow
    #126 2313917-126-8.7.patch23.73 KBtedbow
    #126 interdiff-113-126.txt13.42 KBtedbow
    #113 interdiff-110-113.txt2.85 KBtedbow
    #113 2313917-113.patch263.59 KBtedbow
    #110 2313917-110.patch262.83 KBtedbow
    #110 interdiff-105-110.txt2.72 KBtedbow
    #105 2313917-105.patch260.11 KBtedbow
    #105 2313917-105-no-info-yml-changes.patch11.31 KBtedbow
    #105 interdiff-102-105.txt5.03 KBtedbow
    #102 2313917-102-no-info-yml-changes.patch10.92 KBtedbow
    #102 interdiff-99-102.txt9.16 KBtedbow
    #102 2313917-102-semver.patch260.8 KBtedbow
    #100 interdiff-92-99.txt535 bytestedbow
    #99 2313917-99.patch256.39 KBtedbow
    #99 2313917-99.patch256.39 KBtedbow
    #92 2313917-92-all-info.patch256.39 KBtedbow
    #92 2313917-92-system-info-only.patch7.55 KBtedbow
    #92 interdiff-86-92.txt252.55 KBtedbow
    #86 2313917-86.patch6.62 KBtedbow
    #86 interdiff-80-86.txt5.48 KBtedbow
    #86 interdiff-85-86.txt6.13 KBtedbow
    #85 2313917-85.patch9.84 KBtedbow
    #85 interdiff-80-85.txt8.7 KBtedbow
    #82 2313917-82.patch8.31 KBtedbow
    #82 interdiff-81-82.txt938 bytestedbow
    #81 2313917-81-plus-3004459-constraint.patch18.6 KBtedbow
    #81 2313917-81-do-not-test.patch6.64 KBtedbow
    #81 interdiff-80-81.txt5.5 KBtedbow
    #80 2313917-80.patch8.69 KBtedbow
    #80 interdiff-78-80.txt1.63 KBtedbow
    #78 2313917-78.patch8.74 KBtedbow
    #61 2313917_61.patch8.97 KBMile23
    #56 semver-2313917-56.patch9.21 KBpwolanin
    #54 semver-2313917-53.patch7.96 KBpwolanin
    #49 semver-2313917.49.patch9.21 KBlarowlan
    #37 core-version-key-2313917-37.patch9.25 KBjhedstrom
    #37 interdiff.txt12.74 KBjhedstrom
    #31 core-version-key-2313917-31.patch14.58 KBjhedstrom
    #31 interdiff.txt1.41 KBjhedstrom
    #29 core-version-key-2313917-29.patch14.44 KBjhedstrom
    #29 interdiff.txt1.33 KBjhedstrom
    #21 core-version-key-2313917-21.patch7.3 KBjhedstrom
    #21 interdiff.txt4.17 KBjhedstrom
    #11 increment.txt553 bytespwolanin
    #11 2313917-11.patch5.67 KBpwolanin
    #11 2313917-11-test-only.patch3.67 KBpwolanin
    #8 2313917-8.patch5.67 KBpwolanin
    #8 2313917-8-only-test.patch1.73 KBpwolanin
    #6 2313917-6.patch3.04 KBpwolanin
    Support from Acquia helps fund testing for Drupal Acquia logo

    Comments

    pwolanin’s picture

    Issue summary: View changes
    xjm’s picture

    Priority: Major » Critical

    Critical I think.

    pwolanin’s picture

    Issue summary: View changes
    pwolanin’s picture

    Issue summary: View changes
    xjm’s picture

    A 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...

    pwolanin’s picture

    Status: Active » Needs review
    FileSize
    3.04 KB

    New test - should fail.

    Status: Needs review » Needs work

    The last submitted patch, 6: 2313917-6.patch, failed testing.

    pwolanin’s picture

    Status: Needs work » Needs review
    FileSize
    1.73 KB
    5.67 KB

    test-only plus more complete patch.

    The last submitted patch, 8: 2313917-8-only-test.patch, failed testing.

    Status: Needs review » Needs work

    The last submitted patch, 8: 2313917-8.patch, failed testing.

    pwolanin’s picture

    Status: Needs work » Needs review
    FileSize
    3.67 KB
    5.67 KB
    553 bytes

    Silly me - gave a file the wrong name.

    The last submitted patch, 11: 2313917-11-test-only.patch, failed testing.

    Gábor Hojtsy’s picture

    There 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.

    Gábor Hojtsy’s picture

    pwolanin’s picture

    Indeed - 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?

    Gábor Hojtsy’s picture

    Status: Needs review » Needs work
    1. +++ b/core/modules/system/src/Form/ModulesListForm.php
      @@ -224,6 +224,20 @@ public function buildForm(array $form, FormStateInterface $form_state) {
      +    // @todo Move this method to the \Drupal class?
      

      If we are to leave this todo in for commit, we need an issue opened and referenced.

    2. +++ b/core/modules/system/tests/modules/system_compatible_core_version_test_80x/system_compatible_core_version_test_80x.info.yml
      @@ -0,0 +1,6 @@
      +core: 8.0.x
      

      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.

    Gábor Hojtsy’s picture

    Oh, 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.

    Gábor Hojtsy’s picture

    Issue tags: +Drupalaton 2014
    xjm’s picture

    Issue tags: +TCDrupal 2014

    Discussed 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

    star-szr’s picture

    minor version blocker? :)

    jhedstrom’s picture

    Rather 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.

    catch’s picture

    We'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.

    YesCT’s picture

    Status: Needs work » Needs review

    (to send #21 to the bot)

    tstoeckler’s picture

    It seems we should be updating Drupal::CORE_COMPATIBILIY as part of this issue, no?

    dawehner’s picture

    @tstoeckler
    Well, changing that would make stuff like ModuleHandler::parseDependency() not working anymore. In theory semv would not break APIs any APIs
    in 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 *?

    Gábor Hojtsy’s picture

    Status: Needs review » Needs work

    @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).

    dawehner’s picture

    @Gábor Hojtsy
    It's primarily really confusing as the composer.json files look totally different.

    Gábor Hojtsy’s picture

    @jhedstrom still want to work on this? :)

    jhedstrom’s picture

    Status: Needs work » Needs review
    FileSize
    1.33 KB
    14.44 KB

    This 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.

    Gábor Hojtsy’s picture

    Status: Needs review » Needs work

    The dynamic version test looks much better. Except:

    +++ b/core/tests/Drupal/Tests/Core/Extension/ModuleHandlerTest.php
    @@ -514,4 +514,35 @@ public function testGetModuleDirectories() {
    +    $current = \Drupal::VERSION;
    ...
    +    $minor = substr($current, 1, 1);
    

    $minor in this patch is not a number because substr('8.0.x', 1, 1) is '.' no? Would be better to use explode(\Drupal::VERSION, '.')[1] no?

    jhedstrom’s picture

    Status: Needs work » Needs review
    FileSize
    1.41 KB
    14.58 KB

    Good catch! This fixes that issue, and also switches to using a dataProvider method.

    Gábor Hojtsy’s picture

    Status: Needs review » Reviewed & tested by the community

    Yay, looks great to me :)

    alexpott’s picture

    Status: Reviewed & tested by the community » Needs work
            $theme->incompatible_core = !isset($theme->info['core']) || ($theme->info['core'] != \DRUPAL::CORE_COMPATIBILITY) || !isset($theme->info['regions']['content']);
    

    What about this code for themes? In SystemController::themesPage() - it would be nice to handle extensions (modules, themes and install profiles) together somehow.

    jhedstrom’s picture

    Currently 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?

    markhalliwell’s picture

    amateescu’s picture

    1. +++ b/core/lib/Drupal/Core/Extension/ModuleHandlerInterface.php
      @@ -357,4 +357,15 @@ public function getModuleDirectories();
      +  public function invalidCore($core);
      

      I 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()?

    2. +++ b/core/modules/system/tests/modules/system_test/system_test.module
      --- /dev/null
      +++ b/core/modules/system/tests/modules/system_test/system_test.module.orig
      

      Doesn't look like a valid filename for a module file :)

    jhedstrom’s picture

    FileSize
    12.74 KB
    9.25 KB

    This removes the erroneously added file mentioned in #36, and I think addresses the concern in #33.

    jhedstrom’s picture

    Status: Needs work » Needs review
    amateescu’s picture

    Status: Needs review » Reviewed & tested by the community

    Yep, I think #37 is the best we can do for now, until we have some common base class for modules, themes and profiles.

    catch’s picture

    Sorry I'm not sure whether we actually need this or not:

    From #2135189: Proposal to manage the Drupal core release cycle

    No support overlap window for minor releases. Once a new minor is released, drop support for the old one. So long as only trivial BC breaks are allowed and we provide guidelines for people on how to mitigate them, there's no reason for anyone to stay on an old minor. Site owners unwilling to keep up with minor updates should wait for the LTS release. However, the initial (.0) patch release of a new minor version must not itself contain security fixes that haven't been backported and released to the prior minor; this might in some cases require releasing a final patch release for the prior minor on the same day as releasing the new minor (e.g., release an 8.0.6 on the same day as 8.1.0, but from that point on, no longer support 8.0).

    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.

    JamesOakley’s picture

    But 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.

    catch’s picture

    @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.

    JamesOakley’s picture

    @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.

    catch’s picture

    @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.

    tstoeckler’s picture

    I 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)

    catch’s picture

    Yes 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.

    alexpott’s picture

    Status: Reviewed & tested by the community » Needs work

    We don't seem to have complete test coverage here.

    core: 8.x or core: 8.0.x can be enabled with any Drupal 8.x release (e.g. 8.0.0 or later)

    core: 8.1.x can be enabled for any Drupal release from 8.1.0 or later

    core: 8.1.5 can be enabled for any Drupal release from 8.1.5 or later

    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 have drupal_check_incompatibility() - should these all be changing too?

    YesCT’s picture

    Issue summary: View changes
    Issue tags: +Needs tests, +Needs reroll

    updating 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.

    larowlan’s picture

    Status: Needs work » Needs review
    Issue tags: -Needs reroll +CriticalADay
    FileSize
    9.21 KB
    Gábor Hojtsy’s picture

    Issue tags: +Ghent DA sprint

    @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:

    core/modules/system/src/Form/ModulesListForm.php:        if ($incompatible_version = drupal_check_incompatibility($version, str_replace(\Drupal::CORE_COMPAT
    core/modules/system/system.install:        $compatibility = drupal_check_incompatibility($requirement, $version);
    

    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?

    xjm’s picture

    Priority: Critical » Major

    So 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.)

    pwolanin queued 49: semver-2313917.49.patch for re-testing.

    Status: Needs review » Needs work

    The last submitted patch, 49: semver-2313917.49.patch, failed testing.

    pwolanin’s picture

    Status: Needs work » Needs review
    FileSize
    7.96 KB

    re-roll for fuzz

    Status: Needs review » Needs work

    The last submitted patch, 54: semver-2313917-53.patch, failed testing.

    pwolanin’s picture

    Status: Needs work » Needs review
    FileSize
    9.21 KB

    oops, missed new info.yml files

    mgifford queued 56: semver-2313917-56.patch for re-testing.

    Status: Needs review » Needs work

    The last submitted patch, 56: semver-2313917-56.patch, failed testing.

    Mixologic’s picture

    I'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?

    David_Rothstein’s picture

    @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:

    core: 8.1.1
    dependencies:
      - token (>=8.x-1.5)
    

    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:

    core: 8.x
    dependencies:
      - system (>=8.1.1)
      - token (>=8.x-1.5)
    

    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):

    dependencies:
      - system (8.1.x)
    

    In which case this issue could maybe just be closed?

    Mile23’s picture

    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:

    You 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:

    core: 8.1.5 can be enabled for any Drupal release from 8.1.5 or later

    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)

    Status: Needs review » Needs work

    The last submitted patch, 61: 2313917_61.patch, failed testing.

    David_Rothstein’s picture

    You 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.

    But again, that should already be possible, via:

    dependencies:
      - system (<=8.1.1)
    

    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.

    mondrake’s picture

    David_Rothstein’s picture

    Thanks. Sigh :) It seems to me like fixing that is a better way to go than this issue.

    Version: 8.0.x-dev » 8.1.x-dev

    Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

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

    dawehner’s picture

    IMHO 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

    Version: 8.1.x-dev » 8.2.x-dev

    Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

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

    Version: 8.2.x-dev » 8.3.x-dev

    Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

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

    Version: 8.3.x-dev » 8.4.x-dev

    Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

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

    Version: 8.4.x-dev » 8.5.x-dev

    Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

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

    Version: 8.5.x-dev » 8.6.x-dev

    Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

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

    webchick’s picture

    Priority: Major » Critical

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

    dww’s picture

    Not 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.

    catch’s picture

    The 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:

    dependencies:
      drupal:system (~8.6 || ~9.0)
    

    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.

    tedbow’s picture

    Status: Needs work » Needs review
    FileSize
    8.74 KB

    Here is reroll for now with a couple of changes

    1. +++ b/core/modules/system/src/Tests/Module/DependencyTest.php
      --- /dev/null
      +++ b/core/modules/system/tests/modules/system_compatible_core_version_test_80x/system_compatible_core_version_test_80x.info.yml
      
      +++ b/core/modules/system/tests/modules/system_compatible_core_version_test_80x/system_compatible_core_version_test_80x.info.yml
      +++ b/core/modules/system/tests/modules/system_compatible_core_version_test_80x/system_compatible_core_version_test_80x.info.yml
      @@ -0,0 +1,6 @@
      
      @@ -0,0 +1,6 @@
      +name: 'System compatible core 8.0.x version test'
      +type: module
      +description: 'Support module for testing system dependencies.'
      +package: Testing
      +version: VERSION
      +core: 8.0.x
      

      I removed this test module in favor of using system_test_system_info_alter() to alter the core requirement of common_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().

    2. +++ b/core/modules/system/tests/modules/system_compatible_core_version_test_80x/system_compatible_core_version_test_80x.info.yml
      --- /dev/null
      +++ b/core/modules/system/tests/modules/system_incompatible_core_version_test_9x/system_incompatible_core_version_test_9x.info.yml
      
      +++ b/core/modules/system/tests/modules/system_incompatible_core_version_test_9x/system_incompatible_core_version_test_9x.info.yml
      +++ b/core/modules/system/tests/modules/system_incompatible_core_version_test_9x/system_incompatible_core_version_test_9x.info.yml
      @@ -0,0 +1,6 @@
      
      @@ -0,0 +1,6 @@
      +name: 'System incompatible core 9.x version test'
      +type: module
      +description: 'Support module for testing system dependencies.'
      +package: Testing
      +version: 9.0.0
      +core: 9.x
      

      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

    3. re #68 I think here @dawehner is referring to \Drupal\Component\Version\Constraint I don't see a VersionChecker 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

    Status: Needs review » Needs work

    The last submitted patch, 78: 2313917-78.patch, failed testing. View results
    - codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

    tedbow’s picture

    Status: Needs work » Needs review
    FileSize
    1.63 KB
    8.69 KB
    1. +++ b/core/modules/system/src/Controller/SystemController.php
      @@ -223,7 +223,7 @@ public function themesPage() {
      -        $theme->incompatible_core = !isset($theme->info['core']) || ($theme->info['core'] != \DRUPAL::CORE_COMPATIBILITY);
      +        $theme->incompatible_core = !isset($theme->info['core']) || !$this->moduleHandler()->isCoreCompatible($theme->info['core']) || !isset($theme->info['regions']['content']);
      

      We 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']);

    2. fixed coding standard from #79
    tedbow’s picture

    Ok this patch

    1. Removes the new method to ModuleHandlerInterface
    2. Uses \Drupal\Component\Version\Constraint instead check core compatibility
    3. The do-not-test patch would fail because right now Constraint doesn't handle semantic versioning correctly.
    4. Added a patch with the changes in #3004459: [PP-1] Fold in dependency parsing wisdom from project_composer that fixes Constraint
    tedbow’s picture

    +++ b/core/modules/system/src/Form/ModulesListForm.php
    @@ -342,7 +348,7 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
    -        elseif ($modules[$dependency]->info['core'] != \Drupal::CORE_COMPATIBILITY) {
    +        elseif (!(new Constraint($module->info['core'], \Drupal::CORE_COMPATIBILITY))->isCompatible(\Drupal::VERSION)) {
    

    Copy 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

    tedbow’s picture

    Ok I was looking at the proposed solution to this problem and I am not sure I agree with it.

    1. Right the only string we support for the 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.
    2. The proposal is support: 8.x, 8.0.x, 8.1.x and 8.1.5. These would all to try to make our core: key act like the dependencies: key using our version string constraint system
    3. Right now we don't actually support semantic version in modules #3009338: [META] Support semantic versioning for extensions (modules, themes, etc) in Drupal core, and allow modules to be compatible with Drupal 8 and 9 at the same time
    4. Right now there is critical #2641658: Module version dependency in .info.yml is ineffective for patch releases. Which is needed now because it would allow
      dependencies:
        - drupal:system (>= 8.0.1)

      but 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 in 8.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:

    1. We separate our core dependency from our home brewed version system
    2. #2807145: [policy, no patch] Allow contrib projects to specify multiple major core branches would because as simple as core: 8 - 9 which support all versions of Drupal 8 and 9. Or you could specify a specific version core: 8.6.5 - 9
    xjm’s picture

    @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.

    tedbow’s picture

    ok this patch does #83

    1. It adds \Drupal\Component\Version\CoreSemver with a single public static function :satisfies()
    2. \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().
    3. Adds test coverage for \Drupal\Component\Version\CoreSemver::satisfies() to prove it covers all the cases we need
    4. It also adds some test coverage to prove we don't actually need CoreSemver at all 😜. (explained next)
    5. It turns out as far as \Composer\Semver\Semver::satisfies() is concerned 8.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
    6. It seems like >= 8.0.0.0-dev < 9.0.0.0-dev is actually what we mean by the current core: 8.x in info.yml files.
    7. I will next add a patch without CoreSemver and instead using \Composer\Semver\Semver::satisfies() directly
    tedbow’s picture

    re #85

    1. +++ b/core/tests/Drupal/Tests/Component/Version/CoreSemverTest.php
      @@ -0,0 +1,70 @@
      +    $this->assertSame($result, CoreSemver::satisfies($version, $constraints));
      +    // Proving we actually don't need CoreSemver.
      +    $this->assertSame($result, Semver::satisfies($version, $constraints));
      

      Using both calls where to prove that for our test cases they are exactly the same.

    2. +++ b/core/tests/Drupal/Tests/Component/Version/CoreSemverTest.php
      @@ -0,0 +1,70 @@
      +    $versionParser = new VersionParser();
      +
      +    $this->assertEquals((string) $versionParser->parseConstraints('8.x'),(string) $versionParser->parseConstraints('^8'));
      +    $this->assertEquals((string) "[>= 8.0.0.0-dev < 9.0.0.0-dev]",(string) $versionParser->parseConstraints('8.x'));
      +    $this->assertNotEquals((string) $versionParser->parseConstraints('8.x'),(string) $versionParser->parseConstraints('>8'));
      

      \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

    3. The above proves we don't actually need \Drupal\Component\Version\CoreSemver to do conversion for 8.x.
    4. One thing we might actually need \Drupal\Component\Version\CoreSemver for is to wrap \Composer\Semver\Semver::satisfies() in a try/catch. Right now if you have core: 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

      UnexpectedValueException: Could not parse version constraint 8.nonsense: Invalid version string "8.nonsense" in Composer\Semver\VersionParser->parseConstraint()

    catch’s picture

    If 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).

    tedbow’s picture

    @catch

    Yes it should allow you to specify any range for Drupal that you wanted.

    +++ b/core/tests/Drupal/Tests/Component/Version/CoreSemverTest.php
    @@ -0,0 +1,70 @@
    +      ['8.0.0', '8 - 9', TRUE],
    +      ['9.1.1', '8 - 9', TRUE],
    +      ['9.1.1', '8.7.6 - 9', TRUE],
    +      ['8.7.8', '8.7.6 - 9', TRUE],
    +      ['8.6.8', '8.7.6 - 9', FALSE],
    

    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.

    Mixologic’s picture

    So 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.

    Mixologic’s picture

    Theres 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.

    Wim Leers’s picture

    #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:

    • All this makes wonder if we really want connect our core: key to our own version constraint system .
    • 👍 I think your proposal is excellent. We discussed something similar last week when we were talking about my comment at #2641658-28: Module version dependency in .info.yml is ineffective for patch releases. I would love to get us on a path to be more 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 and update.inc pointers are priceless!

    1. +++ b/core/modules/system/src/Form/ModulesListForm.php
      @@ -20,6 +21,7 @@
      +
      

      Nit: unnecessary whitespace change.

    2. +++ b/core/modules/system/tests/modules/system_incompatible_core_version_test_9x/system_incompatible_core_version_test_9x.info.yml
      @@ -0,0 +1,6 @@
      +version: 9.0.0
      

      This version is distracting from what matters here, let's use 1.0.0?

    3. +++ b/core/modules/system/tests/modules/system_incompatible_core_version_test_9x/system_incompatible_core_version_test_9x.info.yml
      @@ -0,0 +1,6 @@
      +core: 9.x
      

      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. 😆💧🏰

    4. +++ b/core/modules/system/tests/modules/system_test/system_test.module
      @@ -71,6 +75,8 @@ function system_test_system_info_alter(&$info, Extension $file, $type) {
      +    'system_compatible_core_version_test_80x',
      

      This does not exist.

    tedbow’s picture

    @Mixologic thanks for pointing out these cases where core: is used.

    1. I am addressing #90. I wasn't sure about the locale ones in #89. I assume we will still want to make sure we send 8.x in the url to drupal.org for translations.
    2. Addressed all points in #91. Thanks for the review @Wim Leers
    3. Updated \Drupal\Tests\system\Functional\Module\DependencyTest::testCoreVersionDependency() to use the ~8.6 format
    4. Uploading to versions of the patch on with all of the info files replaced with ~8 and 1 with just the system.info.yml to be easier to review

      Probably both will break a bunch of tests

    Mixologic’s picture

    So 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?

    Status: Needs review » Needs work

    The last submitted patch, 92: 2313917-92-all-info.patch, failed testing. View results

    Gábor Hojtsy’s picture

    @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).

    tedbow’s picture

    Interesting that both patches in #92 had the same number of fails. Many if not all are in UpdateTests and they fail with

    Exception: Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "access_check.db_update".

    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 with drush 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.

    Mixologic’s picture

    Had 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.

    catch’s picture

    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".

    fwiw 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.

    tedbow’s picture

    Status: Needs work » Needs review
    FileSize
    256.39 KB
    256.39 KB
    +++ b/core/includes/update.inc
    @@ -59,7 +60,7 @@ function update_check_incompatibility($name, $type = 'module') {
    -      || $file->info['core'] != \Drupal::CORE_COMPATIBILITY
    +      || Semver::satisfies(\Drupal::VERSION, $file->info['core'])
    

    Should have a ! here 😱

    tedbow’s picture

    FileSize
    535 bytes
    tedbow’s picture

    Belatedly replying to @Wim Leers comment #91. Thanks for the review

    1. re #78.3 #3066448: Harden test coverage for dependency Constraint class wasn't created for this issue but if we had gone with the approach in #82 and others where we used \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 using without this class so this issue doesn't need #3066448
    tedbow’s picture

    This patch adds

    1. Doesn't have throw a hard exception if the 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.
    2. Expands test coverage in \Drupal\Tests\system\Functional\Module\DependencyTest::testCoreVersionDependency() specifying to major core version with a || operator and some other cases.
    3. Also included a version of the patch with any of the actual changes to info.yml files. This will be easier to review but should also pass because the current core: 8.x will also be supported

    Status: Needs review » Needs work

    The last submitted patch, 102: 2313917-102-no-info-yml-changes.patch, failed testing. View results
    - codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

    Mixologic’s picture

    +++ b/core/modules/system/tests/modules/system_incompatible_core_version_dependencies_test/system_incompatible_core_version_dependencies_test.info.yml
    index f0076b1e43..e4f8e5737f 100644
    --- a/core/modules/system/tests/modules/system_incompatible_module_version_test/system_incompatible_module_version_test.info.yml
    
    --- a/core/modules/system/tests/modules/system_incompatible_module_version_test/system_incompatible_module_version_test.info.yml
    +++ b/core/modules/system/tests/modules/system_incompatible_core_version_test_9x/system_incompatible_core_version_test_9x.info.yml
    
    +++ b/core/modules/system/tests/modules/system_incompatible_core_version_test_9x/system_incompatible_core_version_test_9x.info.yml
    @@ -1,6 +1,6 @@
    -name: 'System incompatible module version test'
    +name: 'System incompatible core 9.x version test'
     type: module
     description: 'Support module for testing system dependencies.'
     package: Testing
    -version: '1.0'
    -core: 8.x
    +version: 1.0.0
    +core: 99.x
    
    

    Looks 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)

    +++ b/core/tests/Drupal/Tests/Component/Version/DrupalSemverTest.php
    @@ -0,0 +1,57 @@
    +      ['8.0.0', '8 - 9', TRUE],
    +      ['9.1.1', '8 - 9', TRUE],
    +      ['9.1.1', '8.7.6 - 9', TRUE],
    +      ['8.7.8', '8.7.6 - 9', TRUE],
    +      ['8.6.8', '8.7.6 - 9', FALSE],
    

    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)

    tedbow’s picture

    re #104. @Mixologic thanks for the review

    • I didn't rename system_incompatible_module_version_test but since system_incompatible_core_version_test_9x was so similar git picked it up as a copy. I changed the description which should have been done anyways and now git picks it up as new file

      The 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 😿

    • Changed the to the suggestions in DrupalSemverTest.php
    • +++ b/core/includes/update.inc
      @@ -59,7 +60,7 @@ function update_check_incompatibility($name, $type = 'module') {
      -      || $file->info['core'] != \Drupal::CORE_COMPATIBILITY
      +      || !Semver::satisfies(\Drupal::VERSION, $file->info['core'])
      

      Then should also use DrupalSemver

    • +++ b/core/modules/system/tests/src/Functional/Module/DependencyTest.php
      @@ -102,6 +102,52 @@ public function testIncompatiblePhpVersionDependency() {
      +    \Drupal::state()->set('dependency_test.core_version_requirement', "~$major || ~99");
      

      Change these to use ^ to match DrupalSemverTest

    • +++ b/core/modules/system/tests/src/Functional/Module/DependencyTest.php
      @@ -102,6 +102,52 @@ public function testIncompatiblePhpVersionDependency() {
      +    // Test less than a future major.
      +    \Drupal::state()->set('dependency_test.core_version_requirement', "<99");
      

      Removed this test because it seems a made up situation.

    • +++ b/core/tests/Drupal/Tests/Component/Version/DrupalSemverTest.php
      @@ -0,0 +1,57 @@
      +      ['a-super-nonsense-string-will-not-throw-an-exception-but-also-will-not-work', '9.x', FALSE],
      +      ['a-super-nonsense-string-will-not-throw-an-exception-but-also-will-not-work', '7.x', FALSE],
      +      ['a-super-nonsense-string-will-not-throw-an-exception-but-also-will-not-work', '8.x', FALSE],
      

      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()?

    tedbow’s picture

    Status: Needs review » Needs work

    The 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 use core: ~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()

          'core' => isset($data['info']['core']) ? $data['info']['core'] : \Drupal::CORE_COMPATIBILITY,
    

    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?

    Gábor Hojtsy’s picture

    Localize.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 :D

    tedbow’s picture

    Status: Needs work » Needs review

    tldr;

    I am including my original comment below in which I think I figured this out. but as far as I can tell

    1. The value of '%core' substitution does not come from any .info.yml file because
    2. \Drupal\Core\Utility\ProjectInfo::filterProjectInfo() filters the core key out when setting the project data.
    3. In locale_translation_build_projects() the only way $data['info']['core'] can be set is it was added in hook_locale_translation_projects_alter().
      which runs after \Drupal\Core\Utility\ProjectInfo::filterProjectInfo() which is triggered by
          $project_info->processInfoList($projects, $module_data, 'module', TRUE, $additional_whitelist);
          $project_info->processInfoList($projects, $theme_data, 'theme', TRUE, $additional_whitelist);
      
          // Allow other modules to alter projects before fetching and comparing.
          \Drupal::moduleHandler()->alter('locale_translation_projects', $projects);
      

      In locale_translation_project_list()

    4. So in locale_translation_build_projects()
      'core' => isset($data['info']['core']) ? $data['info']['core'] : \Drupal::CORE_COMPATIBILITY,
    5. This should be safe to leave because we will never be getting the new values from the core: key of the .info.yml files.(@see 1)
    6. We probably don't want to switch to using /all/ instead of the value set in locale_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 fire hook_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 debugged locale_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 the core key set in the whitelist so I am not I actually don't don't know how you would have core set unless you added it in hook_locale_translation_projects_alter. Very confusing

    So 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 about core: ^8 || ^9 or any other core value directly from a .info.yml file being used in \locale_translation_build_server_pattern() to replace %core.

    Mixologic’s picture

    Really 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.

    tedbow’s picture

    @Mixologic thanks for the additional information

    1. We can still eliminate that for core without any bc breaks by replacing the %core key in the default_server_pattern with all

      Yep I think we should file a follow up to do that though since it is not needed in this patch.

    2. I am still concerned that reason the 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 whitelist core so it does come from the info.yml files and no tests would fail.

      It would be great to get that in.

    3. Re the changes needed the in update.module I think this also points to use not having test coverage for 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 check core via update_verify_update_archive(). I will make an issue.

    4. I also realized we are changing all the info.yml files for profiles but I there are no changes to make this work.
      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.
    5. 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:

      Good catch!

      $theme->incompatible_core can be TRUE if the core key is missing or incompatible so I provided 2 different messages.

    tedbow’s picture

    Status: Needs review » Needs work

    The last submitted patch, 110: 2313917-110.patch, failed testing. View results
    - codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

    tedbow’s picture

    Status: Needs work » Needs review
    FileSize
    263.59 KB
    2.85 KB
    +++ b/core/modules/system/system.admin.inc
    @@ -294,7 +294,13 @@ function template_preprocess_system_themes_page(&$variables) {
    +        if (isset($theme->info['core'])) {
    +          $current_theme['incompatible'] = t("This theme is not compatible with Drupal @core_version. It is only compatible with Drupal versions that satisfy this constraint: @core_constraint", ['@core_version' => \Drupal::VERSION, '@core_constraint' => $theme->info['core']]);
    +        }
    +        else {
    +          $current_theme['incompatible'] = t("This theme must declare a 'core' value in its .info.yml file.");
    +        }
    

    I 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

    Mixologic’s picture

    Status: Needs review » Reviewed & tested by the community

    This 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..

    jibran’s picture

    Issue tags: +Needs change record

    Let's add a change record as well.

    tedbow’s picture

    Here is change notice: https://www.drupal.org/node/3070687

    It is tricky than I thought because

    It is recommended for modules that are compatible with 8.7 or earlier to still use
    core: 8.x
    until Drupal 8.9.0 is released at which time Drupal 8.7 will no longer be supported.

    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

    jibran’s picture

    Issue tags: -Needs change record

    Thanks, for creating the change record.

    +++ b/core/tests/Drupal/Tests/Component/Version/DrupalSemverTest.php
    @@ -0,0 +1,57 @@
    +  public function providerSatisfies() {
    

    Let's add some more test cases here e.g. ^8@alpha, ^8@beta, 8.x-dev, dev-8.8.x, and 8.x-dev#ceebf041

    jibran’s picture

    Composer also supports self.version which I assume in this case can be \Drupal::VERSION. Do we want to suppport that as well?

    Mixologic’s picture

    Re #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.

    Mixologic’s picture

    It is recommended for modules that are compatible with 8.7 or earlier to still use
    core: 8.x
    until Drupal 8.9.0 is released at which time Drupal 8.7 will no longer be supported.

    8.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.

    larowlan’s picture

    Should we keep behind one test .info file with 8.x to validate that it still works?

    Berdir’s picture

    Didn'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.

    xjm’s picture

    Status: Reviewed & tested by the community » Needs work

    @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.

    xjm’s picture

    Version: 8.8.x-dev » 8.7.x-dev
    xjm’s picture

    Issue tags: +Needs manual testing

    Oh 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:

    1. Installing or updating core
    2. Installing or updating a module
    3. Core is patched but a module uses the old format
    4. A module changes its key to the new format with an old version of core
    5. drush or using the UI/update status report and manual download
    tedbow’s picture

    ok 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 for

    1. Theme can be enabled if using ^8
    2. Module can be enabled using ^8
    3. Update hooks run if using ^8

    the interdiff with 113 does not include the info.yml changes to keep it small so it is just the test changes.

    The last submitted patch, 126: 2313917-126-8.7.patch, failed testing. View results
    - codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

    tedbow’s picture

    fixing test failure and documenting test helper assert method

    tedbow’s picture

    tedbow’s picture

    re #125 @xjm thanks for pointing out these test areas

    I tested:

    1. drush

      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.

    2. Installing drupal

      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.

    3. Core is patched but a module uses the old format

      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.

    4. A module changes its key to the new format with an old version of core

      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.

    5. Installing or updating a module

      works fine with the patch

    6. Installing or updating core
      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.

    7. Installing or updating a module

      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

    Status: Needs review » Needs work

    The last submitted patch, 130: 2313917-130.patch, failed testing. View results

    Mixologic’s picture

    ew, gross. I guess the simpleTestTest was built around expectations of the extension system.

    tedbow’s picture

    Status: Needs work » Needs review
    FileSize
    28.81 KB
    893 bytes

    So 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

    if ($enable_dependencies) {
    
          $module_list = $module_list ? array_combine($module_list, $module_list) : [];
          if ($missing_modules = array_diff_key($module_list, $module_data)) {
            // One or more of the given modules doesn't exist.
            throw new MissingDependencyException(sprintf('Unable to install modules %s due to missing modules %s.', implode(', ', $module_list), implode(', ', $missing_modules)));
          }
    

    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.

    tedbow’s picture

    and here is the 8.7 version

    tedbow’s picture

    1. I created #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 as a follow up
    2. +++ b/core/includes/update.inc
      @@ -59,7 +60,7 @@ function update_check_incompatibility($name, $type = 'module') {
      +      || !DrupalSemver::satisfies(\Drupal::VERSION, $file->info['core'])
      

      We have 7 instance of !DrupalSemver::satisfies(\Drupal::VERSION should we replace this with something like \Drupal::isCoreCompatible($version)

    3. it would be great to get a review of #3070354: Add a test for locale_translation_build_projects() to unensure that 'core' key does not affect translations URLs. It's pretty big hole in test coverage for confusing existing functionality.
    4. I manually tested in #130. removing tag though others could also test
    5. I updated the change record for the assumption that this gets committed to the 8.7 branch
      If you look at the quote in #116
      this can now be changed to

      It is recommended for modules that are compatible with 8.6 or earlier to still use
      core: 8.x
      until Drupal 8.8.0 is released at which time Drupal 8.6 will no longer be supported.

      This means module developers don't have to wait till Drupal 9 is actually released.

    Wim Leers’s picture

    Status: Needs review » Reviewed & tested by the community
    FileSize
    8.9 KB
    29.96 KB

    I read all comments after #99.

    • The line
      'core' => isset($data['info']['core']) ? $data['info']['core'] : \Drupal::CORE_COMPATIBILITY,
      

      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).

    • The change record at https://www.drupal.org/node/3070687 looked fine to me, I just refined it slightly.
    • This was already RTBC'd by @Mixologic in #114. It was un-RTBC'd in #123 because in order for this to be adoptable by contrib modules before 8.9/9.0, we need to backport minimal support for this to Drupal 8.7.
    • #130.4 stood out to me as the only two problematic things discovered in manual testing:
      But it will be able to be installed by drush […] don't check the value of the core key
      I thought it was weird that you say that it will be installed by Drush, even though in point 1 you say it won't be. So I dug in. I realized that the answer was in the interdiff attached to #130 itself: you specifically added validation of the core key to ModuleInstaller::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 the core 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's ModuleInstaller::install(). So we're good to go 👍
      If the module is already installed it will remain installed but it will not run updates for that module.
      This sounds scary! 😨 Imagine I have a contrib module foo with a security release 8.x-2.23 and in 8.x-2.21 it updated its core key to core: ^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 a core 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 has core: ^8.7.6, updates still will not be installed.

    • +1 for updating core's *.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

    1. +++ b/core/lib/Drupal/Component/Version/DrupalSemver.php
      @@ -0,0 +1,37 @@
      +   * This method uses \Composer\Semver\Semver::satisfies() but returns FALSE if
      +   * the version or constraints are not valid instead of throwing an exception.
      ...
      +  public static function satisfies($version, $constraints) {
      

      👍 This looks good. The reasoning for adding this class is solid. Variable naming is identical to the decorated code.

    2. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
      @@ -81,11 +82,18 @@ public function addUninstallValidator(ModuleUninstallValidatorInterface $uninsta
      +        throw new MissingDependencyException("Unable to install modules: module '$module' is incompatible with this version of Drupal core.");
      
      @@ -110,6 +118,9 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
      +              throw new MissingDependencyException("Unable to install modules: module '$module'. Its dependency module '$dependency' is incompatible with this version of Drupal core.");
      

      🤷‍♂️ I find this string strange, but it is identical to the pre-existing exception messages in \Drupal\Core\Extension\ModuleInstaller::install(). So … ✅

    3. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
      @@ -81,11 +82,18 @@ public function addUninstallValidator(ModuleUninstallValidatorInterface $uninsta
      +
      

      🤓 Nit: unnecessary blank line. Fixed. ✅

    4. +++ b/core/modules/system/src/Form/ModulesListForm.php
      @@ -294,10 +295,15 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
      +        '@module' => $module->getName(),
      

      👎 @module is not used. Removed. ✅

    5. +++ b/core/modules/system/system.admin.inc
      @@ -294,7 +294,7 @@ function template_preprocess_system_themes_page(&$variables) {
      -        $current_theme['incompatible'] = t("This theme is not compatible with Drupal @core_version. Check that the .info.yml file contains the correct 'core' value.", ['@core_version' => \Drupal::CORE_COMPATIBILITY]);
      +        $current_theme['incompatible'] = t("This theme is not compatible with Drupal @core_version. Check that the .info.yml file contains a compatible 'core' value.", ['@core_version' => \Drupal::VERSION, '@core_constraint' => $theme->info['core']]);
      

      👎 @core_constraint is not used. Removed. ✅

    6. +++ b/core/modules/system/tests/fixtures/update/drupal-8.update-test-semver-update-n-enabled.php
      @@ -0,0 +1,38 @@
      + * Partial database to mimic the installation of the update_test_schema module.
      

      🤓 Nit: This is not mocking the update_test_schema module being installed, but update_test_semver_update_n(). Fixed. ✅

    7. +++ b/core/modules/system/tests/modules/system_test/system_test.module
      @@ -62,6 +62,10 @@ function system_test_system_info_alter(&$info, Extension $file, $type) {
      +  if (($core_requirement = \Drupal::state()->get('dependency_test.core_version_requirement')) && $file->getName() === 'common_test') {
      +    $info['core'] = $core_requirement;
      +  }
      

      This could use a comment.

    8. +++ b/core/modules/system/tests/modules/update_test_semver_update_n/update_test_semver_update_n.install
      @@ -0,0 +1,13 @@
      +  \Drupal::state()->set('update_test_semver_update_n_update_8001', 'Yes, I was run. Thanks for testing!');
      

      Missed opportunity for a Ted joke 😁

    9. +++ b/core/modules/system/tests/src/Functional/Module/DependencyTest.php
      @@ -102,6 +102,50 @@ public function testIncompatiblePhpVersionDependency() {
      +    $this->assertFalse($assert_session->elementExists('css', '[name="modules[common_test][enable]"]')->hasAttribute('disabled'));
      

      🤔 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.

    10. +++ b/core/modules/system/tests/src/Functional/System/ThemeTest.php
      @@ -367,8 +367,11 @@ public function testInvalidTheme() {
      -    // Check for the error text of a theme with the wrong core version.
      -    $this->assertText("This theme is not compatible with Drupal 8.x. Check that the .info.yml file contains the correct 'core' value.");
      +    // Check for the error text of a theme with the wrong core version
      +    // using 7.x and ^7.
      +    $incompatible_core_message = 'This theme is not compatible with Drupal ' . \Drupal::VERSION . ". Check that the .info.yml file contains a compatible 'core' value.";
      +    $this->assertThemeIncompatibleText('Theme test with invalid core version', $incompatible_core_message);
      +    $this->assertThemeIncompatibleText('Theme test with invalid semver core version', $incompatible_core_message);
      

      👍 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.

    11. +++ b/core/modules/system/tests/src/Functional/System/ThemeTest.php
      @@ -437,24 +440,29 @@ public function testUninstallingThemes() {
      +      // Make sure Bartik is now set as the default theme in config.
      ...
      +      // Confirm Bartik is indicated as the default theme.
      

      🤓 Nit: comments are outdated. Fixed. ✅

    12. +++ b/core/modules/system/tests/src/Functional/System/ThemeTest.php
      @@ -437,24 +440,29 @@ public function testUninstallingThemes() {
       
      

      Übernit: the blank line can be removed now. Fixed. ✅

    13. +++ b/core/modules/system/tests/src/Functional/System/ThemeTest.php
      @@ -470,4 +478,16 @@ public function testThemeSettingsNoLogoNoFavicon() {
      +  private function assertThemeIncompatibleText($theme_name, $expected_text) {
      

      👍 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.

    14. +++ b/core/tests/Drupal/KernelTests/Core/Extension/ModuleInstallerTest.php
      @@ -85,4 +86,43 @@ public function testKernelRebuildDuringHookInstall() {
      +   * Tests install with a dependency with invalid core.
      

      🤓 Nit: s/an invalid core/an invalid core version constraint/ Fixed. ✅

    15. +++ b/core/tests/Drupal/Tests/Component/Version/DrupalSemverTest.php
      @@ -0,0 +1,57 @@
      +class DrupalSemverTest extends TestCase {
      

      👍 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 an hook_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 than core: 8.x until Drupal 8.8 is released (and Drupal 8.6 is deprecated).

    larowlan’s picture

    +++ b/core/modules/system/tests/src/Functional/System/ThemeTest.php
    @@ -470,4 +477,16 @@ public function testThemeSettingsNoLogoNoFavicon() {
    +    $this->assertSession()->elementExists('css', ".theme-info:contains(\"$theme_name\") .incompatible:contains(\"$expected_text\")");
    

    nit: 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)

    tedbow’s picture

    Status: Reviewed & tested by the community » Needs review

    @Wim Leers thanks for the review and fixes.

    I found another problem.

    1. When you run update.php update_fix_compatibility() is called from \Drupal\system\Controller\DbUpdateController::handle().
    2. This will disable any module or theme that returns true for update_check_incompatibility()
    3. So any site running 8.7.5 or earlier that updates a module that now using anything except core: 8.x will disable the module. The user will get no message that module has been disabled.
    4. So it is not just problem of the update_hook_N not running the module will actually be disabled regardless of whether it has any update hooks.

    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 least

    Wim Leers’s picture

    Reproduced #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:

    1. Modify the core.extension config manually
    2. Delete all configuration and reinstall. This does mean that whatever happens during hook_uninstall() never ran, and hence if hook_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().

    Mixologic’s picture

    Hmm. 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?

    Mixologic’s picture

    So 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.

    tedbow’s picture

    I 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.

    Mixologic’s picture

    Right, 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?

    Wim Leers’s picture

    #140: IMHO that would be a huge WTF :|

    #142: That's what I already said in #136 (The only solution is to communicate that this […]), 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 🤓

    Mixologic’s picture

    Hmm. 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 perfectly fix this

    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.

    name: Actions
    type: module
    description: 'Perform tasks on specific events triggered within the system.'
    package: Core
    version: VERSION
    core: 8.x
    expanded_core: ^8 | ^9
    configure: entity.action.collection
    

    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.

    tedbow’s picture

    I 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)

    1. We could write the current patch in a way with a change to \Drupal\Core\Extension\InfoParserDynamic::parse()(or somewhere else) to make sure that no module uses core: ^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.
    2. This would ensure that very few(none?) contrib modules would have core: ^8 or core: ^8.7.6.
    3. We could update docs and make release notes saying "Don't install any update to module that uses anything other than core: 8.x on <=8.7.7"
    4. There currently many sites using pre 8.7.5. 8.7.5 was security release but only affected sites that had Workspaces installed AFAICT. 8.7.6 was just released and was not a security release.
    5. So can assume that when release this many(10's of thousands?) won't upgrade, especially if it doesn't just happen to also be a security release
    6. We could ask any contrib authors that release a branch using anything other than core: 8.x
    7. to put something in there release notes for site owners for with core <=8.7.7

    8. But this likely won't reach many people. If you aren't updating core you probably aren't to read core release notes. If the next releases aren't security releases nobody has to upgrade. Site owners now aren't use to checking the 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 them

    All 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.

    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.

    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 need

    core: 8.x
    ore_requirement: ^8.7.7 || ^9
    

    Having 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...

    1. we introduced a new key but we made it an either/or for with existing core: key
    2. In this issue we updated \Drupal\Core\Extension\InfoParserDynamic::parse() to throw an exception if core: and core_requirement were both present
    3. In \Drupal\Core\Extension\InfoParserDynamic::parse() we also check to make sure that we don't have core_requirement: ^8 || ^9 because we know it won't work for earlier version because it will be missing core:
    4. On any site before this commit any module with core_requirement will fail hard because their version of \Drupal\Core\Extension\InfoParserDynamic::parse() will require the core: key. Because it would fail on the parser they will not get into the problem documented in #138 by update_fix_compatibility()
    5. This would mean that you really shouldn't use 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 versions
    6. Because the parser will hard fail if you try to do core_requirement: ^8 || ^9 or core_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 committed

    Mixologic’s picture

    Okay, so if

    core:

    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'

    tedbow’s picture

    About half of the modules in the ecosystem work with *every version of d8 out there*.

    Is that true because

    1. they haven't started to remove deprecations? If so then they would not work with Drupal 9 correct?
    2. Or is it that half modules don't actually use the things we are deprecating? So a module that worked with 8.4.0 might still just work in Drupal 9 because it just happened to not use the things we are deprecating.

    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?

    Mixologic’s picture

    Issue tags: +mwds2019

    Its 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.

    tedbow’s picture

    I am still on the fence

    1. I think there are complications if support both in the same info.yml file. For instance which one should take precedence? For instance what if there is
      core: 8.x
      core_requirements: ^9
      

      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 ignore core_requirements:

      Could we somehow check to make sure if core: was used that core_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.

    2. We would actually deprecate the 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 supported

    tedbow’s picture

    Ok here is a try

    1. Adds an optional 'core_dependency' key
    2. Adds validation in \Drupal\Core\Extension\InfoParserDynamic::parse() to make sure
      • core or core_dependency is provided
      • Makes sure the core_dependency doesn't try to specify a constraint for version <=8.7.x(because it would be ignored by those versions). Not perfect
      • Makes sure core: only uses [MAJOR].X. I limited to a single digit think we would deprecate core: before Drupal 10🙏
    3. If 'core' is set and 'core_dependency' is not then it transfers the value to 'core_dependency'. Therefore everything after uses the parsing uses 'core_dependency'
    tedbow’s picture

    Version: 8.7.x-dev » 8.8.x-dev
    FileSize
    36.45 KB
    4.31 KB

    Fixed \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.

    Status: Needs review » Needs work

    The last submitted patch, 152: 2313917-152.patch, failed testing. View results

    Wim Leers’s picture

    Status: Needs work » Needs review
    FileSize
    2.09 KB
    37.37 KB
    1. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
      @@ -28,6 +29,20 @@ public function parse($filename) {
      +        if (DrupalSemver::satisfies('8.7.6', $parsed_info['core_dependency']) && !DrupalSemver::satisfies('8.0.0', $parsed_info['core_dependency'])) {
      +          throw new InfoParserException("The 'core_dependency' can not be used to specify compatibility specific version before 8.7.7 in " . $filename);
      

      The comment says 8.7.7, the condition says 8.7.6. Updated the condition.

    2. Updated the expected error message in the failing test. Patch should be green now :)
    Wim Leers’s picture

    #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):

    • To make a module work on Drupal 8, you can do either "just core" or "both"
    • To make a module work on Drupal 8 + 9 that uses no D8-deprecated APIs, you can also do either "just core" or "both"
    • To make a module work on Drupal 8 + 9 that uses APIs introduced in a minor/patch release of Drupal 8.7.7 or later, you must use "just core_requirement"
    • To make a module work on Drupal 8 + 9 that uses APIs introduced in a minor/patch release before Drupal 8.7.7: this is not supported, because the infrastructure didn't exist until Drupal 8.7.7, that's why this issue has been open for years
    • To make a module work on Drupal 9: you could do either "just core" (if it is also compatible with all of Drupal 8), or "both", or "just core_requirement". But it would be recommended to do "just core_requirement".

    Summarized, it's either

    1. "just core" or "both" (because "just core" always works and "both" allows you to be more specific once you start to use 8.7.7-only or later APIs)
    2. "just core_requirement" (only possible if this module only ever supported Drupal 8.7.7 or later)

    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 deprecate core 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 contains core: 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?

    Wim Leers’s picture

    This 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 propose drupal. An example:

    drupal: ^8.7.7 || ^9
    

    versus

    core_requirement: ^8.7.7 || ^9
    
    Mixologic’s picture

    On 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 deprecate core: in d8 for removal in d9 and only have the core_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...

    Wim Leers’s picture

    This is already the case for every module out there that leverages an API feature that wasn't in a previous version,

    True. 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.

    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.

    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:

    A drupal 8 extension, that is not using any deprecated code, should be able to be used with both drupal 8 and 9. (or 10/11 etc)

    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 👍

    markhalliwell’s picture

    "just core"

    +++ b/core/lib/Drupal/Component/Version/DrupalSemver.php
    @@ -0,0 +1,37 @@
    +  public static function satisfies($version, $constraints) {
    +    try {
    +      return Semver::satisfies($version, $constraints);
    +    }
    +    catch (\UnexpectedValueException $exception) {
    +      return FALSE;
    +    }
    +  }
    

    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.

    if ($constraints === \ Drupal::CORE_COMPATIBILITY) {
      @trigger_error(sprintf(
        'Use of %s has been deprecated in 8.8.0. Use a semantic version constraint instead. See %s.',
        \ Drupal::CORE_COMPATIBILITY    
        'https://getcomposer.org/doc/articles/versions.md'
      ), E_USER_DEPRECATED);
      return TRUE;
    }
    

    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

    tedbow’s picture

    re #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.

    This means that the responsibility of what is or isn't "installable" falls back to the project itself... not core.

    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 uses core: 8.x and then updates to my_module 8.x-1.1 that uses core: ^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 state

    If we use core: ^8 we can't prevent this for any site that doesn't have the core update.

    re #155

    But somebody on such an ancient version of Drupal 8 we cannot really support anyway.

    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

    core: 8.x
    core_dependency: ^8.8
    

    '
    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:

    Mixologic’s picture

    A drupal 8 extension, that is not using any deprecated code, should be able to be used with both drupal 8 and 9. (or 10/11 etc)

    Or perhaps I misinterpreted that too?

    Yes, 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)

    I really don't understand why a new "core_requirements" or "core_dependency" field is needed.

    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

    markhalliwell’s picture

    the problem is that Drupal 8.7.5 has my_module 8.x-1.1 that uses core: 8.x and then updates to my_module 8.x-1.1 that uses core: ^8(anything beside 8.x) will have this this module disabled

    IMO, 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.

    Mixologic’s picture

    Yes, 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

    tedbow’s picture

    Issue summary: View changes
    markhalliwell’s picture

    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?

    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 added core_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.

    Berdir’s picture

    > 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,

    Mixologic’s picture

    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?

    No, 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

    markhalliwell’s picture

    Contrib isn't semver (yet):

    https://www.drupal.org/docs/8/understanding-drupal-8/understanding-drupa...

    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.

    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.

    We do not want to require every d8 module to release a new major version to be d9 compatible.

    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.

    markhalliwell’s picture

    I 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?

    Mixologic’s picture

    That does seem, however, extremely unlikely.

    I have data that we've already ran on over 7000k modules that show that its true, for about 3500 modules.

    This is a chance to start us on the right path, can we actually start it... doing it correctly?

    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.

    Berdir’s picture

    > 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.

    markhalliwell’s picture

    Right, ok.

    dww’s picture

    Probably 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 at dependencies in the .info.yml file. Contribs that want to support 8.8.x+ and 9.0.x should say something like:

    core: 8.x
    dependencies:
      drupal:system (~8.8 || ~9.0)
    

    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 kill core and rely on dependencies.

    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?

    tedbow’s picture

    this a performance improvement that i meant to include in #160

    so not commenting on #160 and beyond right now

    1. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
      @@ -11,6 +11,8 @@
      +  private $first_core_dependency_supported_version = '8.7.7';
      

      Making this is a const

    2. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
      @@ -78,4 +92,32 @@ protected function getRequiredKeys() {
      +  private function constraintsSupportsPreCoreDependency($constraint) {
      

      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

    xjm’s picture

    Thanks 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:

    1. 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).

    2. Our long-term goal is to replace the dependency management functionality of .info.yml files with composer.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).

    3. 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.

    4. 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:

    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.

    tedbow’s picture

    1. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
      @@ -28,6 +31,32 @@ public function parse($filename) {
      +      if (isset($parsed_info['core']) && !preg_match("/^\d\.x$/", $parsed_info['core'])) {
      +        throw new InfoParserException("The {$parsed_info['core']} is not valid value for  'core' in " . $filename);
      +      }
      

      I 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 in core: while they are making this change it doesn't mess up their site with #138

      So I think this new check is good. Adding new test coverage for it.

    2. Some other small clean up in InfoParserUnitTest
    Mixologic’s picture

    +++ b/core/includes/update.inc
    @@ -58,8 +59,8 @@ function update_check_incompatibility($name, $type = 'module') {
    +      || !DrupalSemver::satisfies(\Drupal::VERSION, $file->info['core_dependency'])
    
    +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
    @@ -28,6 +31,32 @@ public function parse($filename) {
    +        if ($supports_pre_core_dependency_version && !DrupalSemver::satisfies('8.0.0-alpha1', $parsed_info['core_dependency'])) {
    
    +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
    @@ -81,11 +82,17 @@ public function addUninstallValidator(ModuleUninstallValidatorInterface $uninsta
    +      if (isset($module_data[$module]) && !DrupalSemver::satisfies(\Drupal::VERSION, $module_data[$module]->info['core_dependency'])) {
    
    @@ -110,6 +117,9 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
    +            if (!DrupalSemver::satisfies(\Drupal::VERSION, $module_data[$dependency]->info['core_dependency'])) {
    
    +++ b/core/modules/system/src/Controller/SystemController.php
    @@ -223,7 +224,7 @@ public function themesPage() {
    +        $theme->incompatible_core = !isset($theme->info['core_dependency']) || !DrupalSemver::satisfies(\Drupal::VERSION, $theme->info['core_dependency']);
    
    +++ b/core/modules/system/src/Form/ModulesListForm.php
    @@ -294,10 +295,14 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
    +    if (!DrupalSemver::satisfies(\Drupal::VERSION, $module->info['core_dependency'])) {
    
    @@ -341,7 +346,7 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
    +        elseif (!DrupalSemver::satisfies(\Drupal::VERSION, $modules[$dependency]->info['core_dependency'])) {
    

    Lets 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.

    +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
    @@ -10,6 +11,8 @@
    +  const FIRST_CORE_DEPENDENCY_SUPPORTED_VERSION = '8.7.7';
    

    This should probably be 8.8.0, unless we plan on backporting this to 8.7.x ?

    catch’s picture

    Just 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.

    tedbow’s picture

    1. @drumm just opened up #3075062: Remove core key overriding in .info.yml packaging

      Right 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 a core: key but this issue makes the core: optional or even

      +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
      @@ -28,6 +31,32 @@ public function parse($filename) {
      +        if (!$supports_pre_core_dependency_version && isset($parsed_info['core'])) {
      +          throw new InfoParserException("The 'core_dependency' constraint ({$parsed_info['core_dependency']}) requires the 'core' not be set in " . $filename);
      +        }
      

      Not allowed in this case. If d.o packaging added the core back in then you could enable a module that had core: ^8.8 because Drupal 8.7.x would ignore core_dependency

    2. re #177.1

      Lets refactor all instances of the semver check into the parser,

      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 use core: 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:.

    3. #177

      This should probably be 8.8.0, unless we plan on backporting this to 8.7.x ?

      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

      core_dependency: ^8.7.7
      

      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 to core_dependency

      But of course it depends on whether we think this is too disruptive to backport

    Mixologic’s picture

    Re: 177.1 I dont think this should change any functionality. Easier to express what I mean with an interdiff/patch

    tedbow’s picture

    @Mixologic ok that makes sense I thought you wanted to throw an exception in the parser.

    1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
      @@ -88,7 +87,7 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
      -      if (isset($module_data[$module]) && !DrupalSemver::satisfies(\Drupal::VERSION, $module_data[$module]->info['core_dependency'])) {
      +      if (isset($module_data[$module]) && $module_data[$module]->info['core_incompatible']) {
      

      We can just check
      !empty($module_data[$module]->info['core_incompatible'])

    2. +++ b/core/modules/system/src/Controller/SystemController.php
      @@ -224,7 +223,7 @@ public function themesPage() {
      -        $theme->incompatible_core = !isset($theme->info['core_dependency']) || !DrupalSemver::satisfies(\Drupal::VERSION, $theme->info['core_dependency']);
      +        $theme->incompatible_core = !isset($theme->info['core_dependency']) || $theme->info['core_incompatible'];
      

      We don't actually need to check core_dependency at all.

    3. +++ b/core/modules/update/update.module
      @@ -700,7 +699,7 @@ function update_verify_update_archive($project, $archive_file, $directory) {
      -    if (empty($info['core_dependency']) || !DrupalSemver::satisfies(\Drupal::VERSION, $info['core_dependency'])) {
      +    if (empty($info['core_dependency']) || $info['core_incompatible']) {
      

      Don't need to test core_dependency at all.

    4. 2313917-180.patch failed because the new DrupalSemver was not included.
    tedbow’s picture

    Status: Needs review » Needs work

    The last submitted patch, 181: 2313917-181.patch, failed testing. View results

    tedbow’s picture

    Status: Needs work » Needs review
    FileSize
    1.57 KB
    41.44 KB
    +++ b/core/modules/system/tests/modules/system_test/system_test.module
    @@ -62,6 +62,10 @@ function system_test_system_info_alter(&$info, Extension $file, $type) {
    +  if (($core_requirement = \Drupal::state()->get('dependency_test.core_version_requirement')) && $file->getName() === 'common_test') {
    +    $info['core_dependency'] = $core_requirement;
    +  }
    

    Ok \Drupal\Tests\system\Functional\Module\DependencyTest::testCoreVersionDependency() failed because we were using the system_test_system_info_alter() to change core_dependency. Of course now core_dependency already evaluated to set core_incompatible so that won't affect whether a module can be enabled.

    I think using hook_system_info_alter() to alter the core or core_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 since core_incompatible would never be in the info file itself. Doing this.

    Status: Needs review » Needs work

    The last submitted patch, 184: 2313917-184.patch, failed testing. View results

    tedbow’s picture

    Status: Needs work » Needs review
    FileSize
    9.08 KB
    42.93 KB
    1. +++ b/core/modules/update/update.module
      @@ -699,7 +699,7 @@ function update_verify_update_archive($project, $archive_file, $directory) {
           $info = \Drupal::service('info_parser')->parse($file->uri);
       
           // If the module or theme is incompatible with Drupal core, set an error.
      -    if (empty($info['core']) || $info['core'] != \Drupal::CORE_COMPATIBILITY) {
      +    if ($info['core_incompatible']) {
      

      The update module tests failed because they only call \Drupal\Core\Extension\InfoParserDynamic::parse() and not \Drupal\Core\Extension\ExtensionList::doList() so core_incompatible was not set.

    2. To around the problem with testing \Drupal\Tests\system\Functional\Module\DependencyTest::testCoreVersionDependency() mentioned in #184 we can add more comprehensive test coverage to InfoParserUnitTest because that is where core_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 sure core_incompatible is set correctly in these cases.

      I also renamed \Drupal\Tests\system\Functional\Module\DependencyTest::testCoreVersionDependency() to testCoreCompatibility() and different modules instead of system_test_system_info_alter() to alter the info.

    Mixologic’s picture

    +++ b/core/includes/update.inc
    @@ -58,8 +58,8 @@ function update_check_incompatibility($name, $type = 'module') {
    -      || !isset($file->info['core'])
    -      || $file->info['core'] != \Drupal::CORE_COMPATIBILITY
    +      || !isset($file->info['core_dependency'])
    +      || $file->info['core_incompatible']
    

    This 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, or core_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.

    claudiu.cristea’s picture

    From IS:

    Introduce a new key core_dependency:(better name?) [...]

    Why not supporting a special value in dependencies?

    ...
    core: 8.x
    dependencies:
      - drupal:core: ^8.7.4 || ^9
      - ...
    
    Mixologic’s picture

    Re #188 Please refer to #175 as we've already exhaustively covered that.

    tedbow’s picture

    +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
    @@ -28,6 +31,37 @@ public function parse($filename) {
    +      $parsed_info['core_incompatible'] = !DrupalSemver::satisfies(\Drupal::VERSION, $parsed_info['core_dependency']);
    

    This 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

    eelkeblok’s picture

    Re #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.

    tedbow’s picture

    re #187

    1. Removed the check for 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 because 8.x is valid for \Composer\Semver\Semver::satisfies()

      But now we are just setting core_incompatible we don't need core_dependency after the parser.

    2. Fixed the two minor phpcs errors

    re #190

    1. I removed DrupalSemver. I realized DrupalSemver was actually being used multiple places in that file so I just made it a private static function to avoid the try/catch in multiple places.

    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.

    1. We would have to document that not only is this different than other entries under 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.
    2. We would have to document that some values such as anything with a ^ or || would be valid in
      dependencies:
        - drupal:core: 

      but in no other key under dependencies

    3. It would be confusing because there is a current method many modules use to specify a core version constraint -drupal:system (8.6.x) that is very similar but again actually would use the existing constraint format.
    4. This would be slightly different than our current values under constraints which actually don't use the yaml key/value but are all 1 value that we parse
      So we would have
      dependencies:
        -drupal:core: ^8.7.4 || ^9
        -drupal:webform (5.x)
      

      We could change this to be

      dependencies:
        -drupal:core (^8.7.4 || ^9)
        -drupal:webform (5.x)
      

      But I think the other problems are confusing enough not to use this.

    Other problems fixed in self-review

    1. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
      @@ -60,7 +94,47 @@ public function parse($filename) {
      +              if (DrupalSemver::satisfies($minor_version, $pre_release_version)) {
      

      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.

    2. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
      @@ -60,7 +94,47 @@ public function parse($filename) {
      +            foreach (['alpha1', 'beta1', 'rc1'] as $suffix) {
      

      Adding check for alpha2, beta2 to rc20.

    3. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
      @@ -156,7 +303,73 @@ public function testInfoParserCommonInfo() {
      +  public function testCoreIncompatibility($file_name, $constraint, $expected) {
      

      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.

    Wim Leers’s picture

    Status: Needs review » Needs work

    My 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

    1. +++ b/core/includes/update.inc
      @@ -58,8 +58,7 @@ function update_check_incompatibility($name, $type = 'module') {
      -      || !isset($file->info['core'])
      -      || $file->info['core'] != \Drupal::CORE_COMPATIBILITY
      +      || $file->info['core_incompatible']
      

      👍 This decouples update.inc from the current core *.info.yml key.

      But…
      🤔 See the piggybacking remark above.

    2. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
      @@ -10,6 +11,31 @@
      +  const FIRST_CORE_DEPENDENCY_SUPPORTED_VERSION = '8.7.7';
      

      Nit: missing comment.

    3. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
      @@ -10,6 +11,31 @@
      +  protected static function satisfies(string $version, $constraints) {
      

      👍 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!

    4. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
      @@ -60,7 +118,50 @@ public function parse($filename) {
      +  protected function isConstraintSatisfiedByPreCoreDependencyCoreVersion($constraint) {
      

      Nit: this could be static, which would guarantee this is a side effect free function.

    5. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
      @@ -60,7 +118,50 @@ public function parse($filename) {
      +              }
      +
      +            }
      

      Nit: extraneous blank line.

    6. +++ b/core/modules/system/src/Form/ModulesListForm.php
      @@ -294,10 +294,14 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
      +      $row['#requires']['core'] = $this->t('Drupal Core (@core_requirement) (<span class="admin-missing">incompatible with</span> version @core_version)', [
      

      Nit: </span> is in a weird place. Shouldn't that have been just before the closing parenthesis )?

    7. +++ b/core/modules/system/tests/modules/update_test_semver_update_n/update_test_semver_update_n.install
      @@ -0,0 +1,13 @@
      + * Update 8002.
      ...
      +function update_test_semver_update_n_update_8001() {
      

      Nit: mismatch.

    8. +++ b/core/modules/system/tests/src/Functional/System/ThemeTest.php
      @@ -470,4 +477,16 @@ public function testThemeSettingsNoLogoNoFavicon() {
      +   * @param $expected_text
      

      Nit: string typehint

    9. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
      @@ -98,10 +98,171 @@ public function testInfoParserMissingKeys() {
      +   * Tests that missing 'core' and 'core_dependency' keys are detected.
      ...
      +   * Tests that 'core_dependency: ^8.8' is valid with no 'core' key.
      ...
      +   * Tests that 'core_dependency: ^8.8' is invalid with a 'core' key.
      ...
      +   * Tests that 'core_dependency: ^8.8' is invalid with a 'core' key.
      ...
      +   * Tests that 'core_dependency' throws an exception if constraint is invalid.
      

      👍

    tedbow’s picture

    Issue summary: View changes
    Status: Needs work » Needs review
    FileSize
    40.21 KB
    3.3 KB

    @Wim Leers thanks for the review!

    1. Yes. I thought about that piggybacking but I think 7 places we would have to check 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 in update_verify_update_archive() to determine if it is compatible, you won't need to set core_incompatible here.

    2. Added a comment
    3. 🎉
    4. changed to static
    5. fixed
    6. Check 8.8.x and this pattern of just surrounding incompatible with with the span is done in the existing core compatibility message and for a module not being compatible with another module.
    7. fixed
    8. fixed
    9. 👍

    a couple other things

    1. removing from the remaining tasks

      Determine if core: 9.x should be allowed or if just core_dependency: ^9 should be used for only Drupal 9 modules.

      This can be determined in Drupal 9.

    2. Add #3075062: Remove core key overriding in .info.yml packaging. to remaining tasks
    3. Added to remaining tasks

      Determine whether there is better name than core_dependency

      I think core_version_requiement is more explicit.

    4. Determine if this should be backported to 8.7. \Drupal\Core\Extension\InfoParserDynamic::FIRST_CORE_DEPENDENCY_SUPPORTED_VERSION will have to be updated accordingly
    Mixologic’s picture

    re #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

    Wim Leers’s picture

    Status: Needs review » Reviewed & tested by the community
    Issue tags: +Needs framework manager review

    Agreed 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.

    1. 👍
    2. #3075062: Remove core key overriding in .info.yml packaging has already been fixed & deployed to d.o!
    3. 🤓 We're down to the hardcore bikeshedding! 🥳 I … don't really care. 🤷‍♂️ It's temporary anyway. I kinda like what I proposed in #156 still: drupal: … instead of core_dependency: … or core_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.
    4. That's definitely a core committer (release manager) decision.

    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.

    tedbow’s picture

    Status: Reviewed & tested by the community » Needs review
    FileSize
    22.76 KB
    40.74 KB

    @Wim Leers thanks for RTBC'ing this.

    I going to do hopefully 1 more patch.

    1. I really think core_version_requirement is better changing. Most of the interdiff is this
    2. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
      @@ -60,7 +121,49 @@ public function parse($filename) {
      +  static protected function isConstraintSatisfiedByPreCoreDependencyCoreVersion($constraint) {
      

      static and protected are out of order

    3. +++ b/core/modules/system/tests/modules/system_test/system_test.module
      @@ -62,6 +62,10 @@ function system_test_system_info_alter(&$info, Extension $file, $type) {
      +  if (($core_requirement = \Drupal::state()->get('dependency_test.core_version_requirement')) && $file->getName() === 'common_test') {
      +    $info['core_dependency'] = $core_requirement;
      +  }
      

      This was for \Drupal\Tests\system\Functional\Module\DependencyTest::testCoreCompatibility() that was changed in #184. Since that test no longer relies on system_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 needed

    4. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
      @@ -98,10 +98,171 @@ public function testInfoParserMissingKeys() {
      +    $this->assertSame($info_values['core_dependency'], '^8.8');
      

      This should also assert that 'core' is not set

    5. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
      @@ -98,10 +98,171 @@ public function testInfoParserMissingKeys() {
      +   * Tests that 'core_dependency: ^8.8' is invalid with a 'core' key.
      

      Wrong comment. This is testing for invalid core value.

    6. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
      @@ -98,10 +98,171 @@ public function testInfoParserMissingKeys() {
      @@ -159,4 +320,69 @@ public function testInfoParserCommonInfo() {
      

      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 setting core_version_requirement in the parser now we don't need to do this and should test for it.

    Wim Leers’s picture

    Status: Needs review » Needs work
    1. So #197 addresses point 3 in #194/#195/#196, where @tedbow and @Mixologic are both leaning towards core_version_requirement: … instead of core_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:

      +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
      @@ -12,7 +12,7 @@
         const FIRST_CORE_DEPENDENCY_SUPPORTED_VERSION = '8.7.7';
      
      @@ -57,34 +57,35 @@ public function parse($filename) {
      +        $supports_pre_core_dependency_version = static::isConstraintSatisfiedByPreCoreDependencyCoreVersion($parsed_info['core_version_requirement']);
      
      @@ -125,16 +126,16 @@ protected function getRequiredKeys() {
      +  protected static function isConstraintSatisfiedByPreCoreDependencyCoreVersion($constraint) {
      

      These all contain 'core_dependency' or 'CoreDependency'. I think we should update those too.

    2. #197.3: nice!
    3. #197.4 + .5 + .6: strengthening of the tests, great.

    Marking Needs work for point 1. Then back to RTBC per #196.

    tedbow’s picture

    Status: Needs work » Needs review
    FileSize
    3.78 KB
    40.81 KB

    @Wim Leers thanks. Fixed

    Wim Leers’s picture

    Status: Needs review » Reviewed & tested by the community
    tedbow’s picture

    Status: Reviewed & tested by the community » Needs review
    FileSize
    43.33 KB
    2.55 KB
    41.52 KB

    @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

    name: tester
    type: module
    core: 8.x
    core_version_requirement: ^8
    

    This should work but I was getting the exception

    Drupal\Core\Extension\InfoParserException: The 'core_version_requirement' constraint (^8) requires the 'core' not be set in modules/custom/tester/tester.info.yml

    I figured out that this only happened if the 2nd module that used the exception not the first the problem is coming from:

    +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
    @@ -60,7 +122,49 @@ public function parse($filename) {
    +    }
    +    $evaluated_constraints[$constraint] = FALSE;
    +    return $evaluated_constraints[$constraint];
    

    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.

    +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
    @@ -98,10 +98,171 @@ public function testInfoParserMissingKeys() {
    +  /**
    +   * Tests a invalid 'core_version_requirement'.
    +   *
    +   * @covers ::parse
    +   *
    +   * @dataProvider providerCoreVersionRequirementInvalid
    +   */
    +  public function testCoreVersionRequirementInvalid($file_name, $constraint) {
    ...
    +    $filename = vfsStream::url("modules/fixtures/$file_name.info.txt");
    +    $this->expectException('\Drupal\Core\Extension\InfoParserException');
    +    $this->expectExceptionMessage("The 'core_version_requirement' can not be used to specify compatibility specific version before 8.7.7 in vfs://modules/fixtures/$file_name.info.txt");
    +    $this->infoParser->parse($filename);
    

    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😜)

    The last submitted patch, 201: 2313917-201-test-fix-only.patch, failed testing. View results

    tedbow’s picture

    Explanation of test changes

    1. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
      @@ -243,12 +244,25 @@ public function testCoreVersionRequirementInvalid($file_name, $constraint) {
      +        "$file_name-duplicate.info.txt" => $invalid_core_version_requirement,
      

      We have to create a duplicate file because \Drupal\Core\Extension\InfoParser::parse() will cache by file names.

    2. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
      @@ -243,12 +244,25 @@ public function testCoreVersionRequirementInvalid($file_name, $constraint) {
      -    $this->expectException('\Drupal\Core\Extension\InfoParserException');
      -    $this->expectExceptionMessage("The 'core_version_requirement' can not be used to specify compatibility specific version before 8.7.7 in vfs://modules/fixtures/$file_name.info.txt");
      

      expectException() won't work for us here because we want to make sure it gets thrown on subsequent calls too.

    3. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
      @@ -243,12 +244,25 @@ public function testCoreVersionRequirementInvalid($file_name, $constraint) {
      +    catch (InfoParserException $exception) {
      +      $this->assertSame($exception_message . "$file_name.info.txt", $exception->getMessage());
      +      try {
      +        $this->infoParser->parse(vfsStream::url("modules/fixtures/$file_name-duplicate.info.txt"));
      

      We need to nest the second call inside the first one's catch to make sure the exception is thrown each time.

    4. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
      @@ -243,12 +244,25 @@ public function testCoreVersionRequirementInvalid($file_name, $constraint) {
      +      catch (InfoParserException $exception) {
      +        $this->assertSame($exception_message . "$file_name-duplicate.info.txt", $exception->getMessage());
      +        return;
      +      }
      +    }
      +
      +    $this->fail('The exception was not thrown when parsing the info file the second 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.

    tedbow’s picture

    1. re #203.2
      Actually expectException() and expectExceptionMessage() only don't work for the 1st call to parse().

      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 to parse() did not cause an exception. This also allows the tests to only 1 try/catch and not a nested try/catch

      expectException() and expectExceptionMessage() have to be called before the try/catch to make sure they aren't called because the first parse() doesn't throw an exception.

    2. The specific bug #201 happened because $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 call parse() 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.
    3. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
      @@ -98,10 +99,184 @@ public function testInfoParserMissingKeys() {
      +    $filename = vfsStream::url('modules/fixtures/invalid_core.info.txt');
      +    $this->expectException('\Drupal\Core\Extension\InfoParserException');
      +    $this->expectExceptionMessage("Invalid 'core' value \"^8\" in vfs://modules/fixtures/invalid_core.info.txt");
      +    $this->infoParser->parse($filename);
      

      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 call parse() direclty

    4. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
      @@ -98,10 +99,184 @@ public function testInfoParserMissingKeys() {
      +  public function testCoreVersionRequirementInvalid($file_name, $constraint) {
      

      The 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.

    tedbow’s picture

    I 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

    Mixologic’s picture

    Status: Needs review » Reviewed & tested by the community

    All 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

    catch’s picture

    Status: Reviewed & tested by the community » Needs review
    Issue tags: -Needs release manager review

    I'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.

    Wim Leers’s picture

    Status: Needs review » Needs work

    @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.

    tedbow’s picture

    Status: Needs work » Needs review
    FileSize
    562 bytes
    44.74 KB

    Here 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

    +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
    @@ -148,15 +358,88 @@ public function testInfoParserCommonInfo() {
    +      'current_minor' => [
    +        'current_minor',
    +        "~$major.$minor",
    +        FALSE,
    

    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.

    Wim Leers’s picture

    Status: Needs review » Reviewed & tested by the community

    Per my comment in #208. #209 is the 8.7 patch, #204 is the 8.8 patch.

    larowlan’s picture

    Status: Reviewed & tested by the community » Needs work
    Issue tags: +Needs release manager review, +needs profiling

    Code review

    1. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
      @@ -10,6 +11,34 @@
      +   * The earliest Drupal version that supports the 'core_version_requirement'.
      

      We need an issue summary update here, because it references the old name

    2. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
      @@ -10,6 +11,34 @@
      +  protected static function satisfies(string $version, $constraints) {
      

      We can't have string typehints in 8.7 because php 5?

    3. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
      @@ -28,6 +57,39 @@ public function parse($filename) {
      +        $parsed_info['core_incompatible'] = !static::satisfies(\Drupal::VERSION, $parsed_info['core_version_requirement'] ?? $parsed_info['core']);
      

      same here re php7 features

    4. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
      @@ -60,7 +122,49 @@ public function parse($filename) {
      +          if ($minor_version === static::FIRST_CORE_VERSION_REQUIREMENT_SUPPORTED_VERSION) {
      +            $evaluated_constraints[$constraint] = FALSE;
      +            return $evaluated_constraints[$constraint];
      +          }
      +          if (static::satisfies($minor_version, $constraint)) {
      +            $evaluated_constraints[$constraint] = TRUE;
      +            return $evaluated_constraints[$constraint];
      +          }
      +          if ($patch === 0) {
      +            foreach (['alpha', 'beta', 'rc'] as $suffix) {
      +              foreach (range(0, 10) as $suffix_num) {
      

      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.

    5. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
      @@ -81,11 +81,17 @@ public function addUninstallValidator(ModuleUninstallValidatorInterface $uninsta
      +      if (!empty($module_data[$module]->info['core_incompatible'])) {
      
      @@ -110,6 +116,9 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
      +            if ($module_data[$dependency]->info['core_incompatible']) {
      

      in one instance we use empty, in another we don't - should we be consistent? the empty check is more defensive

    6. +++ b/core/modules/system/src/Controller/SystemController.php
      @@ -223,7 +223,7 @@ public function themesPage() {
      +        $theme->incompatible_core = $theme->info['core_incompatible'];
      

      any reason we can't use the existing key?

    7. +++ b/core/modules/system/src/Form/ModulesListForm.php
      @@ -294,10 +294,14 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
      +        '@core_requirement' => $module->info['core_version_requirement'] ?? $module->info['core'],
      

      php 7 features again

    Wim Leers’s picture

    #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.

    tedbow’s picture

    Assigned: Unassigned » 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.

    tedbow’s picture

    Issue summary: View changes
    Status: Needs work » Needs review
    FileSize
    6.58 KB
    46.13 KB
    46.62 KB

    re #211

    1. Updated the summary
    2. fixed
    3. Sorry I heard we could use this. Anyways I found another 1 in core https://git.drupalcode.org/project/drupal/blob/8.8.x/core/modules/search...
      but that is only in 8.8.x not backported to 8.7.x. Does it need to be fixed anyways?
    4. I split this into a new function
      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.

      • With just core if we convert all info files use core_version_requirement: ^8 since this the same constraint strings isConstraintSatisfiedByPreCoreVersionRequirementCoreVersion() will only evaluate 1 time for all for core modules when \Drupal\Core\Extension\ExtensionList::doList() is called and all info files are parsed.
      • Currently not may modules actually use the - 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 in isConstraintSatisfiedByPreCoreVersionRequirementCoreVersion being evaluated 3x for all modules that use 1 of these
      • There will be modules that need different version constraints of course but still there will be many modules that share constraints. For instance if we introduce a new feature awesome feature in 9.1.0 the we will probably get a lot of ^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.

    5. The first one is checking !empty because $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 it

      if (!isset($module_data[$dependency])) {
         // The dependency does not exist.
         throw new MissingDependencyException("Unable to install modules: module '$module' is missing its dependency module $dependency.");
      }

      which 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.

    6. fixed. We don't to set this at all.
    7. fixed

    The last submitted patch, 214: 2313917-8.7.patch, failed testing. View results

    Status: Needs review » Needs work

    The last submitted patch, 214: 2313917-214.patch, failed testing. View results

    tedbow’s picture

    Assigned: tedbow » Unassigned
    Status: Needs work » Needs review
    FileSize
    46.68 KB
    804 bytes
    46.18 KB
    +++ b/core/modules/system/src/Controller/SystemController.php
    @@ -222,8 +222,6 @@ public function themesPage() {
    -        // Ensure this theme is compatible with this version of core.
    -        $theme->incompatible_core = $theme->info['core_incompatible'];
    

    incompatible_core was used somewhere else. Replaced.

    Status: Needs review » Needs work

    The last submitted patch, 217: 2313917-217-8.7.patch, failed testing. View results

    tedbow’s picture

    Status: Needs work » Needs review

    The 8.7.x patch for #217 failed because of failing test in 8.7.x branch. Retesting

    Wim Leers’s picture

    FileSize
    30.86 KB

    RE: performance. I think actual profiling of this may be overkill.

    1. The first component to test is \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.
    2. The second component to test is how long it would take to check all discovered extensions' core constraint against those 611 versions. Both in a common scenario (where most extensions have the core version constraint) and in a worst-case scenario (where pretty much every extension has a unique core version constraint). I wanted to use 3v4l.org again, but the test script is too big 😃 So the test script is attached instead. It just extracted the Semver::satisfies() logic and its dependencies, plus the version comparision logic that InfoParser runs.

      On my machine:

      $ php --version
      PHP 7.2.14 (cli) (built: Jan 12 2019 05:21:04) ( NTS )
      Copyright (c) 1997-2018 The PHP Group
      Zend Engine v3.2.0, Copyright (c) 1998-2018 Zend Technologies
          with Zend OPcache v7.2.14, Copyright (c) 1999-2018, by Zend Technologies
      wim.leers at Acquia in ~/Work/d8 on 8.8.x*
      $ php test.php
      8 unique constraints to check.
      1007 total constraints to check.
      common scenario: 14.349937438965 milliseconds
      
      611 unique constraints to check.
      worst case scenario: 1786.4348888397 milliseconds
      

      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!

    Mixologic’s picture

    isConstraintSatisfiedByPreCoreVersionRequirementCoreVersion 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 a core_version_requirement key set, then the *only* acceptable value for core_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 no core: key

    In 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.

    tedbow’s picture

    @Mixologic

    What if we checked to see if they have *both* a core key and a core_version_requirement key set, then the *only* acceptable value for core_version_requirement is ^8 || ^9 in that case?

    As you sadi in #147

    About half of the modules in the ecosystem work with *every version of d8 out there*.

    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 besides core_version_requirement: ^8 || ^9 would be useful.

    re

    core_version_requirement: ^8.5 with no core: key

    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 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.

    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 genericisConstraintSatisfiedByPreviousVersion () 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.

    Wim Leers’s picture

    […] feels like an overly complex solution to shield us from edge case maintainer mistakes.

    I 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.

    tedbow’s picture

    I chatted with @Mixologic more about this issue.

    1. after that chat I think it is even more important to have something like 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 discussed
    2. I still think it is also useful on this issue for reasons I stated in #222
    3. I think after chatting with @Mixologic I Just wanted to clarify what the "worst case scenario" is that @Wim Leers did performance testing on in #220.2

      The "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 for core_version_requirement. So for all 611 modules only 1 could have core_version_requirement: ^8 || ^9 and only 1 could have core_version_requirement: ^8.8 || ^9 . Just with those 2 constraints that would be probably most modules.

      So tested with

      • 300 unique constraints to check: 527 milliseconds
      • 150 unique constraints to check: 132 milliseconds
      • 50 unique constraints to check: 16 milliseconds
      • 25 unique constraints to check: 5 milliseconds

      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

    4. I updating the version the functions that are in #3005229-82: Provide optional support for using composer.json for dependency metadata.

      This changes isConstraintSatisfiedByPreCoreVersionRequirementCoreVersion($constraint) to isConstraintSatisfiedByPreviousVersion($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.

    Gábor Hojtsy’s picture

    Issue summary: View changes

    Cleaned 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.

    tedbow’s picture

    @Gábor Hojtsy thanks for updating the summary

    doing a couple other updates

    1. removing: Determine whether there is better name than 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.

    2. Also it has been confirmed by @catch in #207 that this should be backported and he remove the Needs release manager review tag because @xjm has also been active in discussions around this issue

      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)

    3. A couple other small updates to the summary
    4. Adding figuring out profile .info.yml files.

      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 new core_version_requirment would be ignore for installing profiles except The validation in the parser for core_version_requirment and core 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 with core_version_requirment: ^8.9 or core: 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.

    Wim Leers’s picture

    Issue summary: View changes
    Status: Needs review » Reviewed & tested by the community
    Issue tags: -needs profiling
    1. #224.3: Sorry for making that not more clear — I hoped the bold bits were sufficiently clear, they obviously were not. Good call to explicitly test typical scenarios. One last addition: this happens only in the modules administration UI and when listing extensions through other means (e.g. Drush, Drupal Console). Once modules are installed and your site is just responding to requests, none of this matters. Perhaps most importantly: none of this has any effect when invoking hooks for installed modules, because the list of installed modules is stored as a container parameter at container build time!
    2. #224.4: 👍🤔 Perhaps "older" instead of "previous" would be even more clear?
    3. #225: 👍 — one thing that's not yet correct after this update is the current minor's next patch release in which this will ship (which we've got release manager sign-off for, see @catch in #207). That's because this issue has been in progress for a while! I see @tedbow already fixed that! 👍
    4. #226.4: 👍 Yes, fixing how profile info.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! 🤞

    catch’s picture

    1. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
      @@ -28,6 +57,45 @@ public function parse($filename) {
      +        // 'core_version_requirement' will be ignored in previous versions.
      +        if (!$supports_pre_core_version_requirement_version && isset($parsed_info['core'])) {
      +          throw new InfoParserException("The 'core_version_requirement' constraint ({$parsed_info['core_version_requirement']}) requires the 'core' not be set in " . $filename);
      +        }
      

      Nit: stray 'the' or missing 'key', probably the 'core' key is better, fixable on commit.

    2. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
      @@ -28,6 +57,45 @@ public function parse($filename) {
      +          throw new InfoParserException("The 'core_version_requirement' can not be used to specify compatibility specific version before " . static::FIRST_CORE_VERSION_REQUIREMENT_SUPPORTED_VERSION . " in $filename");
      

      Nit: missing '[specify compatibility] for a [specific version]' - fixable on commit.

    3. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
      @@ -60,7 +128,80 @@ public function parse($filename) {
      +  }
      +
      +  /**
      +   * Gets all the versions of Drupal 8 before a specific version.
      +   *
      +   * @param string $version
      +   *   The version to get versions before.
      +   *
      +   * @return array
      +   *   All of the applicable Drupal 8 releases.
      +   */
      +  protected static function getAllPreviousCoreVersions($version) {
      

      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.

    Wim Leers’s picture

    #228.3++ — I thought about that at some point but failed to write it. Great point!

    catch’s picture

    • catch committed f0e313a on 8.8.x
      Issue #2313917 by tedbow, pwolanin, jhedstrom, Wim Leers, Mixologic,...

    • catch committed b015ba7 on 8.7.x
      Issue #2313917 by tedbow, pwolanin, jhedstrom, Wim Leers, Mixologic,...
    catch’s picture

    Version: 8.8.x-dev » 8.6.x-dev
    Status: Reviewed & tested by the community » Patch (to be ported)

    Fixed #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.

    catch’s picture

    Oh 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.

    tedbow’s picture

    @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

    Oh 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 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.

    tedbow’s picture

    😫
    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

    name: Node
    type: module
    core_version_requirement: ^8
    

    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 as core: 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

    name: My module
    type: module
    core_version_requirement: ^8
    

    with no core

    Just as we throw exception if you do

    name: My module
    type: module
    core: 8.x
    core_version_requirement: ^8.8
    

    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.

    tedbow’s picture

    re #233

    There was some discussion here whether we should attempt to backport this to 8.6.x.

    I was thinking about this and this also be complicated.

    Right now

    +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
    @@ -10,6 +11,34 @@
    +  /**
    +   * The earliest Drupal version that supports the 'core_version_requirement'.
    +   */
    +  const FIRST_CORE_VERSION_REQUIREMENT_SUPPORTED_VERSION = '8.7.7';
    

    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.

    catch’s picture

    Version: 8.6.x-dev » 8.8.x-dev
    Status: Patch (to be ported) » Needs review

    That'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.

    tedbow’s picture

    Here 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.

    catch’s picture

    Given #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.

    Wim Leers’s picture

    Assigned: Unassigned » tedbow

    RE: 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.

    Gábor Hojtsy’s picture

    Assigned: tedbow » Unassigned
    Status: Needs review » Fixed
    Issue tags: -Needs release manager review, -Needs framework manager review
    tedbow’s picture

    Gábor Hojtsy’s picture

    I 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 :)

    mondrake’s picture

    Follow 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.

    tedbow’s picture

    @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()

    1. It passed untouched
    2. It relies on node_access_test
    3. It removed the core key all together in node_access_test.info.yml I did get an exception from \Drupal\Core\Extension\InfoParserDynamic::parse() about the missing core key
    4. But if just changed the core key to 7.x I don't get an error
    5. Kernel tests install their module via \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.

    mondrake’s picture

    @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

    Mixologic’s picture

    The 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

    Mixologic’s picture

    I see a lot of this pattern in core currently:

        if (method_exists($this, 'expectException')) {
          $this->expectException(\BadMethodCallException::class);
          $this->expectExceptionMessage('Method Drupal\Component\Datetime\DateTimePlus::diff expects parameter 1 to be a \DateTime or \Drupal\Component\Datetime\DateTimePlus object');
        }
        else {
          $this->setExpectedException(\BadMethodCallException::class, 'Method Drupal\Component\Datetime\DateTimePlus::diff expects parameter 1 to be a \DateTime or \Drupal\Component\Datetime\DateTimePlus object');
        }

    Which seems to be the correct workaround to address those.

    xjm’s picture

    Version: 8.8.x-dev » 8.7.x-dev
    Status: Fixed » Needs work

    Let'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.)

    tedbow’s picture

    I 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

    xjm’s picture

    Status: Needs review » Fixed

    This 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. :)

    Status: Fixed » Closed (fixed)

    Automatically closed - issue fixed for 2 weeks with no activity.

    catch’s picture

    Issue tags: +Drupal 8.8.0 highlights, +Drupal 8.8.0 release notes
    xjm’s picture

    Issue tags: -Drupal 8.8.0 highlights, -Drupal 8.8.0 release notes +8.8.0 highlights, +8.8.0 release notes
    xjm’s picture

    Issue summary: View changes

    Adjusting the release note to make the CR link accessible. Looks good otherwise, thanks!