Allow contrib extensions to be enabled when core branch is 11.x/main

Created on 14 September 2023, over 1 year ago

Problem/Motivation

Currently, contrib modules and themes can't be installed on a git clone of core that's at 11.x. This prevents testing of contrib modules on the latest development branch, and also affects DX because there are many useful contrib modules which are useful to developers (Devel, Admin toolbar, Config Devel, etc etc).

Steps to reproduce

Proposed resolution

Change InfoParserDynamic to allow installing modules when this conditions are satisfied:

- core is 11.x or main
- the extension to install has a requirement of the highest stable version - so currently 10.1.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

πŸ› Bug report
Status

Active

Version

11.0 πŸ”₯

Component
ExtensionΒ  β†’

Last updated 10 days ago

No maintainer
Created by

πŸ‡¬πŸ‡§United Kingdom joachim

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @joachim
  • πŸ‡«πŸ‡·France dqd London | N.Y.C | Paris | Hamburg | Berlin

    is this possibly a reason for this behavior?

    @trigger_error('Calling InfoParserDynamic::__construct() without the $app_root argument is deprecated in drupal:10.1.0 and will be required in drupal:11.0.0. See https://www.drupal.org/node/3293709', E_USER_DEPRECATED);

  • πŸ‡¬πŸ‡§United Kingdom joachim

    This is the bit that needs to be changed:

          // Determine if the extension is compatible with the current version of
          // Drupal core.
          try {
            $parsed_info['core_incompatible'] = !Semver::satisfies(\Drupal::VERSION, $parsed_info['core_version_requirement']);
          }
    
  • πŸ‡«πŸ‡·France dqd London | N.Y.C | Paris | Hamburg | Berlin

    Oops, sorry for the tagging, it was actually just a "looked at" the tags, but should not be added. Pressed enter too quick at night and I was actually too tired to look at it.

  • πŸ‡¬πŸ‡§United Kingdom joachim

    The tags look right to me - accidental removal because of a cross-post.

  • πŸ‡«πŸ‡·France dqd London | N.Y.C | Paris | Hamburg | Berlin

    Hah! double *facepalm* here....

  • 55:00
    7:20
    Running
  • Status changed to Needs review about 1 year ago
  • πŸ‡¬πŸ‡§United Kingdom joachim
  • 46:40
    32:35
    Running
  • Pipeline finished with Canceled
    about 1 year ago
    Total: 499s
    #33430
  • Pipeline finished with Failed
    about 1 year ago
    Total: 576s
    #33439
  • Status changed to Needs work about 1 year ago
  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    This has broken some legitimate tests.

  • πŸ‡¬πŸ‡§United Kingdom joachim

    Hmmm any thoughts on how we allow for this:

    > Behat\Mink\Exception\ResponseTextException: The text "System core incompatible semver test (incompatible with this version of Drupal core)" was not found anywhere in the text of the current page.

  • last update about 1 year ago
    30,357 pass, 7 fail
  • Pipeline finished with Failed
    about 1 year ago
    Total: 707s
    #33910
  • last update about 1 year ago
    30,417 pass
  • Pipeline finished with Success
    about 1 year ago
    Total: 741s
    #33958
  • Status changed to Needs review about 1 year ago
  • πŸ‡¬πŸ‡§United Kingdom joachim
  • Status changed to Needs work about 1 year ago
  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    Added a bit of feedback.

  • last update about 1 year ago
    30,357 pass, 9 fail
  • Status changed to Needs review about 1 year ago
  • πŸ‡¬πŸ‡§United Kingdom joachim
  • Pipeline finished with Failed
    about 1 year ago
    Total: 669s
    #35653
  • Status changed to Needs work about 1 year ago
  • πŸ‡¬πŸ‡§United Kingdom joachim

    @longwave the new failures with the current condition mean that all tests that do this sort of thing:

        $base_info = ['name' => $extension_name];
        if ($extension_type === 'theme') {
          $base_info['base theme'] = FALSE;
        }
    

    must now set the package.

    Is that ok?

  • πŸ‡ΊπŸ‡ΈUnited States cmlara

    I’ll note there is an open issue to consider removing core_version_requirement in light of the fact core is moving to a composer only workflow. ✨ Remove requirement for core_version_requirement for module to be compatible with Drupal 9 Active

    Perhaps that is a better alternative?

    I’ve made the mistake a few times of failing to update the version requirements in a module, CI rightfully threw and error warning me that if I were to tag a release it would not work. I’ve also hade CI alert me that a dependency was not ready.

    There are various testing regiments I’ve seen, some test against latest release and others test against latest head.

    This change appears to make it more likely that a module owner may accidentally create a release that can not actually be installed unless they make sure to only test against tagged core releases.

    I’m not a big fan of development branch code having special logic, it makes it harder to know the real issues, and makes it more likely to confuse users.

  • πŸ‡¬πŸ‡§United Kingdom joachim

    ✨ Remove requirement for core_version_requirement for module to be compatible with Drupal 9 Active has been open for 2 years and is not moving forward very quickly.

    This issue is something that needs to be fixed **soon** so that contrib developers can work on their projects against the core development branch.

  • πŸ‡ΊπŸ‡ΈUnited States cmlara

    has been open for 2 years and is not moving forward very quickly.

    True though there hasn’t been a good argument for it either. This might be what is needed to revive it. I’m sure that issue stalled out a bit as effort went into adopting modules for D9 and D10 as its need became much less important with the community stepping up. This is now not a technical reason rather than a contributor time reason.

    This issue is something that needs to be fixed **soon** so that contrib developers can work on their projects against the core development branch.

    Isn’t this more related to the fact that until we move to a MAIN branch we have 11.x defined as const VERSION = '11.0-dev';, If anything we would wanting that to be 10.2-dev until we split off 10.2 to its own branch and than it gets updated to 10.3-dev. IIRC that was because some tests were dependent upon the branch name matching the version, not sure if that’s been solved yet, but that would be a root issue here wouldn’t it?

    That also I believe brings us back to we need to lab test a few solutions to the GIT Branch Alias problems we have encounter as IIRC that’s one of the major problems preventing switching to a MAIN branch. Last I heard that was just waiting for manual testing of a few scenarios to unblock other development work.

    To me, and maybe there something I’m missing, this feels like this is just ignoring the root problems which are already known and that if effort was put into them, make this fix moot at best, and at worst make it a DX/UX trap.

    Also I’ll add that Drupals own development docs recommend developing Contrib against the next Core release until it’s in beta as API is subject to change. I don’t usually expend any effort until at least the alphas and it’s usually a one-off test just to make sure there are no major breaks when I’ll look at it in beta as I expect most of core to be in heavy flux until that time.

    Certain modules (like Devel) do need to develop against the latest core early to be useful, however that is a fraction of the ecosystem and workarounds likely exist (patching the module locally, or locally disabling the core check, running a Main branch that doesn’t publish dev releases that all code goes into before being cherry-picked to the latest active release branch, etc)

  • πŸ‡¬πŸ‡§United Kingdom joachim

    > Certain modules (like Devel) do need to develop against the latest core early to be useful, however that is a fraction of the ecosystem and workarounds likely exist (patching the module locally

    It's not just that.

    As a developer, working on Drupal core, I need Devel module for DX. And also Admin Toolbar, and other contrib modules that make core more usable.

    Thinking about it, fixing ✨ Remove requirement for core_version_requirement for module to be compatible with Drupal 9 Active won't solve this anyway -- the extension system would be changed to read the composer.json file, which will ALSO report that the module can't be installed on core 11.x.

    > Isn’t this more related to the fact that until we move to a MAIN branch we have 11.x defined as const VERSION = '11.0-dev';,

    Yes. But AFAIK, we don't know how this sort of thing will work once we have a 'main' branch. It may be we need a similar hack to this MR.

  • πŸ‡ΊπŸ‡ΈUnited States cmlara

    won't solve this anyway -- the extension system would be changed to read the composer.json file, which will ALSO report that the module can't be installed on core 11.x

    That is also true with the suggested fix in this issue, composer itself will still reject the install. The core_version_requirement key is included in the package server during publication (as it should be). In both cases a developer would still need to do "require drupal:^11@dev as 10.3.0" (or similar) to bypass composer or downloading the modules via source manually.

    As a developer, working on Drupal core, I need Devel module for DX. And also Admin Toolbar, and other contrib modules that make core more usable.

    That is more an argument that those modules should maintain a development branches with a composer.json that does indicate support for whatever the next release is in the main branch equivalent (but don't publish it as a tagged release so it requires explicit effort to download as a source branch with composer or git)

    Yes. But AFAIK, we don't know how this sort of thing will work once we have a 'main' branch. It may be we need a similar hack to this MR.

    True, though I would contend we should figure that out first, that could allow this to be more surgical or even unnecessary.

  • πŸ‡¬πŸ‡§United Kingdom joachim

    > That is also true with the suggested fix in this issue, composer itself will still reject the install

    There's already something to handle that in https://github.com/joachim-n/drupal-core-development-project

    And I don't think it will necessarily be true -- a core system that checks for core compatibility in module composer.json files might not go via the Composer API but might read the JSON files directly.

    > That is more an argument that those modules should maintain a development branches with a composer.json that does indicate support for whatever the next release is in the main branch equivalent (but don't publish it as a tagged release so it requires explicit effort to download as a source branch with composer or git)

    No! I REALLY don't want to be running dev branches of the tools I need to work on core. I want stable versions that I can rely on.

    > True, though I would contend we should figure that out first, that could allow this to be more surgical or even unnecessary.

    I asked core managers about this at DrupalCon Lille and they said that a. There is no timescale at all for when we move to a main branch, and b. There is no sort of plan yet on how that will work.

    I think we need contrib projects to be installable on core 11 now.

  • πŸ‡ΊπŸ‡ΈUnited States cmlara

    There's already something to handle that in https://github.com/joachim-n/drupal-core-development-project

    Could that project also patch the info.yml than too?

    I think we need contrib projects to be installable on core 11 now.

    That is already doable with local patching (once you have the source downloaded) using standard composer tools, without making Drupal respond differently on development branches.

    Core 11 doesn't even exist yet.

    What I think you really want is for D.O to open a 10.3 branch now, still commit (and tag all issues) to D11 (so we have a 'main' branch) but backport them in real time to 10.3 so testing can work correctly.

  • πŸ‡¬πŸ‡§United Kingdom catch

    We branched 10.2.x recently and it's identical to 11.x in all respects except the version name.

    The next branches will be 10.3.x, at that point 11.x is likely to start accepting 11.x-only changes.

    Given that, this is unlikely to be an issue until we actually have a 'main' branch, and that should be solvable via a branch alias (we hope). All this to say I'm not sure this issue is actually needed, the past six months has mostly been a temporary transitional situation until 11.x and 11.0.x coexist.

  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    I still think we're going to need this, or something similar, when we have the main branch? Otherwise nobody will be able to install any contrib there, because nothing should declare compatibility with it. So in the meantime while 11.x is acting as main I think we should still try to fix it.

  • πŸ‡¬πŸ‡§United Kingdom joachim

    https://www.drupal.org/project/devel_a11y β†’ is another reason for needing this.

    @longwave This is pending a decision from you about #14.

  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    Re #14 I think it's fine for modules that are testing functionality outside of this to have to set the "Testing" package. I am not sure what alternative we have apart from trying to special case them in the if statement instead, which feels more fragile.

  • Pipeline finished with Canceled
    10 months ago
    Total: 371s
    #105087
  • Pipeline finished with Failed
    10 months ago
    Total: 568s
    #105091
  • Status changed to Needs review 10 months ago
  • πŸ‡¬πŸ‡§United Kingdom joachim

    Rebased and added package as per #25.

  • Status changed to Needs work 10 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Did not test but appears to have some test failures.

  • Pipeline finished with Canceled
    10 months ago
    #105870
  • Pipeline finished with Failed
    10 months ago
    Total: 477s
    #105871
  • Pipeline finished with Success
    10 months ago
    Total: 541s
    #105901
  • Status changed to Needs review 10 months ago
  • πŸ‡¬πŸ‡§United Kingdom joachim
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    There a good way to test?

    Applied the MR and tried to download a module but still get the error.

  • πŸ‡¬πŸ‡§United Kingdom joachim

    What error did you get?

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    - drupal/devel[5.1.0, ..., 5.x-dev] require drupal/core ^9 || ^10 -> found drupal/core[9.0.0-alpha1, ..., 9.5.x-dev, 10.0.0-alpha1, ..., 10.3.x-dev] but the package is fixed to 11.x-dev (lock file version) by a partial update and that version does not match. Make sure you list it as an argument for the update command.

  • πŸ‡¬πŸ‡§United Kingdom joachim

    That's a Composer error. This issue is not about Composer. It assumes you have already installed the package with Composer and you are now trying to install it in Drupal.

    (To install it with Composer, use https://github.com/joachim-n/drupal-core-development-project)

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    So I cloned the devel module with the MR still applied and still can't install.

  • πŸ‡¬πŸ‡§United Kingdom joachim

    Have you cleared caches? It turns out the extension system caches .info file data -- see the InfoParser class.

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Cleared cache multiple times.

  • πŸ‡¬πŸ‡§United Kingdom joachim

    Can you try using the admin UI rather than Drush please?

    I just managed to enable admin toolbar using that.

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Ah okay I found the issue after using admin_toolbar which did work.

    Devel has php: 7.4 in it's info files which blocked it, even though the the warning is
    Requires: Drupal Core (^9 || ^10) (incompatible with version 11.0-dev)
    But that's probably a separate issue

    Anyway using admin_toolbar did work with the MR. So +1 from me.

  • Status changed to RTBC 10 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Been a few days and I rechecked with another module and didn't have any issue.

  • πŸ‡©πŸ‡°Denmark ressa Copenhagen

    The title makes this issue sound 11.x specific ... wouldn't this be more precise?

    "Allow any contrib extensions to be enabled in dev-version of Drupal core"

  • πŸ‡¬πŸ‡§United Kingdom joachim

    > "Allow any contrib extensions to be enabled in dev-version of Drupal core"

    AFAIK it's only a problem on 11.x and in the future main branch.

    If you're developing on 10.2.x, then contrib modules that say ^10 will work because they satisfy the semver requirement.

  • πŸ‡©πŸ‡°Denmark ressa Copenhagen

    Ok, I just assumed that this MR will also override core_version_requirement: ^11 in a contrib extension, and allow installing it in a future D12-dev.

  • πŸ‡¬πŸ‡§United Kingdom joachim

    That's true, it will. But I'd assumed that by then core will be on a `main` branch?

  • Status changed to Needs work 9 months ago
  • πŸ‡³πŸ‡ΏNew Zealand quietone

    I removed the code changes to InfoParserDynamic and ran InfoParserUnitTest.php which passed. Therefore I think this should have another test case. And I was wondering what happens for a contrib module that has a test module in the Testing package.

  • πŸ‡¬πŸ‡§United Kingdom joachim

    > And I was wondering what happens for a contrib module that has a test module in the Testing package.

    Urgh yeah, will they fail to install in tests?

    How are contrib modules currently testing against 11.x?

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Can say currently I am not testing against 11.x

    IF I really really need to though I clone the module and edit the info file to let me install. Super pain though for a module that has a bunch of dependencies.

  • πŸ‡¬πŸ‡§United Kingdom joachim

    We're going to need another hack :(

    'Core Testing' and 'Testing'?

    Or a special property for those info files that specifically need to opt-out of this because they are testing compatibility?

  • πŸ‡ΊπŸ‡ΈUnited States cmlara

    How are contrib modules currently testing against 11.x?

    Most are probably not.

    For those that are testing in GitLabCi the 'opt into next major' job which re-writes info.yml files to allow installing on D11. A patch file can be provided to patch the dependencies into compatibility and the Lenient plugin allows downloading dependencies with composer.

    For local: The same process one would use to handle modules that currently do not support D10, Composer Lenient and patches (which is the same as for GitlabCi)

    As noted previously I still suggest against development release specific feature in the core and contend ✨ Remove requirement for core_version_requirement for module to be compatible with Drupal 9 Active would be a better route for this.

    https://www.drupal.org/project/backward_compatibility β†’ was mentioned in ✨ Allow Drupal core developers and site builders to download contrib module in the next version Active though I'm not sure how it interacts with kernel tests, presumably functional tests would be covered if used and unit tests don't care. It is certainly an option for solving the argument in #20 to allow installing development tools to work on core.

  • πŸ‡¬πŸ‡§United Kingdom joachim

    I don't understand how ✨ Remove requirement for core_version_requirement for module to be compatible with Drupal 9 Active would help here, since its issue summary says:

    > If the core_version_requirement line is present in a project, process it as normal.

    ✨ Allow Drupal core developers and site builders to download contrib module in the next version Active is not relevant here -- that issue is about installing packages with Composer. This issue is about enabling modules with the Core extension system.

  • πŸ‡©πŸ‡°Denmark ressa Copenhagen

    Actually, ✨ Allow Drupal core developers and site builders to download contrib module in the next version Active is both about downloading and installing, which I have clarified in the title.

    There's also this suggestion by @mglaman:

    It sounds like the core_version_requirement in the module should just be deprecated in favor of composer.json constraints, period.

    From #3267143-76: Add a composer plugin that supports 'composer require-lenient' to support major version transitions β†’ .

  • πŸ‡ΊπŸ‡ΈUnited States cmlara

    If the core_version_requirement line is present in a project, process it as normal.

    Since that issue hasn't been finalized it could be enhanced to remove honoring the core_version_requirement at all. Assuming that part of the proposal wasn't changed, over time it would likely be adopted by contrib to remove the line to reduce maintenance burden (personally I'm good with just composer.json and find it more flexible). This would likely be especially true for modules like Devel that know they need to be used on bleeding edge releases.

    is not relevant here -- that issue is about installing packages with Composer. This issue is about enabling modules with the Core extension system.

    I was simply giving credit to an issue that references this issue and reminds us that the backwards_compatibility module (that I forgot existed) was created as an opt-in method for contrib to solve many (if not all?) of the pain points previously raised here without forcing core to adopt dev-branch only logic.

    We are now also saying this is "going to need another hack". In my experience when a developer finds themself describing a solution as a 'hack' it has been a sign development is either going down a less ideal path, or the code is trying to do something very brittle. It is not always the case, however in my experience it is a warning sign to take a step back and consider a different approach.

  • πŸ‡¬πŸ‡§United Kingdom joachim

    > It sounds like the core_version_requirement in the module should just be deprecated in favor of composer.json constraints, period.

    That won't work either. Or rather, it'll still need a hack here, because a contrib module that says it's compatible with '^10' won't work with core 11.

    And yes, it's a hack. But so is composer-lenient! :)

  • πŸ‡ΊπŸ‡ΈUnited States cmlara

    That won't work either. Or rather, it'll still need a hack here, because a contrib module that says it's compatible with '^10' won't work with core 11.

    I don’t believe it would need a hack here if the info compatibility check was deprecated in 10.3 and completely ignored in 11.x.

    This issue would not fix the composer side either. Downloading with composer is never mandatory, I can always obtain the files myself and put it in my code base locally and happily satisfy composer by informing it that my local project provides the files.

  • πŸ‡¬πŸ‡§United Kingdom joachim

    The composer side is already fixed.

    If you deprecate then remove the info compatibility check, and don't necessarily install with Composer, then how is the extension system supposed to determine if a module is compatible or not?

  • πŸ‡ΊπŸ‡ΈUnited States cmlara

    If you deprecate then remove the info compatibility check, and don't necessarily install with Composer, then how is the extension system supposed to determine if a module is compatible or not?

    The simple answer is that it does not attempt to. A site owner has "forced" the software in, at that point it is on the site owner to take responsibility for where they utilize it.

  • πŸ‡¬πŸ‡§United Kingdom joachim

    Ah right. I think that kind of change is a much bigger conversation than what this issue is trying to do, and will take much longer to be dealt with (personally, I'm REALLY not in favour of replacing a human-readable and -writeable and commentable format with JSON which is at best only one of those things).

  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    I am not sure we can drop core_version_requirement entirely until we have stopped shipping tarballs. Once everyone is on Composer it seems fine to rely on Composer's constraints, but until then users are free to download incompatible tarballs and that is the only mechanism to prevent them from being used. But that is for another issue anyway.

    I was wondering what happens for a contrib module that has a test module in the Testing package.

    If this is no different now to how it currently stands then I think that is out of scope for fixing in this issue; this patch solves an immediate problem and while that is also a problem, it is a slightly different one that I think can be handled in a followup?

  • πŸ‡ΊπŸ‡ΈUnited States cmlara

    My main request is that this not be "dev branch specific". My tests should run the same against a tagged stable as the dev branch.

    Can the logic at least be changed to a Settings parameter to enable this?

    -    if (str_ends_with(\Drupal::VERSION, '-dev')) {
    +    if (Settings::get('ALLOW_MODULE__INSTALL_BYPASSS_VERSION_CHECKS', FALSE)) {
    

    A settings is not dev banch specific. is explicitly opt-in requiring the 'site owner' to make a conscious decision to utilize it.

    Being opt-in allows me as a developer to test against upcoming mainline strictly and yet still provide an option to solve the issues here.

  • πŸ‡¬πŸ‡§United Kingdom joachim

    Actually, I think the contrib testing modules problem might work already:

        if (!isset($parsed_info['core_version_requirement'])) {
          if (str_starts_with($filename, 'core/') || str_starts_with($filename, $this->root . '/core/')) {
            // Core extensions do not need to specify core compatibility: they are
            // by definition compatible so a sensible default is used. Core
            // modules are allowed to provide these for testing purposes.
            $parsed_info['core_version_requirement'] = \Drupal::VERSION;
          }
          elseif (isset($parsed_info['package']) && $parsed_info['package'] === 'Testing') {
            // Modules in the testing package are exempt as well. This makes it
            // easier for contrib to use test modules.
            $parsed_info['core_version_requirement'] = \Drupal::VERSION;
          }
    

    Doesn't that mean that if a contrib module a) is in the Testing package and b) does NOT specify core_version_requirement, then it's automatically taken to be compatible?

  • πŸ‡ΊπŸ‡ΈUnited States cmlara

    Doesn't that mean that if a contrib module a) is in the Testing package and b) does NOT specify core_version_requirement, then it's automatically taken to be compatible?

    That is what its suppose to do per the last CR on the subject https://www.drupal.org/node/3070687 β†’ .

    Some contrib do have a core_version_requirement specified as the exemption wasn't available in the initial 8.7.7 release. In some cases I've seen it carry over when unneeded, and in other cases some contrib still has API that is compatible with 8.7.7 and as such declares compatability.

    Expanding on my suggestion from #57, a setting could be done so that it can always install, even if in the testing package with core_version_requirement set.

    BTW: since this is in the InfoParser, does this impact how modules display in the UI? I would think this might remove any warning from the modules page that the module is 'incompatible' making it harder for a developer to realize how far from mainline they have diverged.

    Perhaps the bypass should be done in the Module Installer and Modules UI instead?

  • πŸ‡¬πŸ‡§United Kingdom joachim

    > Some contrib do have a core_version_requirement specified as the exemption wasn't available in the initial 8.7.7 release

    Note that this is for contrib *Testing* modules. Not the main module in a project.

    > does this impact how modules display in the UI

    Testing modules normally don't show in the UI.

  • πŸ‡ΊπŸ‡ΈUnited States cmlara

    Testing modules normally don't show in the UI.

    This code appears to impact non-testing root modules which would show up in the UI.

        if (str_ends_with(\Drupal::VERSION, '-dev')) {
          // We skip extensions in the 'Testing' package, because there fixture
          // modules which are specifically for tests to check for an extension
          // being incompatible.
          if (!isset($parsed_info['package']) || $parsed_info['package'] !== 'Testing') {
            $parsed_info['core_version_requirement'] .= ' || ' . \Drupal::VERSION;
          }
        }
    
    Note that this is for contrib *Testing* modules. Not the main module in a project.

    Correct. The core_version_requirement may be set in a testing module because the parent module needed to test D8.7.7-8.8.1 (inclusive). Some modules never removed it even though they no longer test against that old a version while some modules may still test against D8.7.7.

    Here is an example where I just didn't know I could omit core_version_requirement from a test module.

    My knowledge having improved, next time I'm in that file I will remove it (since that branch only needs to test D9+) however it is reasonable to to believe other modules will have a similar setup.

  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    I am starting to like the idea of a new setting as proposed in #57. This is both testable and predictable, and we have precedent with the setting that allows test modules to show in the UI.

    However the downside is that users may start to enable it in order to accomplish major version upgrades, and then encounter (and report) bugs that are due to incompatibilities they have deliberately put in place.

    Composer solves this problem by allowing per-package version aliases in composer.json. Would something similar work here? A setting containing an array of module names, that allows you to explicitly ignore/override core_version_requirement for those modules only?

  • πŸ‡¬πŸ‡§United Kingdom joachim

    > Here is an example where I just didn't know I could omit core_version_requirement from a test module.

    You couldn't before.

    With this MR, you now *should* if you want your module to be testable on 11.x. Hmm, that means we would need to skip the check for testing modules on other versions too. Argh, this issue is like Laurel & Hardy pipework :(

    > A setting containing an array of module names, that allows you to explicitly ignore/override core_version_requirement for those modules only?

    That's going to be a pain because a developer would have to keep adding to it for further modules they want to use on 11.x.

    > However the downside is that users may start to enable it in order to accomplish major version upgrades

    People are going to shoot themselves in the foot no matter what anyway. We can put some VERY LOUD documentation on the setting to explain that it's meant for contrib development work.

    > I am starting to like the idea of a new setting as proposed in #57.

    It's certainly less hacky than the current MR! It does mean an extra check of settings and an extra setting value loaded into memory.

  • Pipeline finished with Failed
    9 months ago
    Total: 742s
    #121095
  • πŸ‡ΊπŸ‡ΈUnited States cmlara

    Adding on to my comment in #61

    One more realization, would tampering at the InfoParser impact modules like upgrade_status that want to check on the modules and report? Is that another argument that this check should be done in the module installer and modules install form instead of in the InfoParser?

    Bonus to @joachim note, likely less calls to the new setting as it only needs to requested when an install is occurring (though it will still be in memory from settings.php) if done outside the parser.

    However the downside is that users may start to enable it in order to accomplish major version upgrades, and then encounter (and report) bugs that are due to incompatibilities they have deliberately or accidentally put in place.

    A fair concern, I would suggest if we don't tamper with the info.yml the modules page will already show the module is incompatible, it could also be modified to add a header warning that incompatible modules have been allowed to install with the configuration override.

    IIRC the status report already reports incompatible modules? If so if we don't tamper with the parser that would be another method for site owners to identify they are running outside of spec correct?

    Beyond that I almost think that using this for site upgrades might be a useful feature. Using a config setting isn't much different than site owners patching the info.yml, at least with this method we would "know" that a module is being installed without patches.

    All of the above are just suggestions, really for me as long as its not branch specific logic(use a setting) I'm mostly neutral on the proposal in general.

  • Pipeline finished with Success
    8 months ago
    Total: 1021s
    #153274
  • Pipeline finished with Success
    6 months ago
    Total: 606s
    #196540
  • Pipeline finished with Success
    6 months ago
    Total: 589s
    #212997
  • Status changed to Needs review 6 months ago
  • πŸ‡¬πŸ‡§United Kingdom joachim

    Created a new branch & MR with the new approach.

    I've not added any of the changes to tests yet, as some if not all won't be necessary.

  • Pipeline finished with Failed
    6 months ago
    Total: 178s
    #213104
  • Pipeline finished with Failed
    6 months ago
    Total: 574s
    #213112
  • Pipeline finished with Failed
    6 months ago
    Total: 165s
    #213131
  • Status changed to Needs work 6 months ago
  • πŸ‡ΊπŸ‡ΈUnited States cmlara

    From comment #61:
    Why are we changing this in the InfoParser instead of the MoudleInstaller?

  • Status changed to Needs review 6 months ago
  • πŸ‡¬πŸ‡§United Kingdom joachim

    Because InfoParserDynamic is where it's determined whether an extension is suitable for installation:

          $parsed_info['core_incompatible'] = !Semver::satisfies(\Drupal::VERSION, $parsed_info['core_version_requirement']);
    

    If we let 'core_incompatible' be set, then we have to act in a LOT more places. Not just ModuleInstaller, but every else that checks that key.

  • πŸ‡ΊπŸ‡ΈUnited States cmlara

    Good reason. I had it in my mind that it was done in the module handler.

    For the second half of #61 could this instead be something like:

    $is_compatible = Semver::satisfies(\Drupal::VERSION, $parsed_info['core_version_requirement']);
    $parsed_info['core_incompatible'] =!( $is_compatible || $ignore_core_version );

    (where $ignore_core_version is set to TRUE earlier in the run instead of appending the current core version to the parsed data)

    Goal being to avoid tamping with the data that is used elsewhere (like UI) to avoid hiding the fact the module is not actually current version compatible even though it’s installed.

  • πŸ‡¬πŸ‡§United Kingdom joachim

    > For the second half of #61 could this instead be something like:

    Good idea! Much cleaner than fiddling with the version string.

    I've made that change.

  • Pipeline finished with Failed
    6 months ago
    Total: 173s
    #213827
  • Status changed to Needs work 6 months ago
  • The Needs Review Queue Bot β†’ tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide β†’ to find step-by-step guides for working with issues.

  • Status changed to Needs review 6 months ago
  • πŸ‡¬πŸ‡§United Kingdom joachim

    Rebased.

  • Pipeline finished with Failed
    6 months ago
    Total: 254s
    #216162
  • Status changed to Needs work 6 months ago
  • The Needs Review Queue Bot β†’ tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide β†’ to find step-by-step guides for working with issues.

  • πŸ‡³πŸ‡ΏNew Zealand quietone
  • πŸ‡¬πŸ‡§United Kingdom joachim

    @quietone I'd specifically put 'enabled' because 'installed' is ambiguous - people were thinking this issue was about installing with Composer, when it's not.

  • Status changed to Needs review 6 months ago
  • πŸ‡¬πŸ‡§United Kingdom joachim
  • Pipeline finished with Success
    6 months ago
    Total: 474s
    #216499
  • πŸ‡©πŸ‡°Denmark ressa Copenhagen

    Thanks for all the great work @joachim, I very much look forward to this feature! I could really have used it the last few months for Admin Toolbar, which is still not Drupal 11 ready ( πŸ“Œ Add Drupal 11 support Needs review ).

    I do agree with @quietone that "install" is the correct word to use here then, since "download = Composer". I recently posted this comment:

    It's important to discern between downloading and installing modules, they are two very different things in Drupal.

    1. You download the module and its additional necessary libraries (defined in the module's composer.json file) with Composer.
    2. Then you install the module either via the GUI on the Extend page, or with Drush -- the module gets enabled, changes are made to the database (the configuration), necessary folders are created, etc.

    See https://www.drupal.org/docs/extending-drupal/installing-modules β†’ as well as these two issues for more discussions:

    From Download β‰  install β†’ .

    I have added a few words in the Issue Summary, to make the focus clear.

  • πŸ‡¬πŸ‡§United Kingdom joachim

    > You download the module and its additional necessary libraries (defined in the module's composer.json file) with Composer.

    I very much support efforts to define and clarify terminology, but it's going to be hard in this case because Composer itself talks of 'Installing dependencies' and has the command `composer install`.

    So I think we need to disambiguate the term 'install' in Drupal.

  • πŸ‡©πŸ‡°Denmark ressa Copenhagen

    Yes, it's a bit of a mess ... In the WordPress Plugins GUI, you press "Install", which in this case means both "download AND install".

    If you check the Project Browser issue, "download" and "install" were chosen for these two steps. But let's continue the discussion by all means, and disambiguate.

    Until a consensus has been reached, we probably need to be fairly precise when talking about different stages of module handling, to make sure everyone knows which part is in focus.

  • πŸ‡ΊπŸ‡ΈUnited States cmlara

    Visually (review of code action only) looks good to me, and appears to address my previously raised concerns.

    Question on the feature side of this: You mention test work, does that include adding a flag to KernelTestBase and above to allow this to be easily enabled for phpunit runs? Possibly pair it with an environment variable for setup in phpunit.xml or GitlabCi?

  • πŸ‡¬πŸ‡§United Kingdom joachim

    Ah right, you mean so that a kernel test in a contrib module will accept to enable that module in the test site?

    I hadn't actually thought of that! I'd like to get this in and leave that to a follow-up!

  • πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

    I've tested the MR on our project, with dozens of modules, from which >90% don't have an updated core_version_requirement. Works as expected by setting the kill-switch in settings.php. I wonder if this needs tests. Tentatively, marking as Needs tests

  • Status changed to Needs work 5 months ago
  • πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

    Maybe we can extend UpdateScriptTest::testExtensionCompatibilityChange() or ExtensionListTest::testCheckIncompatibility()?

  • πŸ‡¬πŸ‡§United Kingdom joachim

    joachim β†’ changed the visibility of the branch 3387322-allow-contrib-extensions-on-11x to hidden.

  • πŸ‡¬πŸ‡§United Kingdom joachim

    joachim β†’ changed the visibility of the branch 3387322-allow-contrib-extensions-on-11x-detect-dev-version to hidden.

  • Status changed to Needs review 5 months ago
  • πŸ‡¬πŸ‡§United Kingdom joachim

    Thanks for the hint! Got a unit test working.

  • Pipeline finished with Failed
    5 months ago
    Total: 179s
    #221098
  • Status changed to Needs work 5 months ago
  • The Needs Review Queue Bot β†’ tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide β†’ to find step-by-step guides for working with issues.

  • Status changed to Needs review 5 months ago
  • πŸ‡¬πŸ‡§United Kingdom joachim
  • Pipeline finished with Success
    5 months ago
    Total: 530s
    #221118
  • Status changed to Needs work 5 months ago
  • πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

    Thank you for adding tests. Apart from a leftover (see the MR), I think we need:

  • Pipeline finished with Failed
    5 months ago
    Total: 452s
    #221580
  • Status changed to Needs review 5 months ago
  • πŸ‡¬πŸ‡§United Kingdom joachim

    Not sure about the test failures -- web/core/modules/system/tests/src/Functional/Menu/LocalTasksTest.php is failing for me locally on 11.x

  • Status changed to Needs work 5 months ago
  • The Needs Review Queue Bot β†’ tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide β†’ to find step-by-step guides for working with issues.

  • Pipeline finished with Success
    5 months ago
    Total: 565s
    #232950
  • Status changed to Needs review 5 months ago
  • πŸ‡¬πŸ‡§United Kingdom joachim

    Rebased.

  • Pipeline finished with Success
    5 months ago
    Total: 524s
    #244664
  • Status changed to Needs work 2 months ago
  • The Needs Review Queue Bot β†’ tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide β†’ to find step-by-step guides for working with issues.

Production build 0.71.5 2024