Allow all sub-modules to be compatible with next_major

That was discussed here #3412308: Consider providing support for dependencies in "next major" tests → , and specifically in the MR comments (ie: https://git.drupalcode.org/project/gitlab_templates/-/merge_requests/101...). There might even be a slack thread about it, but I can't find it now.

The workaround is to patch whatever is needed and populate the COMPOSER_PATCHES_FILE variable.

We were talking about the possibility of doing a recursive "grep + sed" but it felt like it was out of the scope of the default templates, specially when trying to fix external modules. Note that MRs could be used for those modules and then use the COMPOSER_PATCHES_FILE for the fixes (the D11 automatic bot is already WIP and might start creating D11 compatibility issues).

I'd say "Closed (work as designed)" but will wait to see if there are further thoughts. If you're happy with the above explanation and workaround, then feel free to close it.

Thanks for the background. I did look at the start of #3412308: Consider providing support for dependencies in "next major" tests → but not all of the thread, and not the MR comments. So the purpose of that issue was to allow the contrib module itself to be able to run phpcs, phpstan, eslint, etc at the next major (to get useful early info) but not be able to run phpunit. I suppose that distinction makes sense. I see what you mean, it may give a false sense of hope that dependencies are future-compatible when there is no work started on those modules yet. So collaboration and composer patches is better.

See comments 14 to 17 here: #3437131: Allow installing modules on next major that have a drupal/core dependency in their composer.json →

I still think that the PATCHES+LENIENT should be preferred, but if we keep the scope to only project files (submodules within the main module), then I think it should be ok to do a recursive "grep+sed" on the info files.

Re-opening this issue.

@fjgarlin
A key note the comments in #3437131: Allow installing modules on next major that have a drupal/core dependency in their composer.json → are talking about sub-modules (those modules that are distributed with the module being tested) not dependencies (modules distributed in other packages)

I don't believe we ever discussed sub-modules in this issue, honestly I thought they were being included.

I would agree with Berdir that sub-modules make sense to include if we are going to keep updating the main module.info the sub-modules as are equivalent.

We should however not update external dependencies.

Side note
We may want to consider pushing ✨ Remove requirement for core_version_requirement for module to be compatible with Drupal 9 Active through and get rid of core_version_requierment in core which absolves us of this concern.

So if the project has sub-modules, with the current work already done, are these sub-modules not classed as compatible? I thought that sub-modules did not have a separate compatibility status and they were always the same as the main project. Maybe that was in previous core versions with .info before we moved to .info.yml ? The example I gave in the IS was for an external 3rd-party module. I will try to test what happens when a project has sub-modules only and no 3rd-party requirements, as I think that is the scenario which you are proposing to improve.

I thought that sub-modules did not have a separate compatibility status and they were always the same as the main project.

Each sub-module has its own module.info file. This isn't parsed into composer, they are however parsed by Drupal Core and would prevent installing the sub-module if it does not indicate compatibility with the core version.

The exception to that is TESTING modules (those that have the testing category and are usually placed in the tests/module folder) are permitted to omit core_version_requirement (however if they include it IIRC those too will be enforced)

Agree with all.

Lenient+patches for external dependencies.
grep+sed for internal submodules.

Let's go ahead and add a new line here

.amend-core-requirements-drupal-11: &amend-core-requirements-drupal-11
  - grep -q "\^11" *.info.yml || (grep -q "\^10" *.info.yml && sed -i "s/\^10/\^10 \|\| ^11/" *.info.yml)
  - TODO: Recursive grep+sed on all info.yml files within the project.

Need to transform the above into an MR.

Thanks @cmlara for the explanation in #8. Yes the sub-modules in my projects are all for testing purposes only, they have package: Testing in their .info.yml and do not have any core_version_requirement:. This is what I was thinking about when I wrote #7. Sorry for the distraction.

I lightly mentioned testing modules being different, however I should explicitly ask:
Should we be updating the info.yml for TESTING modules?

