In #3102261: Update composer.json and rules.info.yml to require core 8.7 we made the change to rules.info.yml
- core: 8.x
+ core_version_requirement: ^8.7.7
At core 8.9 we now have three coding standards messages stating that this should also be added to the test files too.
---------------------------------------------------------------------------------------
1 | WARNING | "core_version_requirement" property is missing in the info.yml file
| | (DrupalPractice.InfoFiles.CoreVersionRequirement.CoreVersionRequirement)
---------------------------------------------------------------------------------------
We can leave the requirement at 8.7.7 for the time being, we just need that 'core_version_requirement' key in the files.
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | 3160795-13-composer-core-version-requirement.patch | 562 bytes | tr |
| #8 | 3160795-8.remove-core-and-set-package-as-testing.patch | 1.9 KB | tr |
| #7 | 3160795-7.remove-core-and-set-package-as-testing.patch | 1.5 KB | jonathan1055 |
Comments
Comment #2
jonathan1055 commentedPatch with +core_version_requirement: ^8.7.7 for the three test modules
Comment #3
megachriz@jonathan1055
If I'm correct, then
core_version_requirementdoesn't have to be added to test modules. What I think needs to be done instead is removing thecorekey. The dev version of Rules now requires Drupal 8.8, so removing thecorekey does no harm either.Comment #4
megachrizFrom https://www.drupal.org/node/3070687:
Comment #5
jonathan1055 commentedFor info, the tests were clean on 7th April, but by 19th April we had these three warnings. It was probably a new version of Coder which introduced the check.
[edit: cross posted, before I had seen the comments above]
Comment #6
megachrizFor your interest, Feeds Extensible Parsers has a test module without keys
coreandcore_version_requirementand no coding standard violation on that part: https://www.drupal.org/pift-ci-job/1762066Feeds has neither a coding standard violation on missing the
core_version_requirementkey for test modules: https://www.drupal.org/pift-ci-job/1756232Comment #7
jonathan1055 commentedThanks for the info MegaChriz. Yes I thought I had read somewhere about test modules needing neither 'core' nor 'core_version_requirement'. I just tried removing both and in my Travis build all the tests failed dramatically with
PHPCS locally was also still giving me the 3 coding standards messages. So I searched the Coder commit log, found issue #3104236: Add test for `core_version_requirement` key in `info.yml` files? and sure enough it does say "check for the core_version_requirement key in *.info.yml files, excluding info files of test modules".
So I checked DrupalPractice/Sniffs/InfoFiles/CoreVersionRequirementSniff.php and it verifies that the file is a test file, not by the file location, but by checking that the package is 'Testing'. The rules files all have
package: Rules. So with that change and removingCore: 8.xit works. I no longer get the coding standards messages, and the Travis test builds all run properly.Let's see what drupal.org testbot says
Comment #8
tr commentedThese changes require core 8.8.2+, and we've committed all sorts of patches from #3089502: [meta] Rules deprecated code in the past few days, so 8.7.7 is no longer accurate in rules.info.yml. Let's change this at the same time.
Comment #9
jonathan1055 commentedGood idea. Just queued a test at Core 8.8, but I think it was probably a waste of time, as the selection gave 8.8.8 anyway. Still, it will give us assurance.
Comment #10
jonathan1055 commentedRTBC
Comment #12
tr commentedCommitted.
Comment #13
tr commentedAnd we should really update the core version requirement in composer.json as well, as long as we're doing this.
Comment #14
tr commentedThe PHP 7.4 test in #13 was failing because of #3158445: Composer require failure due to "Failed to execute git clone" in drupalci/php-7.4-apache. That's still not resolved. Instead of trying to squeeze this change into this already-fixed issue, I think I will just take care of it when we have to update composer.json for D9, which should be any day now ... Moving the above patch to #3162077: Update dependencies for D9
Comment #15
jonathan1055 commentedI was wondering about that composer failure, so thanks for the background. Yes leave this as fixed. I've altered to title to reflect the other significant change done here.