Problem/Motivation

When creating a module, theme or profile, you have to create a .info.yml. file for Drupal to find your module, theme or profile.

In the .info.yml file you can add comments. YAML also allows comment at the end of a line. http://symfony.com/doc/current/components/yaml/yaml_format.html#comments

The ExtensionDiscovery processes all .info.yml files using a regex and will find all e.g. modules even with comments, except when you have a comment at the end of the type property. e.g. type: module # This is a comment.

The regex used in ExtensionDiscovery doens't process that .info.yml file correctly as e.g. a module, which means that Drupal doesn't recognize your module. The module is not found on the /admin/modules page and drush en my_module will result in an error: Unable to install modules action due to missing modules my_module.

The following .info.yml file will work:

# Machine name is hello_world
name: Hello World Module # required
description: My First D8 Module. A simple hello world module. # required
type: module 
core: 8.x # required
# optional settings below.
package: Custom
dependencies:
  - node
  - block
  

This .info.yml won't work:

# Machine name is hello_world
name: Hello World Module # required
description: My First D8 Module. A simple hello world module. # required
type: module # required
core: 8.x # required
# optional settings below.
package: Custom
dependencies:
  - node
  - block

Steps to reproduce

  • Create a module or take an existing module.
  • Update the .info.yml file on the type: moduleline. Change it to type: module # This is a comment
  • Clear the cache
  • Try to enable your module on /admin/modules or drush en my_module
  • /admin/modules won't display your module and drush will result in an error.

Proposed resolution

  • Update the regex used in ExtensionDiscovery so modules that have a trailing comment on the type line are also found.

Remaining tasks

  • Review the patch.

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Bill Choy’s picture

Issue summary: View changes
Bill Choy’s picture

Looks like a bad Regular Expression.

use Drupal\Core\Extension\ExtensionDiscovery;
...
/**
 * Helper function to scan and collect module .info.yml data.
 *
 * @return \Drupal\Core\Extension\Extension[]
 *   An associative array of module information.
 */
