Malformed module.info.yml prevents install with a confusing error

I see this if I do not supply 'core' key -

The website encountered an unexpected error. Please try again later.

Drupal\Core\Extension\InfoParserException: Missing required keys (core) in modules/pp/pp.info.yml. in Drupal\Core\Extension\InfoParser->parse() (line 45 of core/lib/Drupal/Core/Extension/InfoParser.php).

#3 seems pretty clear to me if we are just talking about the 'core' key.

@david_garcia: Could you add an example malformed yml to the issue summary please?

This may need a title update and a refocus onto simply the malformed aspect, if incomplete files are handled with an appropriate error.

Thanks for posting the full text of the error... I had the same problem after copying the standard profile and simply removing the d.o packaging info. I would have never traced it down without being able to Google it.

@Dane Powell I added that info to the issue summary.

I wanted to raise a similar issue, but found this after a search. Right now the admin/modules page goes WSOD with no useful message if a module happens to miss one of the required .info.yml keys (e.g. core: 8.x).

I think a better UX would be to display this error on the UI rather than crashing the whole admin->extend interface.

@almaduoh made a good suggestion in #2730807: WSOD on admin/modules if description is set but is NULL in module.info.yml:

I think a more generic solution for all .info.yml files where required keys are missing would be to throw a MissingRequiredInfoKeyException (or some such) which could be caught and properly displayed on the UI.

I think that is the correct solution when the key is required.

So both modules and themes have a default array to merge to the information in .info.yml - see _system_rebuild_module_data() and \Drupal\Core\Extension\ThemeHandler::rebuildThemeData(). I think it is worth considering that if the yaml value is NULL we should use the default. That would fix both #2730807: WSOD on admin/modules if description is set but is NULL in module.info.yml and #2594937: Empty 'libraries:' in theme .info.yml file produces confusing PHP warning

I;ve moved this comment to the related issues. For this specific issue throwing an exception is exactly what we want to do. We just need to catch it in the UI.

FWIW this also affects malformed info.yml files in install profiles.

Here's a patch with test and fix.

I think the fix in #15 is appropriate, and has a great test! I've re-queued for testing, but marking RTBC assuming it comes back green.

Needs a reroll :(

Rerolled

Thanks @almaudoh! Back to RTBC.

+++ b/core/modules/system/src/Form/ModulesListForm.php
@@ -143,8 +144,14 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+      $this->messenger()->addMessage($this->t('Modules could not be listed due to an error: "%error"', ['%error' => $e->getMessage()]), 'error');

Oops should have noticed... this ought to be ->addError() - it means there are less hardcoded strings about.

Oops! Missed that.

Really sorry for the review in drips and drabs.

+++ b/core/modules/system/src/Form/ModulesListForm.php
@@ -143,8 +144,14 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+      $this->messenger()->addError($this->t('Modules could not be listed due to an error: "%error"', ['%error' => $e->getMessage()]));

quoting the error message is probably not necessary because there is the colon. The % will format with a em in the UI. There is precedence of message: %error in \Drupal\Core\Database\Install\Tasks::$tasks.

Also the message should be in the present. I.e.
Modules can not be listed due to an error: %error

Added a patch with a above mentioned change. Removed the quotes from error message.

But still confused because $this->t is a function so if we remove quotes from message then how this be recognized as a string? May be my patch is not correct in terms of this.

@Vidushi Mehta I meant to remove the double quotes from around %error. Ie. the final code should look like:
$this->messenger()->addError($this->t('Modules can not be listed due to an error: %error', [%error => $e->getMessage()]));

+++ b/core/modules/system/tests/src/Functional/Form/ModulesListFormWebTest.php
@@ -51,4 +51,29 @@ public function testModuleListForm() {
+      ->pageTextContains('Modules could not be listed due to an error: "Missing required keys (core) in ' . $path . '/broken.info.yml"');

Note you also need to update the assertion text.

That's what I was thinking. Thanks @alexpott for making me correct I was also in doubt.
Added a correct patch with the changes.

+++ b/core/modules/system/tests/src/Functional/Form/ModulesListFormWebTest.php
@@ -70,7 +70,7 @@
+      ->pageTextContains('Modules can not be listed due to an error: "Missing required keys (core) in ' . $path . '/broken.info.yml"');

Need to remove the double quotes :)

+++ b/core/modules/system/src/Form/ModulesListForm.php
@@ -141,8 +142,14 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+      $this->messenger()->addError($this->t('Modules can not be listed due to an error: %error', [%error => $e->getMessage()]));

Should be $this->messenger()->addError($this->t('Modules can not be listed due to an error: %error', ['%error' => $e->getMessage()]));

@Vidushi Mehta it's a really good idea to test your code locally before uploading a patch. Also to learn how to run the PHPUnit test too. See https://www.drupal.org/docs/8/phpunit/running-phpunit-tests

I dont know what's going wrong. I've tested it on my local and patch is applying properly.
Added another with checking it on locally. @alexpott Thanks for making me correct again :) Hope this time it works.

interdiff should be in .txt format.

Thanks @Vidushi Mehta!

Rerolled patch. Left RTBC.

Adding review credits

Committed as 2be3c89 and pushed to 8.7.x.

Cherry-picked as e59e83e and pushed to 8.6.x.