/admin/reports/status should not WSOD if it finds an extension missing core_version_requirement in its info.yml file

At least the other way round (trying a module that needs at least 8.7.7 on a lower version) is by design, maybe this could be handled better.

This is a side effect of a recent issue to verify that all installed modules should be compatible but it should maybe not fail on those that aren't installed?

So this behavior is by design. That said, it's worth discussing whether there are downsides to that choice. Marking as a task to discuss whether to change how this is handled since it's not 100% a bug, but promoting to major and marking as a beta target given the impact and importance.

So this by design but the title here is not completely accurate.

/admin/reports/status should not WSOD if it finds an incompatible module

It is not that it found an incompatible module but the key that would tell Drupal whether it is compatible or not was not found.

this was done intentionally in #3105701: Do not allow core: 9.x in info.yml files

So you get a similar error as if you missed the type or name key.

An incompatible module would be an info file with core_version_requirement: ^8.8 or core_version_requirement: ^9.1

We could change this because we don't the bug in #2917600: update_fix_compatibility() puts sites into unrecoverable state. Then if core_version_requirement is missing we could just set core_incompatible = FALSE like we do if we have core_version_requirement: ^8.8.

There's also a related problem that won't be easy to fix because we would need to backport to 8.7.

page_manager requires 8.8 and removed the core requirement key from test modules. metatag still supports 8.7 but has require-dev for page_manager, that breaks completely on 8.7 even though only a single test actually needs to install page_manager: https://www.drupal.org/pift-ci-job/1633485

1. Re #7. I think we should still throw a hard exception if we have core: 9.x because that is very rare and if we throw an error then module developers won't make this mistake.
2. @DamienMcKenna as far as the site building experience, is the use case that someone has updated core to 9.0.x but not ensured that all their contrib modules are D9 compatible first? If they have made sure their modules are updated to D9 compatible versions they won't hit this problem because all their modules would have core_version_requirement.
3. In https://www.drupal.org/docs/9/how-to-prepare-your-drupal-7-or-8-site-for...

   We recommend making sure your modules are compatible before you update core to D9(I know not everybody will find/follow these instructions)

4. You only get the WSOD current if you don't have error reporting set to "None" correct?
5. 1 problem I see if we don't throw the hard exception is we would have effectively introduced a new way to upgrade different from the recommended way in the documentation above. Though this would not be intentional.

   Here is what I mean. If someone a wrote blog post with these instructions:

   1. Ensure your hosting environment matches the platform requirements of Drupal 9.
   2. Update to Drupal 8.8x or 8.9.x (if not already on that version)
   3. Update core codebase to Drupal 9
   4. Visit update.php. This will inform you of which modules are compatible with Drupal 9.
   5. Update all contributed projects and ensure they are Drupal 9 compatible
   6. Make custom code Drupal 9 compatible
   7. Re-visit: update.php: If you no modules/themes that remain that are D9 incompatible you will able to run DB updates and have successfully updated.

   although the above instructions I don't think would be recommended, with change I suggested is possible(not necessarily recommended) in #6 and the changes we made to add error messages for incompatible modules in #2917600: update_fix_compatibility() puts sites into unrecoverable state it would be possible.

   Technically this would be possible already if all of your modules had at least core_version_requirment: ^8.8 but I think that would be an unlikely scenario.

   My point is, I think we should be very careful about a change like this and think about the unintended effects. It might still be worth making but it will have other effects.

   If we don't make the change here and tried to the steps I outlined above in step 4 "Visit update.php" then you would get the hard exception about core_version_requirement being missing, you see this if error reporting are on.

   If we don't make this change it is much more convenient to upgrade your modules before you update core.

   If we make this change it may actually be more convenient to upgrade your modules after you update core.

   If it is more convenient even if we document that you shouldn't do this it may become the default way people upgrade.
   There might be other effects I am not thinking of also.

@DamienMcKenna as far as the site building experience, is the use case that someone has updated core to 9.0.x but not ensured that all their contrib modules are D9 compatible first?

