Malformed modulename.info.yml file causes fatal error

Moved to profile.module per #2050763-16: Refine "base system" component (notes on refactoring of "base system" category here: https://docs.google.com/a/acquia.com/spreadsheet/ccc?key=0AusehVccVSq2dF...).

Hm. This definitely isn't about profile.module. Not sure where else to put it than base system.

If you can download such a module, you are dealing with a maintainer who pushed something that doesn't even work on his local machine then ...
Sure, it's annoying, but I fail to see why this would be major, other than a more friendly message - which already does a good job of indicating where something goes wrong.

(note, I hardly even see this as a bug: wrong format, you get an exception, done. Just like you get an PHP error when you type something wrong in your module file.)

Right, still not convinced, but I won't fight it over as it's an easy 'fix'. :)

With that, maybe only one nitpick/question:

+++ b/core/includes/common.incundefined
@@ -4572,9 +4572,17 @@ function drupal_parse_info_file($filename) {
+        drupal_set_message(t('Could not parse metadata in %name. Removing this extension is highly recommended.', array('%name' => $filename)), 'error');

The previous exception contained the line on which it failed, should we try and see whether $exception contains that or not ?

FWIW, given that a hapless end user can screw their entire site without even enabling a faulty module but just having it in the file tree, I'm in favour of doing some hardening around this. It's properly categorized as a "major" issue, though; we could certainly ship without it.

I wonder though if this check is in the proper place, or if it should be moved further up the chain to apply more generally to YAML parse faults. It seems like a = instead of a : shouldn't WSOD your site, no matter where it's coming from.

However if there is a way to catch where the problem is and pass it along to the developer (or any other interested parties) it sure would be nice. Catching the problem early on before it has a chance to break something means that we don't get surprised when we discover (or create) another nasty path.

Crossposting: #2116377: Make .info.yml syntax errors tell you where they come from; also non-required modules shouldn't break the installer

I think it is correct that a malformed info.yml file creates a fatal error as we have no idea what to do at this point - should the module still be enabled - what about data integrity etc. So I think we should refocus this issue to instead provide a helpful error message when this occurs.

The patch attached does the following:

• Creates an InfoParser class that throws InfoParserExceptions that detail the filename of the info.yml that is causing the issue
• Validates that the info.yml contains a name and type key
• Creates an info_parser service
• Adds tests for all logic in the class

This patch also fixes #2116377: Make .info.yml syntax errors tell you where they come from; also non-required modules shouldn't break the installer so that the installer also reports a decent error message.

Before this patch if you have a broken .info.yml the installer would display an error like this:

With this patch:

So now at least the user can see where the error is occurring and has a chance of fixing it.

Looks great! Just some weird spacing issues.

1. +++ b/core/lib/Drupal/Core/Extension/InfoParser.php
   @@ -0,0 +1,86 @@
   +class InfoParser implements InfoParserInterface  {
   
   +++ b/core/lib/Drupal/Core/Extension/InfoParserInterface.php
   @@ -0,0 +1,56 @@
   +interface InfoParserInterface  {
   

   Double space before {

2. +++ b/core/lib/Drupal/Core/Extension/InfoParser.php
   @@ -0,0 +1,86 @@
   +  public function parse ($filename) {
   

   Space between parse and (

3. +++ b/core/lib/Drupal/Core/Extension/InfoParser.php
   @@ -0,0 +1,86 @@
   +        if (!empty($missing_keys)) {
   +
   +          $message = format_plural(count($missing_keys), 'Missing required key (!missing_keys) in !file.', 'Missing required keys (!missing_keys) in !file.', array('!missing_keys' => implode(', ', $missing_keys), '!file' => $filename));
   

   Extra blank line

4. +++ b/core/lib/Drupal/Core/Extension/InfoParser.php
   @@ -0,0 +1,86 @@
   +    if (empty($this->parser)) {
   

   empty() could be replaced with !

5. +++ b/core/lib/Drupal/Core/Extension/InfoParserException.php
   @@ -0,0 +1,43 @@
   +class InfoParserException extends \RuntimeException {
   +  /**
   

   Missing blank line

6. +++ b/core/lib/Drupal/Core/Extension/InfoParserInterface.php
   @@ -0,0 +1,56 @@
   +}
   \ No newline at end of file
   diff --git a/core/lib/Drupal/Core/SystemListingInfo.php b/core/lib/Drupal/Core/SystemListingInfo.php
   

   Missing newline

1. +++ b/core/lib/Drupal/Core/Extension/InfoParser.php
   @@ -0,0 +1,85 @@
   +          $message = format_string("Unable to parse !file. Parser error !error.", array('!file' => $filename, '!error' => $e->getMessage()));
   

   We could directly go to String::format

2. +++ b/core/lib/Drupal/Core/Extension/InfoParser.php
   @@ -0,0 +1,85 @@
   +   * @return Parser
   

   This should be the full namespace, sorry this feels so nitpicky.

3. +++ b/core/lib/Drupal/Core/Extension/InfoParser.php
   @@ -0,0 +1,85 @@
   +  protected function getParser() {
   +    if (!$this->parser) {
   +      $this->parser = new Parser();
   +    }
   

   Just wondering, is there a reason we don't have the parser as service and just inject it directly?

4. +++ b/core/lib/Drupal/Core/Extension/InfoParserInterface.php
   @@ -0,0 +1,57 @@
   +   * Parses Drupal module and theme .info.yml files.
   

   I know this is out of scope but theoretically we also use it for profile .info.yml files.

5. +++ b/core/lib/Drupal/Core/Extension/InfoParserInterface.php
   @@ -0,0 +1,57 @@
   +   * @throw \Drupal\Core\Extension\InfoParserException
   +   */
   

   I kind of like when people describe when their exceptions in thrown.

6. +++ b/core/lib/Drupal/Core/Utility/ProjectInfo.php
   index 2165bd7..1d99987 100644
   --- a/core/modules/aggregator/tests/modules/aggregator_test_views/aggregator_test_views.info.yml
   
   --- a/core/modules/aggregator/tests/modules/aggregator_test_views/aggregator_test_views.info.yml
   +++ b/core/modules/aggregator/tests/modules/aggregator_test_views/aggregator_test_views.info.yml
   
   +++ b/core/modules/aggregator/tests/modules/aggregator_test_views/aggregator_test_views.info.yml
   @@ -1,4 +1,5 @@
   +type: module
   
   index 0357267..08e6aa3 100644
   --- a/core/modules/dblog/tests/modules/dblog_test_views/dblog_test_views.info.yml
   
   --- a/core/modules/dblog/tests/modules/dblog_test_views/dblog_test_views.info.yml
   +++ b/core/modules/dblog/tests/modules/dblog_test_views/dblog_test_views.info.yml
   
   +++ b/core/modules/dblog/tests/modules/dblog_test_views/dblog_test_views.info.yml
   @@ -1,4 +1,5 @@
   +type: module
   

   Good catch!

7. +++ b/core/modules/system/lib/Drupal/system/Tests/Extension/InfoParserUnitTest.php
   @@ -0,0 +1,96 @@
   +class InfoParserUnitTest extends DrupalUnitTestBase {
   

   Is there a real reason why we don't write a real unit test already?

1. Done
2. Done
3. Out of scope because I think then we should make all Yaml parsing use this service
4. Done
5. Done
6. Thanks :)
7. Yep we use format_plural - no OO equivalent and faking it is not a 1 line function

1. +++ b/core/modules/system/tests/fixtures/broken.info.txt
   @@ -0,0 +1,9 @@
   +dependencies::;;
   

   This makes me twitch so bad. I guess it works as desired :)

2. +++ b/core/modules/system/tests/fixtures/missing_key.info.txt
   @@ -0,0 +1,8 @@
   +# info.yml for testing missing type key.
   

   I do this all the time, so I'm glad we're handling it better now.

This is great, the unit test is sound, RTBC!

+++ b/core/modules/update/update.module
@@ -624,7 +624,7 @@ function update_verify_update_archive($project, $archive_file, $directory) {
-    $info = drupal_parse_info_file($file->uri);
+    $info = \Drupal::service('info_parser')->parse($file->uri);

This is the only thing I'm not crazy about. We're actually calling this in like 7 or 8 places. Seems like it probably belongs on the Drupal class as a top-level method. But since this is neither the first nor last place that we're doing this, it doesn't make sense to hold up this lovely, lovely patch on this.

AWESOME WORK folks! The before/after screenshots in #16 make my heart sing. :)

Committed and pushed to 8.x. Thanks!

Updated issue summary.