It does not look like the backwards_compatibility module actually tests the code at the moment, however modules that perform those actions and simialr might be examples where we would want to NOT update tests modules. Addtionaly since 8.8.2 the key could just be omitted, meaning that if the version key exists it should exist for a specific reason.

Also worth factoring in that it is easier to add additional commands than it is to remove existing commands (before_script vs re-writing the entire job).

Should we be updating the info.yml for TESTING modules?

phpunit (next major) just failed for one of my projects, because one of the test modules can't be installed on 11 due to the core requirement in the info.yml. That means that the job is pretty useless for that project right now.

What kind of scenarios are you thinking of where something like this shouldn't be updated by the job? If we are updating the main info.yml and the info.yml of sub-modules, wouldn't it then be the most logical choice to also update the info.yml of test modules?

I see some merit in not modifying anything, because then it is also less likely that you forget to make these changes when you are working on compatibility with next major, especially since the upgrade status job doesn't seem to show required info.yml updates at least for test modules (not sure about regular sub-modules right now). But I also think it doesn't make sense at all that I don't have to update the requirement for the main module or for sub-modules when I want to use phpunit (next major), but I need to do this for test modules. Also, project update bot added support for updating info.yml updates for sub-modules and test modules/themes a few months ago, so it is less likely that you miss this now when merging these MRs.

Alternatively, if we end up not doing this, then I would at least document how to use a before script to do this yourself, if desired. An other alternative would be to document that we recommend to remove core version requirement from info.yml files for test modules. If that is really the recommendation.

What kind of scenarios are you thinking of where something like this shouldn't be updated by the job? If we are updating the main info.yml and the info.yml of sub-modules, wouldn't it then be the most logical choice to also update the info.yml of test modules?

Validating a module can not be installed or testing with a module that is explicitly not compatible are concepts that may want to be tested by modules that modify certain areas of Core or wish to provide overall health reporting.

Any module that tampers with the ModuleInstaller or Core info parsing for example may wish to run those types of tests to validate that the code is functional and only operates when intended.

Upgrade Status (or any module providing similar reporting) would be a type of module that would want to review these values to test reporting and validate that the module correctly reports that a module is incompatible.

Will that be the majority, no, is it a possibility yes.

We could force those type of modules to re-write their test files after the wiper scripts execute however I’m not a fan of deviation from the source repo when it can be avoided and core_version_requirement is optional in test modules.

This is essentially a “do we want to make it easy for most by narrowing the target audience” question, it’s not necessarily wrong to do so, however it moves the template to using even more opinionated logic and over time decisions like this add up. Thankfully with GitLabCi we can always move to using our own code and don’t have to depend on this template. This is just a comment regarding how with decisions we could reach a point where the template is as opinionated as DrupalCi was unless we think out all the possible related scenarios.

Any module that tampers with the ModuleInstaller or Core info parsing for
example may wish to run those types of tests to validate that the code is
functional and only operates when intended.

That's kind of funny, because I've got such a module and I'm testing all of that. And I have a lot of test modules in that module, but not for different version constraints, there are actually other ways to validate that part. But I get what you had in mind now, I agree there are probably a small number of modules outside of core, that might actually need that indeed. I'm not sure that means we shouldn't do it by default, though. As you say, people with "exotic" needs could always modify the pipeline. Maybe we could add a flag to opt out?

If the best practice was to not use version constraints for test modules anyway, then it would be an entirely different discussion, but as I said earlier, I'm honestly not sure what the best practice is in this case.

For context here is the change notice related to this (and it’s shoe-horned into a previous release notice )

Test modules

Note that test modules can omit the core_version_requirement key entirely as of Drupal 8.8.2, so that they will work with any core version they are tested against. Prior to 8.8.2, the value is required for test modules and will cause site errors if it is not included. These errors are even encountered if $settings['extension_discovery_scan_tests'] = TRUE; is not enabled in settings.php.

Personal experience: I can say I had no clue it was optional, now that I know the next time I have to update test modules for a version jump (and all new modules I write) do not include it. That does. It make it a best practice, just my 2 cents on how I would use it.

