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: module
line. Change it totype: 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
Comment | File | Size | Author |
---|---|---|---|
#35 | 2513524-34-tests-only.patch | 1.53 KB | andregp |
#35 | interdff_2513524_26-34.txt | 683 bytes | andregp |
#35 | 2513524-34.patch | 2.24 KB | andregp |
#26 | 2513524-26.patch | 2.23 KB | JeroenT |
Comments
Comment #1
Bill Choy CreditAttribution: Bill Choy commentedComment #2
Bill Choy CreditAttribution: Bill Choy commentedLooks like a bad Regular Expression.
which calls scanDirectory() function
Comment #3
Bill Choy CreditAttribution: Bill Choy as a volunteer and commentedI'll create the following patch using Non-capturing group
Comment #4
Bill Choy CreditAttribution: Bill Choy as a volunteer and commentedComment #5
TR CreditAttribution: TR commentedOK, 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.
Comment #6
mgiffordHoping this runs the bot for the patch in #4.
Comment #7
TR CreditAttribution: TR commented@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:
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.
Comment #8
rakesh_verma CreditAttribution: rakesh_verma commentedI tried the patch given in #4 and it solves the problem successfully.
Comment addition was successful after applying patch.
Comment #9
TR CreditAttribution: TR commented@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.
Comment #10
tstoecklerRe 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.
Comment #11
TR CreditAttribution: TR commented@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.
Comment #12
tstoecklerThat is 100% correct, it totally should have been! Sadly it wasn't, though...
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 byInfoParser
anyway, then it's not worth it. Originally, this was not the case however.It did have a significant improvement when it got in (see above), it just sadly wasn't documented.
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.
Comment #13
dawehnerAt least #2605654: Modify Drupal\Core\Extension\ExtensionDiscovery to allow for scanning multiple file systems, enabling vfsStream testing will give a start of some form of test coverage.
Comment #14
TR CreditAttribution: TR commented@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.
Comment #15
dawehnerJust 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.Comment #26
JeroenTComment #27
Wim LeersÜ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:
And the proper patch passes.
So: 🚢
Comment #28
Wim LeersComment #30
JeroenTThanks for the review @Wim Leers!
Back to RTBC.
Comment #31
TR CreditAttribution: TR commentedComment #32
xjmOne 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!
Comment #33
xjmWhile we're at it, let's also address #27. Please don't ask committers to fix things on commit. Thanks!
Comment #34
TR CreditAttribution: TR commentedRe #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.
Comment #35
andregp CreditAttribution: andregp at CI&T commentedAddressing #33.
Comment #37
JeroenTUpdated 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.
Comment #38
JeroenTComment #39
joachim CreditAttribution: joachim at Factorial GmbH commentedLGTM.
Comment #40
alexpottCommitted 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!