Another way to trigger this would presumably be installing 9.0.0 and downloading a contrib module that's outdated (or is it different for uninstalled modules?), or downgrading an installed module for some reason.

1 problem I see if we don't throw the hard exception is we would have effectively introduced a new way to upgrade different from the recommended way in the documentation above.

I think if we want to explicitly prevent people from doing this, we should introduce a check in system_requirements() update to check all installed extensions and that they're compatible prior to allowing updates to be run, similar to what we did in #3098475: Add more strict checking of hook_update_last_removed() and better explanation.

Can we "relax" the error at least for uninstalled modules?

I see the WSOD on all pages of the site, not only on the status report. Also drush cr can't complete because of the exception.

The white screen has only a "The website encountered an unexpected error. Please try again later." message.

Error reporting is set to "None" at /admin/config/development/logging and not overridden anywhere - this is a new clean installation (from web ui) of the latest D9-dev with the standard profile. But PHP's error_reporting = E_ALL.

re #10

I think if we want to explicitly prevent people from doing this, we should introduce a check in system_requirements() update to check all installed extensions and that they're compatible prior to allowing updates to be run,

We actually did this in #2917600: update_fix_compatibility() puts sites into unrecoverable state
But as I said #6

It is not that it found an incompatible module but the key that would tell Drupal whether it is compatible or not was not found.

re

(or is it different for uninstalled modules?)

No if core_version_requirement is not found it doesn't matter if the module is uninstalled or not it will throw this hard error.

I working on patch.

Basically
In \Drupal\Core\Extension\InfoParserDynamic::parse() throw hard error if

1. core is set to anything but core: 8.x. This is existing behavior
2. If neither core or core_version_requirment is set. Exception recommends setting core_version_requirment, this exception message is existing. The only difference would be that if core: 8.x is set there would be no exception
3. Leave unchanged
   

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

   core: 8.x will evaluate to $parsed_info['core_incompatible'] === TRUE.

Then the existing logic in system_requirements() that we added #2917600: update_fix_compatibility() puts sites into unrecoverable state will stop updates if you just have core: 8.x and core_version_requirement is not set. It will display an error message and link to Troubleshooting database updates. There are no changes needed for this.

@pingwin4eg Maybe your error reporting is not set at /admin/config/development/logging to show errors and warnings?

Edit: Or you need to add this to settings.php since you probably can't use the UI right now:

$config['system.logging']['error_level'] = 'verbose';

So there are two different audiences here:

1. A developer who is trying to upgrade a module to Drupal 9, or write a new module on Drupal 9.
2. A site owner who is trying to upgrade to Drupal 9 but has not yet visited update.php, or who has already tried to upgrade but has incompatible modules in the codebase for whatever other reason. This might happen particularly if installed modules were all updated to D9-ready versions, but uninstalled modules were not.

For the module developer, they need the specific, clear exception in order to fix the module. If the module is installed but is not D9-compatible, everything is broken and we should not allow the site to run until the module is fixed, because this could lead to all sorts of data integrity problems. If the module's not installed, the current behavior is also helpful to them. (The alternative would be that the module would just silently not show up in the module list, without them getting any debugging information as to why.)

For the site owner, the specific error message is not helpful to them. What they need to know is that they have a module that's not compatible with D9 in the codebase, so they need to either get rid of it or go back to their D8 codebase. We still should be failing hard if the module is installed, because again, data integrity problems etc. However, if the module's not installed, they probably don't want their site on fire for that.

So the question is how to bridge those two audiences for the two respective scenarios.

Ok here is a patch I worked on this before I chatted with @xjm

It implements what suggested in #14

Re #16

I wasn't expecting to get a WSOD as a few weeks previously it had worked fine and just blocked my ability to install incompatible modules.

It would solve this problem. You would not be able to install the modules. You would see this message

