Improve the "next major" phpunit jobs

Created on 10 January 2024, 12 months ago
Updated 11 September 2024, 3 months ago

Problem/Motivation

Once Drupal 11.0.0-alpha is out, we should refactor the "next major" fix. This is added as a measure to test against (the yet non-existent) 11.x. It is called "amend-core-requirements-drupal-11" and it technically fakes Drupal 11 support.

There were conversations on #3413333: Consider running PHPStan with "next minor & major" tests and #3412308: Consider providing support for dependencies in "next major" tests on whether this should be added at all, or expanded to do it as was in lenient-related modules. For now we're adding this lazy fix to the module being tested, but not to related ones. But this decision is opinionated.

"11.x" is the "main" branch, but it was called "11.x" for now (explained here https://www.drupal.org/about/core/blog/new-drupal-core-branching-scheme-... ).

There is no version yet of Drupal 11, so asking the modules to add it wouldn't make sense (yet).
Once the first alpha release is out, modules can and should start checking for compatibility and add it.

But the main thing to do in this issue is to refactor those lines that would fake compatibility.

Steps to reproduce

Drupal 10 modules can test for Drupal 11 compatibility by faking it, but if they need lenient for testing, they need MRs or patches for those modules to get it, which might not make a lot of sense whilst 11.0.0-alpha is not out.

Proposed resolution

Remove the check or maybe make it more generic so it catches other modules too (go through the contrib folder).
- If the fix needs to be made, store it and show the user a warning about it
- Based on that fix, decide whether we want to return success or fail even if the tests pass.

Remaining tasks

MR.

📌 Task
Status

Closed: outdated

Component

gitlab-ci

Created by

🇪🇸Spain fjgarlin

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

Comments & Activities

  • Issue created by @fjgarlin
  • 🇩🇪Germany jurgenhaas Gottmadingen

    Thanks for starting this follow-up. I'm not sure, though, if such a special treatment is even required. While there is in fact no Drupal 11 alpha yet, we can already "use" Drupal 11 with the dev release. And that's what's happening when testing against the next major. It's like a composer require drupal/core:^11.x-dev, no alpha required really.

    So, with that in mind, there is no reason why contrib modules couldn't already declare Drupal 11 compatibility in their info.yml file.

    But I'm not pushing on this, I'm also OK if we keep this "lazy fix" in the template and remove it later. The only potential downside is that then some tests may suddenly fail, that worked OK before, and then the noise might be bigger.

  • 🇪🇸Spain fjgarlin

    Note that the check is no longer in the "before_script" part, it's in an internal reusable template named .amend-core-requirements-drupal-11.

  • 🇨🇭Switzerland berdir Switzerland

    what about adding a warning when this is detected but still do it anyway?

    I think it's useful to see the current state without forcing compatibility.

    > So, with that in mind, there is no reason why contrib modules couldn't already declare Drupal 11 compatibility in their info.yml file.

    There are in fact reasons for that. 11.x is still in development, major changes still happen. The symfony 7 update isn't committed yet, phpunit will be updated to version 10. Those things are quite likely going to break your module. You can't take a compatibility back. once it's out there in a release, composer will install that, possibly breaking sites.

  • 🇪🇸Spain fjgarlin

    That's a good idea. Just add a warning if the CI job is "auto-fixing" the compatibility in the *.info.yml file.

  • 🇨🇭Switzerland berdir Switzerland

    I think we could even go as far as making the job fail in that case, since it is marked as being allowed to fail. Then even if the tests are passing, it's helpful in that it's running the tests but it also tells you very clearly that it is not actually "next major" compatible yet.

    I think now that it's all part of script, that would be fairly easy. store the output of sed (which I think varies depending on whether or not it did any replacements) in a variable, set that as exit code at the end.

  • 🇪🇸Spain fjgarlin

    Fully agree with that approach.

    I altered a bit the issue description.

  • 🇪🇸Spain fjgarlin

    The snippet has been refactored a few times, and made more generic here 📌 Update templates so 11.0 is the default/current branch RTBC .
    It also shows a diff of what's changed.

    We are in the same situation now with Drupal 12 and also have the "Upgrade status" job, where it provides fixes for the module.

    The only thing that we are not doing is to trigger an error, because in situations like this (Drupal 12 doesn't exist yet), it wouldn't make sense.
    Note that we also have the option to have an empty value for "next major", which will not run the variant.

    All in all, I think that the changes made in several issues made the current approach good enough. I'll close this issue unless we feel very strongly about throwing the error.

  • Status changed to Closed: outdated 3 months ago
Production build 0.71.5 2024