function _system_rebuild_module_data() {
  $listing = new ExtensionDiscovery(\Drupal::root());
   ...
   // Find modules.
  $modules = $listing->scan('module');

which calls scanDirectory() function

protected function scanDirectory($dir, $include_tests) {
  ...
      // Determine extension type from info file.
      $type = FALSE;
      $file = $fileinfo->openFile('r');
      while (!$type && !$file->eof()) {
        preg_match('@^type:\s*(\'|")?(\w+)\1?\s*$@', $file->fgets(), $matches);
        if (isset($matches[2])) {
          $type = $matches[2];
        }
      }
      if (empty($type)) {
        continue;
      }
      $name = $fileinfo->getBasename('.info.yml');
      $pathname = $dir_prefix . $fileinfo->getSubPathname();
Bill Choy’s picture

I'll create the following patch using Non-capturing group

--- a/drupal/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
+++ b/drupal/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
@@ -426,7 +426,7 @@ protected function scanDirectory($dir, $include_tests) {
       $type = FALSE;
       $file = $fileinfo->openFile('r');
       while (!$type && !$file->eof()) {
-        preg_match('@^type:\s*(\'|")?(\w+)\1?\s*$@', $file->fgets(), $matches);
+        preg_match('@^type:\s*(\'|")?(\w+)\1?\s*(?:\#.*)?$@', $file->fgets(), $matches);
         if (isset($matches[2])) {
           $type = $matches[2];
         }

Bill Choy’s picture

TR’s picture

Version: 8.0.0-beta11 » 8.0.x-dev
Component: configuration system » extension system
Status: Active » Needs work
Issue tags: -YAML, -.info file

OK, but why not replace the preg_match() with InfoParser, which is what we use everywhere else to parse .info.yml files? If you use InfoParser, then we can be sure that each line of the .info.yml is parsed the same way every time, so if one key allows comments at the end of the line so will all other keys. Trying to maintain a separate regular expression which does the same thing is a recipe for further bugs.

mgifford’s picture

Status: Needs work » Needs review

Hoping this runs the bot for the patch in #4.

TR’s picture

@mgifford - setting the status isn't enough to trigger a retest, you need to click on the "Add test" link in #4 to retest.

But, IMO, this is still the wrong fix. Reading the source code for ExtensionDiscovery you will see:

  /**
   * InfoParser instance for parsing .info.yml files.
   *
   * @var \Drupal\Core\Extension\InfoParser
   */
  protected $infoParser;

along with an accessor method that does lazy instantiation of that variable.

I read this as intent to use InfoParser in D8, and consider preg_match() just a relic from the first implementation of ExtensionDiscovery.

rakesh_verma’s picture

I tried the patch given in #4 and it solves the problem successfully.
Comment addition was successful after applying patch.

TR’s picture

@rakesh_verma: Well no, it doesn't solve the problem, it just hides the one specific error mentioned by the original poster.

The bug here is that preg_match() is used to parse the .info.yml file in just this one place, while everywhere else in core InfoParser is used. Having two mechanisms to parse the file *ensures* that there will be inconsistencies now and forever. The fix is to parse it the same way everywhere, that way we have the same behavior everywhere.

tstoeckler’s picture

Re TR: This specifically uses preg_match() instead of the InfoParser for performance reasons. Changing that should be a separate issue which would require benchmarks including e.g. on the Modules page to prove that there is no significant regression.

TR’s picture

@tstoeckler: I don't assume that, because there is no comment in the code that says the choice of preg_match() was deliberate. If it was deliberately chosen for performance reasons, then that should have been mentioned somewhere. In the code, at the very least, so future developers know not to mess with it. That wasn't done, so it seems you're jumping to conclusions here, unless you have personal undocumented knowledge of why the preg_match() was used. Can you point to the issue where that was decided? The preg_match() is in the initial commit of ExtensionDiscovery, and that issue is #2188661: Extension System, Part II: ExtensionDiscovery - there is no discussion there about using a preg_match() in lieu of a more correct parsing mechanism. There's no benefit to being fast but wrong ...

"Require" benchmarking to fix a buggy 'optimization' that wasn't even documented as an 'optimization' ? It should be the other way around - if someone insists on using preg_match() instead of the InfoParser to parse .info.yml files, that use of preg_match() should have a documented and significant improvement in speed before it is done. There shouldn't be extra hoops to jump through to do it the correct way, instead you should have to prove your case if you want to use an ad hoc mechanism like preg_match().

Where are the tests to show that this 'optimization' is even doing the right thing? It clearly HASN'T been parsing the files the same as InfoParser, so perhaps the problem with comments at the end of the line is only one of many parsing problems due to preg_match(). Why isn't an extensive test case required for this patch to prove that it parses the same as InfoParser?

The point of concern is that this section of code with preg_match() will never be updated when InfoParser is changed, and even if the preg_match() can be made to do the exact same thing as InfoParser right now then it's a point of failure for the future. Continued use of preg_match() here should be justified with test cases and benchmarking, and there ought to be documentation both in ExtensionDiscovery and InfoParser stating why preg_match() is used and warning that changes to the InfoParser need to be tracked in ExtensionDiscovery.

tstoeckler’s picture

If it was deliberately chosen for performance reasons, then that should have been mentioned somewhere. In the code, at the very least, so future developers know not to mess with it.

That is 100% correct, it totally should have been! Sadly it wasn't, though...

That wasn't done, so it seems you're jumping to conclusions here, unless you have personal undocumented knowledge of why the preg_match() was used.

Yes, I do. I was involved when the original ExtensionDiscovery patch landed. The idea is that in contrast to D7 *all* info files are scanned at the very same time (i.e. both modules, themes, profiles, etc.). They are then later properly parsed by a YAML parser, but that is fairly expensive, so they are separated per their type key up-front. The simple preg_match() is inarguably a lot faster than a Yaml::parse() (which is what InfoParser ends up doing), but I don't know the code path well enough to say whether this is still justified. I.e. if all the info ends up being parsed by InfoParser anyway, then it's not worth it. Originally, this was not the case however.

It should be the other way around - if someone insists on using preg_match() instead of the InfoParser to parse .info.yml files, that use of preg_match() should have a documented and significant improvement in speed before it is done.

It did have a significant improvement when it got in (see above), it just sadly wasn't documented.

Where are the tests to show that this 'optimization' is even doing the right thing? It clearly HASN'T been parsing the files the same as InfoParser, so perhaps the problem with comments at the end of the line is only one of many parsing problems due to preg_match(). Why isn't an extensive test case required for this patch to prove that it parses the same as InfoParser?

I totally agree there should be a test. But there isn't one at the moment, that's simply a fact. The fact that the parsing is different is in the nature of preg_match() vs. Yaml::parse() so it will always be different. A unit test, however, would ensure that all/many different use-cases are possible and are consistently parsed.

---

Basically I agree on all points except that we cannot use InfoParser here. We do not need benchmarks on the function itself to verify that InfoParser is going to be significantly slower, that's just in the nature of what it does. We could do some macro-benchmarks of entire page loads (i.e. Modules page / cache clear / ...) but again I think that should be a separate issue. Changing the usage of preg_match() is simply out of scope here. But I would totally welcome a unit test of ExtensionDiscovery as part of this issue, to prove that the updated regular expression is correct.

dawehner’s picture

TR’s picture

Status: Needs review » Needs work

@tstoeckler: OK, since that's the case, I guess fixing the preg_match() is the way to go here.

I'm changing the status to "Needs work" because the use of preg_match() here has to be documented in code comments. And perhaps the patch can wait until scanDirectory() is testable, so that we can ensure a test gets written to accompany this fix.

dawehner’s picture

Issue tags: +Needs tests

Just a related issue..

In general we should have a concrete .info.yml as part of a test, so we are sure your bugfix actually worked.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev
JeroenT’s picture

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests, -drupaldevdays +ddd2022
+++ b/core/tests/Drupal/Tests/Core/Extension/ExtensionDiscoveryTest.php
@@ -98,6 +98,17 @@ public function testExtensionDiscoveryCache() {
+  public function testExtensionDiscoveryTypeComment() {

Übernit: could use a void return type 🤓 But that can be fixed on commit for sure and is not a hard rule.

Test-only patch locally:

There was 1 failure:

1) Drupal\Tests\Core\Extension\ExtensionDiscoveryTest::testExtensionDiscoveryTypeComment
Failed asserting that an array has the key 'module_info_type_comment'.

And the proper patch passes.

So: 🚢

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 26: 2513524-26-test-only.patch, failed testing. View results

JeroenT’s picture

Status: Needs work » Reviewed & tested by the community

Thanks for the review @Wim Leers!

Back to RTBC.

TR’s picture

xjm’s picture

Title: comments in .info.yml problem » Allow comments at the end of lines .info.yml files
Status: Reviewed & tested by the community » Needs review

One question about this issue. It is against Drupal coding standards to have comments after a line of code. Comments are always supposed to be on their own line before the line of code that the comment refers to.

What's the reasoning for expanding the regex to deliberately support this in more places? It might be "bad code style should not cause an exception" (if that is indeed what happens; I did not check) but the fact that we do have to expand the regex in a performance-critical spot makes it so that I still think it's worth deliberately discussing that.

Thanks!

xjm’s picture

Status: Needs review » Needs work

While we're at it, let's also address #27. Please don't ask committers to fix things on commit. Thanks!

TR’s picture

Re #32

In this case, YAML is a defined format which allows comments at the end of lines. It's not a syntax error, so parsing should not fail. Valid YAML should not crash Drupal just because of a comment position.

We have different coding standards for different types of code - PHP standards are not the same as CSS or JS standards. If we want to enforce this with YAML files then that should be done in the coding standards checks, but I believe that's a separate issue from this one, which is about preventing Drupal from choking on valid YAML.

Note: Core uses comments at the end of line in YAML files. See core.services.yml for example.

andregp’s picture

Addressing #33.

The last submitted patch, 35: 2513524-34.patch, failed testing. View results

JeroenT’s picture

Title: Allow comments at the end of lines .info.yml files » ExtensionDiscovery is unable to find modules that have a comment at the end of the type property in a .info.yml file
Issue summary: View changes

Updated the IS. I hope that makes things more clear.

It is possible that Drupal doesn't allow comments after a line, but adding a comment after a line in a .info.yml file works on every line except on the line where you define the type e.g. type: module.

Drupal no longer recognises your module. This is probably an edge case, but in my opinion this is something that we should fix.

JeroenT’s picture

Issue summary: View changes
joachim’s picture

Status: Needs review » Reviewed & tested by the community

LGTM.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 1066bfe28a to 10.0.x and 761e2df52c to 9.5.x and 1787414f9c to 9.4.x and 3586d9b316 to 9.3.x. Thanks!

  • alexpott committed 1066bfe on 10.0.x
    Issue #2513524 by andregp, JeroenT, Bill Choy, TR, tstoeckler, dawehner...

  • alexpott committed 761e2df on 9.5.x
    Issue #2513524 by andregp, JeroenT, Bill Choy, TR, tstoeckler, dawehner...

  • alexpott committed 1787414 on 9.4.x
    Issue #2513524 by andregp, JeroenT, Bill Choy, TR, tstoeckler, dawehner...

  • alexpott committed 3586d9b on 9.3.x
    Issue #2513524 by andregp, JeroenT, Bill Choy, TR, tstoeckler, dawehner...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.