Do not allow core: 9.x in info.yml files

Here is a patch

1. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
   @@ -68,37 +65,44 @@ public function parse($filename) {
   +          $invalid_core_message .= " The 'core' key can be removed because this module does not support any Drupal version prior to " . static::FIRST_CORE_VERSION_REQUIREMENT_SUPPORTED_VERSION;
   ...
   +          $invalid_core_message .= " The 'core' key must be set to '8.x' to support Drupal versions prior to " . static::FIRST_CORE_VERSION_REQUIREMENT_SUPPORTED_VERSION;
   

   These messages are giving the developer a lot of info but I think it is good to explain as much as we can.

2. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
   @@ -131,7 +135,7 @@ public function parse($filename) {
      protected function getRequiredKeys() {
   -    return ['type', 'name'];
   +    return ['type', 'name', 'core_version_requirement'];
      }
   

   core_version_requirement becomes required. I moved the check for the missing keys after we set it for core and testing packages.

Failed all functional tests because there was not exception for test info.yml files. Fixed.

But I don't think this approach is a good one now. In Drupal 8 if you had core: 7.x or core: 4.x it would not cause a parsing error you just couldn't enable the module. If we give a parser error now then any code bases that might have this random values will as soon as the replace core with DRupal 9 start to have this hard error.

I think rather we should just check for core: 9.x and not not 8.x.

Here is patch that specifically looks for core: 9.x.

@Wim Leers I tried that. See #4. I think the problem with that approach is that we don't do anything like this is in Drupal 8 so you can have any value in core: and it will not give a you fatal error.

I thought it would be disruptive when upgrading from Drupal 8 to Drupal 9 because that means if you have anything besides core: 8.x you site would have a fatal error that would also effect drush(and drupal console I think). So you couldn't even rebuild cache. Maybe that is unlikely. For contrib projects that packaging on drupal.org will add core: 8.x but only for top level modules. I know when I was working on related issues earlier I found that at least 1 contrib project that had a bad value for core: in submodule. In Drupal 8 that would simply mark the module as incompatible. If throw a parsing error on this level you couldn't do anything.

I will check if there are many of these. If there aren't then maybe that wouldn't be a problem

(The current code in principle allows 10.x.)

Actually the current code

if (!preg_match("/^\d\.x$/", $parsed_info['core'])) {
   throw new InfoParserException("Invalid 'core' value \"{$parsed_info['core']}\" in " . $filename);
}

stop 10.x because it doesn't allow 2 digits. But don't know if that was actually the intention.

But even if we did could do 10.x the module wouldn't be compatible because only core_version_requirement is used to check compatiblity in the any either version of the patch here.

My concern in #11 with @Wim Leers' ask in #10

It should only allow 8.x. That'd mean that 9.x is not allowed nor 10.x. (The current code in principle allows 10.x.)

was that this would cause fatal errors in that didn't happen in Drupal 8. The problem is you can anything core: for a module in Drupal 8 it won't throw an exception you just can't enable the module.

I did some searching in contrib and I don't that will be problem. I had an automated script pull tons of contrib modules and only found a couple of cases. I can do the search again if we need more info. Of course with custom code we can't know.

Basically if a site has bad values in core: that they were just ignoring Drupal because they didn't enable the module they will have to either update the values or remove the modules or Drupal 9 will throw a hard exception.

I know think that is unlikely and the parser will give them instructions if that does happen.

Here is patch that only excepts core: 8.x

This should fix the test fail

@xjm thanks for the review

re #16

1. No I got this part wrong.
   This should have still been an else. We want to always throw an error if core_version_requirement is not set.

   but since we introduced the core_version_requirement key core has been invalid in cases such as core_version_requirement: ^8.8 or core_version_requirement: ^8.8 || ^9. In those cases you can't specify core: 8.x or any core value at all because other any version of core before 8.7.7 would still let you enable the module, because it doesn't know about core_version_requirement.

   @see the 9.0.x test \Drupal\Tests\Core\Extension\InfoParserUnitTest::testCoreCoreVersionRequirement88()

2. No since this is the 9.x branch we are stilling making them incompatible.
   It is just with this issue I am following @Wim Leers suggestion in #10.

   It should only allow 8.x. That'd mean that 9.x is not allowed nor 10.x.

   So we can't leave the test modules with core: 5.x or the parser would throw an exception.

re #18
This is already the case without this patch. The core not just optional you have to remove the core key if you have core_version_requirement: ^8.8 || ^9 see my point 1) above

1. fixes #16.1
2. Add more test cases to \Drupal\Tests\system\Functional\Form\ModulesListFormWebTest::testModulesListFormWithInvalidInfoFile() which was existing test but only had 1 case for a broken info.yml file. We have InfoParserUnitTest But I thought it would be better to also a more functional test cases.

Test fails and self review

1. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
   @@ -56,26 +56,34 @@ public function parse($filename) {
   +        if ($is_testing_package) {
   +          // Modules in the testing package are exempt as well. This makes it
   +          // easier for contrib to use test modules. We only do not set
   +          // 'core_version_requirement' to allow testing incompatible moodules.
   +          if (!isset($parsed_info['core'])) {
   +            $parsed_info['core_version_requirement'] = \Drupal::VERSION;
   +          }
   +        }
   

   I moved this check first but didn't delete the other copy further down. deleting.
   Also misspelling "moodules"

2. \Drupal\Tests\system\Functional\UpdateSystem\UpdateScriptTest::testExtensionCompatibilityChange()
   which I just helped add in #2917600: update_fix_compatibility() puts sites into unrecoverable state failed because it was relying on changing to core: 7.x to produce a message on the update page.

   Since the parser would now throw an exception for anything other than core: 8.x this has to be removed.
   So we can't rely on that change compatibility. I removed the 2 test cases that relied on this.

   I think it is ok to remove this test cases because not longer can make a module be set as core_incompatible with changes to the core key.

3. SingleVisibleProfileTest failed because it made an info file with
   'core' => \Drupal::CORE_COMPATIBILITY,
   This would need core_version_requirment now.

   I changed to
   'core_version_requirement' => '^8 || ^9 || ^10',
   I could have use
   'core_version_requirement' => '*',
   but I am not sure this good example of what we want tests to use. Some test info files are already using. It has the advantage of never needing to be update. But it also shows a bad example if anyone learning to make modules ever sees this code. I think we need issue to decide what we want to use in these cases.

#21 thanks for the review

1. I update the comment. The intention was to let test module and test modules only to not have core_version_requirement if the have core set. To allow testing error. I updated the comment

   I don't think this is needed now but not changing this right now so I can have patch where that is the only difference to see what that change alone breaks.

2. fixed

#22
@jungle thanks for pointing these out. Fixed

Here is patch that removes the section of code mentioned in #21.1

If #23 passes and this doesn't then it is still needed for test modules. I don't think it is. I think this was just put in when we specifically didn't allow 9.x and now we only allow 7.x

working on test failures

Hopefully this fixes tests. Will comment on patch to explain fixes.

+++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
@@ -87,7 +85,7 @@ public function parse($filename) {
-        if (!$supports_pre_core_version_requirement_version && isset($parsed_info['core'])) {
+        if (!$is_testing_package && !$supports_pre_core_version_requirement_version && isset($parsed_info['core'])) {

So this makes an exception for the testing package to not throw an error if core should not be set.

So right I am reverting more changes in test modules info.yml files where I removed core

But I am not actually sure we want to exempt test modules in this way. will address that in next patch

Comment for #32

So as an alternative to #31 we could actually just throw an exception for test module like we do for all other modules when core should not be set at all.

This would mean we would have remove the core: 8.x from the 6 test modules that have this.

This is because the test modules will automatically get core_version_requirement: 9.0.0-dev set(current behavior this patch doesn't change) so core: 8.x is not valid.

I think this is better unless other have reasons we should treat testing modules different.

I think #31 was unrelated JS test fail. Retesting.

Assigning to myself for a self review and probably more test coverage. I already see a couple improvements to make so probably makes sense to wait for someone else to review till my next patch.

1. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
   @@ -56,26 +56,23 @@ public function parse($filename) {
   -      if (!isset($parsed_info['core']) && !isset($parsed_info['core_version_requirement'])) {
   -        if (strpos($filename, 'core/') === 0 || strpos($filename, $this->root . '/core/') === 0) {
   +      if (!isset($parsed_info['core_version_requirement'])) {
   +        if (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;
   +        }
   +        elseif (strpos($filename, 'core/') === 0 || strpos($filename, $this->root . '/core/') === 0) {
   ...
   -        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;
   -        }
   

   I changed the order of evaluation earlier in the patch. I don't think it is necessary any more. Reverting.

2. +++ b/core/modules/system/tests/modules/database_statement_monitoring_test/database_statement_monitoring_test.info.yml
   @@ -1,6 +1,5 @@
   -core: 8.x
   

   These changes to test files are because they will cause an error when the parser automatically sets core_version_requirement: 9.0.0-dev which doesn't allow 8.x to be set.

3. +++ b/core/modules/system/tests/modules/system_incompatible_core_version_dependencies_test/system_incompatible_core_version_dependencies_test.info.yml
   @@ -4,4 +4,4 @@ description: 'Support module for testing system dependencies.'
   -  - drupal:system_incompatible_core_version_test
   +  - drupal:system_core_incompatible_semver_test
   diff --git a/core/modules/system/tests/modules/system_incompatible_core_version_test/system_incompatible_core_version_test.info.yml b/core/modules/system/tests/modules/system_incompatible_core_version_test/system_incompatible_core_version_test.info.yml
   
   diff --git a/core/modules/system/tests/modules/system_incompatible_core_version_test/system_incompatible_core_version_test.info.yml b/core/modules/system/tests/modules/system_incompatible_core_version_test/system_incompatible_core_version_test.info.yml
   deleted file mode 100644
   

   Deleted this test file and related test cases because we can no longer test a file with core: 8.x and no core_version_requirement set.

   In this patch I am adding explicit unit test coverage for this in \Drupal\Tests\Core\Extension\InfoParserUnitTest::testCoreWithoutCoreVersionRequirement()

   I have also added \Drupal\Tests\Core\Extension\InfoParserUnitTest::testCore8x()

   which tests core: 8.x with various values of core_version_requirement where should be allowed.

4. +++ b/core/modules/system/tests/src/Functional/System/ThemeTest.php
   @@ -376,7 +376,6 @@ public function testInvalidTheme() {
   -    $this->assertThemeIncompatibleText('Theme test with invalid core version', $incompatible_core_message);
        $this->assertThemeIncompatibleText('Theme test with invalid semver core version', $incompatible_core_message);
   

   Again here a theme with just core: 8.x and no core_version_requirement won't parse. So can't do the functional tests but we can do the unit tests added in this patch.

   Since we are stilling doing the functional test with module with core_version_requirement set to an invalid value we still have function test coverage for a theme having core_incompatible set to TRUE in the parser and it stopping the theme from being enabled.

Added release no snippet

Added the change record https://www.drupal.org/node/3119415

Exactly the same as the release snippet for now. I don't think this needs a long explanation. Both link to the original change record for the core_version_requirement key.

@Gábor Hojtsy good idea!
Ok added more info to that exception

Just forgot to update the expected message in \Drupal\Tests\system\Functional\Form\ModulesListFormWebTest::testModulesListFormWithInvalidInfoFile()

@dww thanks for the review

1. fixed
2. This because we fail on the first error. The next test case shows that we check core: 9.x value

   I think it is worth just failing on 1 error at a time not complicating the parser class by checking for all errors and outputing list. These should really only be seen by module developers so they can fix see the next error and fix that. Since it fails hard it will be hard to miss as they develop/port their modules.

   Added a comment to the test.

3. added unique name to each case
4. whoops! fixed
5. yep, I guess the assert explains itself.
6. The existing test has this when it was only dealing with 1 yaml file. I didn't want to take an existing assert away.

   This confirm in the first case that even though the error is shown the filter text is also shown.
   This is actually pretty weird. Why do you want the filter item to be shown if there aren't any modules list? The test was added as is in #2558645: Malformed module.info.yml prevents install with a confusing error

   For the 2nd time it is called I thought it was better to have the same assert because the only difference on the 2nd time should be that the error is not shown.

   Unless that explanation gives you or someone else an idea as to why it is there I will file a follow-up. I will also read the original issue and see why they added it.

   I don't think we should remove an existing assert even if the test has been refactored a bunch.

7. fixed
8. Not clear what you are asking for. it can't be just <<. that throws an error. Or just shorter label than CORE_WITHOUT_CORE_VERSION_REQUIREMENT ?

   Not addressing but as you left as RTBC assuming if this doesn't get addressed its not a big deal.

9. You maybe correct but there 4 of these in the existing file so I don't think having 1 be different is any clearer.

   But also this is just the way double quoted strings work, right?
   Not changing for now.

Re #45.8 an .9

I realized that the way \Drupal\Tests\Core\Extension\InfoParserUnitTest is set up now it make adding test cases for parser exceptions. We should actually need any new test methods each time.

I think it should maybe refactored.
We have a bunch of functions that expect an error based on different info.yml files. I think we could just 1 test method for all these with just have a dataProvider

// Tests that 'core_version_requirement: ^8.8' is invalid with a 'core'
      // key.
      'testCoreCoreVersionRequirement88' => [
        'yml' => <<<YML
package: Core
core: 8.x
core_version_requirement: ^8.8
version: VERSION
type: module
name: Form auto submitter
dependencies:
  - field
YML,
        'expected_exception' => "The 'core_version_requirement' constraint (^8.8) requires the 'core' key not be set in",
      ],

So we could get rid of 8 methods and replace it with 1.

I created this follow-up #3119761: Replace multiple test methods in InfoParserUnitTest with 1 testInfoException and a dataprovider

there is the start of a patch there. It should be much simpler and have the same(actually expanded) test coverage.