If you went to update.php or admin/reports/status and you had any of these modules enabled(b/c they previously had a valid core_version_requirement then you would see message about the modules being installed but incompatible
You can see the message screenshots in the summary of #2917600: update_fix_compatibility() puts sites into unrecoverable state

If you went to update.php or admin/reports/status and you had any of these modules enabled(b/c they previously had a valid core_version_requirement then you would see message about the modules being installed but incompatible

But that message would only show up on the status report or update.php, right? What happens on the extension page in that scenario? The site is effectively broken when incompatible modules are enabled, including ones missing the core_version_requirement key.

+++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
@@ -69,9 +69,12 @@ public function parse($filename) {
-          // Non-core extensions must specify core compatibility.
-          throw new InfoParserException("The 'core_version_requirement' key must be present in " . $filename);
+          if (!isset($parsed_info['core'])) {
+            // Non-core extensions must specify core compatibility.
+            throw new InfoParserException("The 'core_version_requirement' key must be present in " . $filename);
+          }

So the effect from this is that if core: 8.x is present but core_version_requirement isn't, the error message for the developer is no longer displayed anywhere. So how does the developer then get the information that that key is missing and that's why the module's not compatible?

> So how does the developer then get the information that that key is missing and that's why the module's not compatible?

They won't be able to install the module?

re #16

Yes they wouldn't see anything on the extension page.
But also this is true for modules that are enabled and have core_version_requirement: ^8.8. Since we added core_version_requirement in 8.7.7 it probably means there are tons of modules that are using it to denote compatibility with specific versions of 8. So now in 9.0.x they would see no message about these modules except if they visit update.php or the status report.

We have related existing problems such as a module could have been updated and the new version has higher requirement for php. If they don't go to update.php or status report they won't know that.

I could think of a few ways to solve this problem.

1. Check on the modules page if any of the modules are enabled but where core_incompatible === TRUE. Add a message with a link to the status report where they would see specific modules
2. On the modules page call \Drupal\system\SystemManager::listRequirements() and check if there requirements we set in system_requirements() for incompatible modules, php version or missing modules are set. If so message and link to status report
3. Add a new possible key to hook_requirements(), requirements_context. If any requirements set \Drupal\system\SystemManager::REQUIREMENT_CONTEXT_MODULES then show a message with a link to the status report.
   Update system_requirement() to add this context for incompatible modules, missing modules or php version problems.

We could the same with themes on the themes page

Here are 3 diffs from #17 for these ideas. Not including the full patches because theren't test anyways and they are rough ideas

Either the scope of this issue needs to change to not just be about /admin/reports/status or we should open a follow-up to discuss what appears on the extension page. I'll leave that decision to the scope wranglers. I basically never get it right.

That said, the module page now has the 'Compatible' vs. 'Not compatible' details element on each project. So if we want to put more stuff on the extension page about incompatibilities such as PHP version and whatnot, we have plumbing in place to handle it.
See #23.

Sorry, brain fart. It's the available updates report that has the Compatible vs. Not compatible details I was thinking of. The modules page already has a collapsed details element for each project, and that already has logic to show you things like PHP incompatibility. I crossed my wires. I had meant to potentially start having the available updates report show us more of those kinds of compatibility problems, inspired by the checks we already have on the modules (extend) page...

1. #22 and the scope.

   Currently on many pages will WSOD because of this error if you clear cache or if the page for some reason resets the extension caches so I think this issue should handle the problem.

   So if you drush cr or visit admin/config/development/performance and clear caches then many will stop working on all pages after that.

   I assume we want to fix that and not just on the /admin/report/status page.

   Updating the Issue Summary to reflect the problem is on

2. Re #23

   The modules page already has a collapsed details element for each project, and that already has logic to show you things like PHP incompatibility.

   With the current problem where a module is missing core_requirement, for both installed and uninstalled modules you won't actually see this message because the form won't load.

3. Look at my 3 ideas in #21 built on top of #17 I think option 2) is the best way to fix this.

   Don't throw the exception in \Drupal\Core\Extension\InfoParserDynamic::parse() if core_version_requirements is missing. Instead put an error on /admin/modules if we find a module that is not compatible and is enabled. Any module without core_version_requirement will be incompatible but also any module that has core_version_requirement that is not a compatible constraint. In error we link to the admin/report/status so they can see the list of modules that aren't compatible.

   We should do the same with /admin/appearance

Here is patch that

1. Doesn't throw an exception in \Drupal\Core\Extension\InfoParserDynamic::parse() if core_version_requirement is not set.
2. Adds a message and a link to status page on /admin/modules if a module is enabled but not compatible
3. Adds a message and a link to status page on /admin/appearance if a theme is enabled but not compatible

Practical effects this has

1. You can visit /admin/report/status, admin/appearance, and admin/modules and you will not throw an error but will get a message to view /admin/report/status
2. /admin/report/status will show all modules that aren't compatible(for missing core_version_requirement or other reasons). These messages are existing
3. update.php shows existing messages about incompatible modules without crashing
4. Modules and themes that are not installed will show no error for missing core_version_requirement on module/theme listing page, as long as they have core: 8.x

   but they won't be able to be enable and they will have a message that says they aren't compatible. This message is not new.

5. modules with <code>core set to anything other than 8.x will still throw an error

I like where this is headed, thanks! A few concerns:

1. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
   @@ -69,9 +69,12 @@ public function parse($filename) {
            else {
   -          // Non-core extensions must specify core compatibility.
   -          throw new InfoParserException("The 'core_version_requirement' key must be present in " . $filename);
   +          if (!isset($parsed_info['core'])) {
   +            // Non-core extensions must specify core compatibility.
   +            throw new InfoParserException("The 'core_version_requirement' key must be present in " . $filename);
   +          }
            }
   

   Shouldn't this become elseif() instead of else { if() } ?

2. +++ b/core/modules/system/src/Controller/SystemController.php
   @@ -209,11 +209,19 @@ public function themesPage() {
   +        $this->messenger()->addWarning(t(
   
   +++ b/core/modules/system/src/Form/ModulesListForm.php
   @@ -171,12 +171,20 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +        $this->messenger()->addWarning(t(
   

   $this->t(), right?

3. +++ b/core/modules/system/src/Controller/SystemController.php
   @@ -209,11 +209,19 @@ public function themesPage() {
   +          'There are errors with your themes please visit the <a href=":link">status report page</a>',
   

   A) Needs punctuation after 'themes'. Either a comma, or perhaps a whole new sentence.

   B) The whole thing should end with a period.

   C) I think I remember some UI guidelines for core that we don't say "Please". ;) Maybe:

   "There are errors with your themes. Visit the {link} to learn more."

   (or something).

4. +++ b/core/modules/system/src/Form/ModulesListForm.php
   @@ -171,12 +171,20 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +          'There are errors with your modules please visit the <a href=":link">status report page</a>',
   

   This also needs help, as above.

5. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdateScriptTest.php
   @@ -323,6 +323,28 @@ public function providerExtensionCompatibilityChange() {
   +      'module: core_version_requirement key missing' => [
   +        [
   +          'core_version_requirement' => '^8 || ^9',
   +          'type' => 'module',
   +        ],
   +        [
   +          'core' => '8.x',
   +          'type' => 'module',
   +        ],
   +        $incompatible_module_message,
   +      ],
   

   Slightly confusing since it looks like the first array has core_version_requirement. I should look at the whole test in context. Probably nothing to fix here, just flagging it. ;)

6. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
   @@ -663,4 +662,33 @@ public function testUnparsableCoreVersionRequirement() {
   +   * Tests that 'core: 8.x' with no 'core_version_requirement' key.
   

   This summary doesn't parse. Missing some words at the end (or something).

Since this is RC-blocking, and I don't want to hold up progress, this fixes all of #27 except point 5 for which there's nothing to fix. ;)

I didn't see @dww patch when did my patch here. My patch is the same except for slight different wording for #27.3 where I add "installed" because it is only if the extension is installed and .6. Also added additional test coverage
@dww thanks for the review

1. fixed
2. fixed
3. fixed
   Used

   There are errors with some installed themes. Visit the status report page for more information.

4. fixed
5. This tests installing a extension that is compatible and then updating the module to an incompatible version. Since this patch introduces a new way for an extension to be marked incompatible with core, having core: 8.x and no core_version_requirement we should have this new test case.
6. fixed

Also add test coverage for the message on the module and theme admin pages.

Oh well, bummer for duplicated effort. Glad you arrived at almost identical changes. ;)

However, that interdiff doesn't look right. That's only including the new tests. Here's the actual interdiff between #25 and #29.

Also attaching an interdiff between #28 and #29 FWIW.

Cheers,
-Derek

Random fail:

There was 1 error:

1) Drupal\Tests\field_layout\FunctionalJavascript\FieldLayoutTest::testEntityForm
WebDriver\Exception\CurlExec: Curl error thrown for http POST to http://chromedriver-jenkins-drupal-patches-44445:9515/session/4bd399f065f40dfbf2035b687f33c3c9/execute with params: {"script":"(function (element) {\n    var event = document.createEvent(\"HTMLEvents\");\n\n    event.initEvent(\"drop\", true, true);\n    event.dataTransfer = {};\n\n    element.dispatchEvent(event);\n}(arguments[0]));","args":[{"ELEMENT":"0.9328211445737633-9"}]}

Requeued.

At first it seemed suspect to not have an else case anymore, but when looked at in the larger context of the rest of the function, it makes sense.

Reread all the issue comments, worked through the coverage locally, and everything checks out. Thanks to both @dww and @tedbow for pushing this one through!

I happened to have the layout_library module sitting in a local repo's modules/ for I guess the past two years. Tried to install Drupal, got:

[ibnsina:d8git | Sat 15:39:01] $ quickstart

In InfoParserDynamic.php line 73:
                                                                               
  The 'core_version_requirement' key must be present in modules/layout_librar  
  y/layout_library.info.yml                                                    

That seemed as good an opportunity as any to manually test this change (and and a good example of why we should make it). I applied #29 and re-ran the quickstart command, and Drupal installed. So that's some manual testing confirmation.

1. +++ b/core/modules/system/tests/src/Functional/Form/ModulesListFormWebTest.php
   @@ -123,4 +122,56 @@ public function testRequiredByThemeMessage() {
   +    $incompatitable_modules_message = 'There are errors with some installed modules. Visit the status report page for more information.';
   ...
   +      $this->drupalGet('admin/modules');
   

   nit: incompatitable.

2. +++ b/core/modules/system/tests/src/Functional/Form/ModulesListFormWebTest.php
   @@ -123,4 +122,56 @@ public function testRequiredByThemeMessage() {
   +      $this->drupalGet('admin/modules');
   +      $this->assertText($incompatitable_modules_message);
   +
   +      file_put_contents($file_path, Yaml::encode($compatible_info));
   +      $this->drupalGet('admin/modules');
   +      $this->assertNoText($incompatitable_modules_message);
   +    }
   

   It's consistent so just a typo, fixable on commit.

#35
🤦‍♂️

I joked that in #2313917: Core version key in module's .info.yml doesn't respect core semantic versioning that at least I would learn to spell incompatible.

Today I learned that at some point I accidentally added "incompatitable" to my phpstorm application dictionary 😥

Re: #35 / #36: Heh, whoops. I think "incompatitable" is in my mental dictionary now, too. ;) Sorry I missed it earlier.

+1 that #36 is still RTBC. Interdiff looks good. Bot is happy.

Thanks/sorry,
-Derek

Committed/pushed to 9.1.x and cherry-picked to 9.0.x, thanks!

Are we backporting this to 8.9.x? The error is not likely to happen on D8, but we should keep the logic the same.

@xjm, This is the patch for 8.9

Patch fails the testbot.

Unfortunately we missed this against 8.9.x (since new APIs can't be added after the release). It's in 9.0.x though which is the critical part. Thanks!

I typed "new APIs" in #46 because I was thinking about the statement I made earlier that 8.9 and 9.0 are supposed to have the same API and behavior. The issue is a behavior change as well as string addition, which makes it minor-only by default, and the actual WSOD is not likely to happen on D8, which reduces the priority for the backport anyway.

Our policy in general for the production branch is "if in doubt, don't backport" since there's statistically always a chance that a new fix or improvement will introduce a different regression, and our first priority is the stability of the production branch.