BTW: if anyone wants to push 🐛 Allow contrib extensions to be enabled when core branch is 11.x/main Needs work through (+ maybe a followup for adding an environment variable for KernelTest usage) and all our logic around this goes away and it becomes a core problem not a templates problem.

I'm going to work on the approach agreed in #9. It might be a little different following the simplification in #3456092: Support all types of version constraints in core_version_requirement for NEXT_MAJOR run →

I have changed *.info.yml to **/*.info.yml in the sed command. Locally this finds the main .info.yml and the sub-module in a lower folder.

I also had to add empty string "" for the -i argument as I'm using macOS locally. If this is acceptable to leave in, then it will save the "unterminated substitute pattern error" for future developers (see point 3 on https://stackoverflow.com/questions/28592043/what-is-wrong-with-my-strin...)

I'll test this MR and report back.

The first test failed with

>>> Existing value of core_version_requirement: ^8 || ^9 || ^10
sed: can't read s/core_version_requirement.*/core_version_requirement: \^11/: No such file or directory

See https://git.drupalcode.org/project/scheduler/-/jobs/2185389#L49

This could have been due to the extra double-quote empty string, so I removed that, which made a difference, but the file(s) were not updated

>>> Existing value of core_version_requirement: ^8 || ^9 || ^10
>>> Updated value of core_version_requirement: ^8 || ^9 || ^10

See https://git.drupalcode.org/project/scheduler/-/jobs/2185437#L48

The grep+sed works locally in a terminal session (I'm not running gitlab templates locally), so I am not sure what to change next.

Maybe the "grep" PIPE "sed" needs to be turned into a loop? The first "grep" is still *.info.yml

Maybe we don't even need the "grep" at all? We can just change the value to what's needed regardless of whether the value is there already or not. That'll leave only the "sed" command.

I added some more debug, and it shows that the top-level *.info.yml file was not modified, but the sub-module one using **/*.info.yml was found and changed.

https://git.drupalcode.org/project/scheduler/-/jobs/2195188#L49

So one solution might be doing two commands. I will create a second sub-module just to make sure, as Scheduler currently only has one.
I will also look at not doing the grep at all. But the problem is that my local terminal commands behave slightly differently when run in the pipeline, and that makes development tricky.

I had the idea to just add the two patterns for info.yml into the same sed command, and it worked.

sed -i "s/core_version_requirement.*/core_version_requirement: \^11/" *.info.yml **/*.info.yml

There is extra debug, and this also tests a second and third .info.yml in lower folders, all can be seen in the log:
https://git.drupalcode.org/project/scheduler/-/jobs/2196271#L49

Ready for review, then I will remove the debug.

Note that if globstar is off (the default, probably also on CI, at least I got an error from ls **/*.info.yml when testing this), **/*.info.yml is the same as */*.info.yml:

Taking paragraphs module as an example:

$ echo *.info.yml
paragraphs.info.yml

$ echo **/*.info.yml
**/*.info.yml

$ shopt -s globstar
$ echo **/*.info.yml
modules/paragraphs_demo/paragraphs_demo.info.yml modules/paragraphs_library/paragraphs_library.info.yml modules/paragraphs_type_permissions/paragraphs_type_permissions.info.yml paragraphs.info.yml tests/modules/paragraphs_test/paragraphs_test.info.yml

$ find . -name "*.info.yml"
./tests/modules/paragraphs_test/paragraphs_test.info.yml
./paragraphs.info.yml
./modules/paragraphs_library/paragraphs_library.info.yml
./modules/paragraphs_type_permissions/paragraphs_type_permissions.info.yml
./modules/paragraphs_demo/paragraphs_demo.info.yml

find with an xargs to sed might be the more flexible option here

However, also keep in mind that composer has installed Drupal in the web folder, so that's also going to find every single core and contrib dependency .info.yml.

Also, I did steal this to make contrib dependencies compatible like this:

phpunit (next major):
  before_script:
    - 'sed -i "s/core_version_requirement.*/core_version_requirement: \^11/" web/modules/contrib/*/*.info.yml'

(I didn't need submodules in my case)

TIL about "globstar".

So, I guess we should maybe check or force-enable globstar so the current approach works.

Or, as suggested in the comment above find with an xargs to sed might be the more flexible option.

I have made some changes and this is ready for review. The files to change are detected using INFO_FILES=$(find -L . -name "*.info.yml") within the /modules/custom/$CI_PROJECT_NAME folder, not the top-level directory, to avoid finding every .info.yml. Then xargs sed as suggested.

There is lots of debug in the current MR, to show files found, before, after, etc. Most of this will be removed, but the final git diff could remain, to show exactly what has been altered in the process.

Without this MR - submodule scheduler_rules_integration cannot be installed and is incompatible with this version of Drupal core
https://git.drupalcode.org/project/scheduler/-/jobs/2313524

Test using MR240 - ignore the test failures, they key thing is that the sub-module can be installed and therefore the tests run
https://git.drupalcode.org/project/scheduler/-/jobs/2334054

Changing the title to reflect the reduced scope of this issue.

I've triggered the downstream pipelines: https://git.drupalcode.org/issue/gitlab_templates-3414505/-/pipelines/24...

I agree that we can keep the last "git diff" (the ones after the divider) to see the changes made. I think the rest of the debug can be removed now, so marking NW based on that, otherwise it could have gone to RTBC. I'm happy to do another review once the debug code has been moved out.

I've removed the debug. Here is run of the cleaner version
https://git.drupalcode.org/project/scheduler/-/jobs/2341034#L46

I had thought about this point before - it would be nice not to make the 'sed' do any change if the regex string to replace already contains ^11 otherwise we get the slightly confusing:

These files have been changed to allow testing for Next Major compatibility:
$ git diff $CI_PROJECT_DIR ':!:*composer*.json' || true
--- a/keycdn.info.yml
+++ b/keycdn.info.yml
@@ -2,6 +2,6 @@ name: KeyCDN
 type: module
 description: KeyCDN Integration
 core: 8.x
-core_version_requirement: ^8 || ^9 || ^10 || ^11
+core_version_requirement: ^11
 dependencies:
   - purge:purge

It still runs fine, and does not affect the outcome in general, but could there be cases where making the change above has a detriemntal effect?

I can create the regex that does not match if there is '11' see https://regex101.com/r/nAlOAW/1 but adding this locally into the sed does not work. Happy to leave this MR as-is for merging, but if you have any good ideas about the improved regex let me know.

I'm happy for that change to go in, if the code already has what's needed then there is no need to change it. NW based on that since you already figured out the regex and all.

Done. I made two improvements. The easier way to not match ^11 is with a separate /\^11/! at the start of the command, not part of the target pattern. The second improvment was to append || ^11 to the end of the line, not replace all the existing conditions.

Test run on https://git.drupalcode.org/project/scheduler/-/jobs/2346436#L45
You can see that only 3 files were edited, because the 4th one already has ^11.

Ready for review.

We have this:

-core_version_requirement: 11
+core_version_requirement: 11 || ^11

Does it make sense/is it expected?

Yes, I was aware of that, and was wondering if you were going to ask. It is expected, because the condition to avoid matching a line is specifically where the line does not contain ^11 so I added that test with a plain 11 to see what it did. I don't think many real projects would have 11 as a constraint without any qualifier such as ^

The alternative would/could be to avoid matching if the line contains just plain 11, but that would match 9.11 or 10.11 if such versions existed, and then the change to add ^11 would not get done. Maybe that's a better way to do it, realistically, because those version strings won't exist before this is all changed for the new next major 12. I was being cautious about making sure the edit was done, but it will probably avoid questions and confusion if I make that minor change now.

I've pushed that change as described above.

Retested https://git.drupalcode.org/project/scheduler/-/jobs/2353964#L45
The log no longer shows the change to /src/Access/deeper.info.yml which you highlighted above. I did add another new test file at that lower sub-directory, to check that it gets edited as required, because none of the existing test .info.yml files would get altered with this latest change.

Thanks for the changes. The MR and tests look good to me. I think it's ready. RTBC.

Merged.

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