Quote consistency in .info.yml files for Core modules

I have compared the patch to the audit file and all the changes look appropriate.

Two modules that had one word titles now have no quotes: HAL and Workflow, now have no quotes.

Nine modules that had Multiple word titles now have single quotes: Place Blocks, Internal Dynamic Page Cache, Inline Form Errors, Menu UI, Migrate Drupal, Internal Page Cache, Quick Edit, Responsive Image, and Activity Tracker.

Three modules that either had no quotes or double quotes around the description now have single quotes: Serialization, Tour, and CKEditor.

I didn't see Datetime changes in the patch however. Going to set this to needs work.

Thanks for bringing this up and making time for a patch. This is a great initiative that will help with consistency amongst all core modules and will also hopefully provide a standard for other contrib modules to follow.

I now see the Datetime changes and the single quotes around the description. Thanks for making the change and interdiff! The patch also applies successfully with Simplytest. Thanks again for doing this. I'm hoping others agree with your findings.

Looking good :)

I am also confirming the RTBC on this. I went through each change on the "Extend" page to verify the module names and descriptions worked for each change.

I also like this paradigm for use in contrib as well. It would be a welcomed convention to know when to use quotes and when not to. This could be a separate issue to discuss.

Good stuff. Thanks!

One comment on the issue summary which has:

"Write tests to prevent issue from occurring in the future as new or experimental modules are added to core."

This is an interesting idea. I don't know if we have any *tests* that check for code formatting. We use Code Sniffer when checking our Drupal code for coding standards, but it would be interesting to write tests to check formats. Curious if anyone knows if this exists anywhere.

This is a confusing, these are yml files and quotes are not needed. Why add such a requirement?

This inconsistency has been triggering my OCD for years :P Thanks @volkswagenchick! :)

I love this idea of making things consistent. Whatever is decided should probably be documented in our coding standards. A good place to start would be to add a new issue to the Coding Standards issue queue.

Some additional info about strings in .yml files:

• http://blogs.perl.org/users/tinita/2018/03/strings-in-yaml---to-quote-or...
• https://stackoverflow.com/questions/19109912/do-i-need-quotes-for-string...
• https://symfony.com/doc/current/components/yaml/yaml_format.html

tl;dr It seems that the prevailing "best practice" is to not use any quotes unless you need them.

-mike

Let's add a test to ensure this never comes back, could be done with coder (linting) or an actual test.

@volkswagenchick, thanks for the response. I understand that going with what people do in practice is very useful information but I still question it when that practice is unnecessary.

It seems that the prevailing "best practice" is to not use any quotes unless you need them.

+1. I agree with ultimike

I can't help but reread the goals in the YAML specification. The first goal of YAML is "YAML is easily readable by humans". For me adding quotes reduces readability by adding clutter and it even looks ugly. I don't say that lightly. Although limited to migration yml files I work with yml every day.

I've set the tests to run again, this time against the 8.7.x branch.

Maybe it'd be worth looking at opening a Coding Standards issue to find a linter that follows the format we want to use, and then getting that set up on the testing infrastructure?

@volkswagenchick, thanks for reviewing some more documentation (#22).

Wish I had time to do a proper review now :-(

Modules with one word title should use no quotes around the title.
Modules with multiple word title should use single quotes around the title.

What is the rationale behind such a decision? Strings with multiple words in YML files work fine. This might be a bit confusing because some IDEs (i.e. PhpStorm) highlight quoted strings differently.

Also wonder why this issue is limited to module info files?

Needs a reroll for 9.1.x. Note that some files no longer existing, e.g. core/modules/block_place/block_place.info.yml.

Re-rolling a patch against 9.1.x , kindly review a patch.

@volkswagenchick Did you want to mark RTBC?

Random failure , setting back to RTBC.

I am pleased to see the progress here.

I applied that patch and found that not all .info.yml files have been fixed.

$ grep -ri \' core/modules/*/*.info.yml
core/modules/migrate_drupal_multilingual/migrate_drupal_multilingual.info.yml:package: 'Core (Experimental)'

I also noticed that many files have two spaces after the ':', whereas only one character is necessary.

$ grep -ri 'description:  ' core/modules/*/*.info.yml | wc -l
78
$ grep -ri 'name:  ' core/modules/*/*.info.yml | wc -l
29

The tags show that this needs a reroll and that it needs tests. The reroll seems to have happened but not the test. The test was asked for in #20.

The Issue Summary proposed resolution does not match with what is done in the patch.

Setting to NW for the above.

Closed #2116897: Adopt identical YAML formatting for info.yml files as a duplicate, adding credit.

Hi, @quietone,

Working on the observations you have shared. Will update as soon as possible.

Thanks.

Hi, @quietone,

I have addressed the observations in this patch that you have shared in #45. Please review the same and let me know for any observation. And, if this looks good we can go ahead for writing the test cases asked in #20. Also, just to highlight the issue summary needs to get updated, as the first two points are strike-through, but we are still working on the patch.

Thanks.

This is a stab at creating tests.

It is placed under core/tests/Drupal/Tests/Core/Discovery. Not sure whether this is the right location.

Test results after applying the patch from #47:

$ phpunit -c /app/phpunit.xml core/tests/Drupal/Tests/Core/Discovery/YamlConsistencyTest.php
PHPUnit 8.5.8 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\Core\Discovery\YamlConsistencyTest
................................................................. 65 / 80 ( 81%)
............... 80 / 80 (100%)

Time: 280 ms, Memory: 6.00 MB

OK (80 tests, 240 assertions)

Reworked YamlConsistencyTest.

Plus added @group annotation.

Wrong extension for interdiff in #49.

Nice to have a test! Thx,

1. +++ b/core/tests/Drupal/Tests/Core/Discovery/YamlConsistencyTest.php
   @@ -0,0 +1,56 @@
   +  protected $paths;
   

   Not used

2. +++ b/core/tests/Drupal/Tests/Core/Discovery/YamlConsistencyTest.php
   @@ -0,0 +1,56 @@
   +  protected function setUp(): void {
   

   Not needed

3. +++ b/core/tests/Drupal/Tests/Core/Discovery/YamlConsistencyTest.php
   @@ -0,0 +1,56 @@
   +    $paths = glob($this->root . '/core/modules/*/*.info.yml');
   

   What about the info.yml files in test modules in Core modules?

4. +++ b/core/tests/Drupal/Tests/Core/Discovery/YamlConsistencyTest.php
   @@ -0,0 +1,56 @@
   +      $pattern = '/^[^\']+$/';
   

   Why are double quotes not tested for?

#51.1: removed
#51.2: removed
#51.3: see glob($this->root . '/core/modules/*/tests/modules/*/*.info.yml')
#51.4: see additional assertion

Grep results:

$ grep -ri \' core/modules/*/*.info.yml | wc -l
0
$ grep -ri \" core/modules/*/*.info.yml | wc -l
0
$ grep -ri 'name:  ' core/modules/*/*.info.yml | wc -l
0
$ grep -ri 'description:  ' core/modules/*/*.info.yml | wc -l
0

$ grep -ri \' core/modules/*/tests/modules/*/*.info.yml | wc -l
529
$ grep -ri \" core/modules/*/tests/modules/*/*.info.yml | wc -l
9
$ grep -ri 'name:  ' core/modules/*/tests/modules/*/*.info.yml | wc -l
0
$ grep -ri 'description:  ' core/modules/*/tests/modules/*/*.info.yml | wc -l
0

Hi,

I have found more info.yml file having inconsistencies quotes and even I have removed quotes from this aggregator_display_configurable_test.info.yml file which makes fail test.

I am creating patch for this.

Please review the patch.

Let me know for any recommendations.

Thanks.

@Pooja Ganjage, thanks for making a patch. The patch in #55 is 274KB which is significantly larger than the previous patch. That may be because you picked up on files that are currently not tested, see comment below, but that is hard to know without an interdiff. An interdiff really makes life easier for a reviewer. If you haven't already seen them instructions are available for creating an interdiff.

+++ b/core/tests/Drupal/Tests/Core/Discovery/YamlConsistencyTest.php
@@ -0,0 +1,55 @@
+    $paths = array_merge($paths, glob($this->root . '/core/modules/*/*.info.yml'));
+    $paths = array_merge($paths, glob($this->root . '/core/modules/*/tests/modules/*/*.info.yml'));

This is still limiting the directories being searched. This returns 369 files but a find, find core/modules -iname \*.info.yml | grep -v themes | nl, returns 419. I think this needs something more generic to recurse the directory tree.

I presume the grep results are after the patch and before the patch. Since the after results return 0 for all the greps but the test fails, I wonder where the error is?

Also, a reminder that the issue summary needs an update.

Cheers

All grep results in #52 are after the patch. They were included to illustrate the 'needs work' status (which remained unchanged in #52).
The test in #52 failed because of an assertion that allows no quotes at all.

Since there is no interdiff in #55, this patch builds on #52.

Modifications in .info files:

1. Remove single quotes around values for name and for description.
   

   grep -rl \" core/modules/*/tests/modules/*/*.info.yml | xargs sed -i 's/name: "\(.*\)"/name: \1/'
   grep -rl \" core/modules/*/tests/modules/*/*.info.yml | xargs sed -i 's/description: "\(.*\)"/description: \1/'

2. Remove double quotes around values for name and for description.
   

   grep -rl \' core/modules/*/tests/modules/*/*.info.yml | xargs sed -i "s/name: '\(.*\)'/name: \1/"
   grep -rl \' core/modules/*/tests/modules/*/*.info.yml | xargs sed -i "s/description: '\(.*\)'/description: \1/"

3. Remove single quotes around value for version.
   grep -rl \' core/modules/*/tests/modules/*/*.info.yml | xargs sed -i "s/version: '\(.*\)'/version: \1/"
4. Manual modifications in these files with special cases:
   

   core/modules/migrate_drupal/tests/modules/migrate_state_finished_test/migrate_state_finished_test.info.yml
   core/modules/migrate_drupal/tests/modules/migrate_state_not_finished_test/migrate_state_not_finished_test.info.yml
   core/modules/locale/tests/modules/locale_test/locale_test.info.yml
   core/modules/locale/tests/modules/locale_test_translate/locale_test_translate.info.yml
   core/modules/system/tests/modules/requirements1_test/requirements1_test.info.yml
   core/modules/system/tests/modules/requirements2_test/requirements2_test.info.yml
   core/modules/system/tests/modules/system_incompatible_module_version_dependencies_test/system_incompatible_module_version_dependencies_test.info.yml
   core/modules/system/tests/modules/system_incompatible_module_version_test/system_incompatible_module_version_test.info.yml

   Special cases meaning:
   * Quotes around properties. (quotes removed)
   * Quotes inside description. (4 files still have quotes, but not around the value for description)
   * Double single quotes (e.g. ''install'' --> changed to 'install')
   * Quotes around the value for dependencies. (quotes removed)

Modifications in testing:

1. Reordered the assertions.
2. New assertion to allow single quotes inside a name value.
   Covers this case:
   description: Tests the 'active' migrate state
3. Commented out no quotes at all assertion (@todo)
   Is it too strict? Given the case in the preceding bullet and also the comment in #17 "to not use any quotes unless you need them".
4. Commented out assertion for description and excess whitespace. (@todo)
   The current assertion fails because some .info files do not have a description. Are descriptions required or not?

Clarifying the standards around .info file consistency would benefit further development of the assertions.

Regarding recursing the directory tree: to do

Now with .patch extension.

Change of tack for the test:

• .info files are first parsed with the Symfony Yaml parser.
• quote requirements are checked via a trait extended from the Symfony Yaml Escaper class.
• use of regex assertions to check if a value is unquoted, single quoted or double quoted in .info.yml file..
• checks are limited to values for name and description.

The patch contains another solution to recurse the folders search for .info.yml files. (if finds 419 files, see #57)

Some module names were not properly (un)quoted.

fixed test case kindly check.

The test fails with a deprecation notice .. highlighting that relying on Symfony's

Symfony\Component\Yaml\Escaper

class is flawed, because Symfony considers it internal and is subject to change, by them at any time.

I think the best way forward is to copy and modify the Escaper into Drupal's namespace

I don't like that fact that we take on the maintenance of someone else's code but on the good side it is small and self contained

The copied Symfony class doesn't match our coding standards, so work needed to clean that up.

Alternatively should we just use the Symfony class for the double-quoting rules and a custom method on the test itself for the single-quoting rules? That would save us copying the class at all; we only use it in this one place anyway.

Finally, should we have unit tests for the failure cases in the test?

Fixed coding standards in the copied Symfony class.

The alternative solution suggested in 67.2.

Rerolled patch.

'Needs work' because it does not comply with the standards proposal.

A reworked patch.

The test no longer uses the Symfony Escaper class.
It follows the standards proposal in the issue summary.

These sample values for description pass the consistency test:

• can also use " quotes ' and $ a % lot /&?+ of other ~ @ <> stuff
• no need to escape backslash \
• this is a link https://www.drupal.org
• '& starts with a special character, needs quotes'
• 'to express one single quote inside a quoted string, use '' two of them'

Thanks for working on this, I just created #3278144: Remove quotes from olivero.info.yml.

The suggested guideline in this issue is "Standards proposal for name and description values: The first character in the name value is a letter or underscore." which I agree with, since you don't need to quote a YAML string, as long as the first character is not a special character.

In the Issue Summary, only titles and descriptions are mentioned, but what about other strings, for example under regions in a theme, or alt text? Shouldn't we be consistent, and don't quote any string at all by default, except for special cases? (The rules under "In the following cases, use single quoting").

For more, see:

• https://www.yaml.info/learn/quote.html#plain
• https://stackoverflow.com/a/69850618

So I agree with @ultimike's proposal (4 October 2018):

tl;dr It seems that the prevailing "best practice" is to not use any quotes unless you need them.

I also agree with Chi (21 March 2019):

Modules with one word title should use no quotes around the title.
Modules with multiple word title should use single quotes around the title.

What is the rationale behind such a decision? Strings with multiple words in YML files work fine. This might be a bit confusing because some IDEs (i.e. PhpStorm) highlight quoted strings differently.

Also, this comment by @Chi is interesting, and makes sense to me:

Also wonder why this issue is limited to module info files?

... as well as @ultimike's observation:

I love this idea of making things consistent. Whatever is decided should probably be documented in our coding standards. A good place to start would be to add a new issue to the Coding Standards issue queue.

I agree, it would be great to document YAML coding standards, for example for quoting. Perhaps the "Proposed resolution" for quoting as seen in the Issue Summary could be added to YAML Configuration files?

Or should an issue be created under https://www.drupal.org/project/issues/coding_standards for discussion?

Added reroll of path #76 on Drupal 9.4.x.

The process for changing coding standards is documented on the project page for the Coding Standards project. It starts with making an issue in that project. My search turned up one issue referring to YAML, #2823463: Devise an extensible way to document extensible data structures.

Fixed Drupal CS issue of patch #80.

Was it an intentional sadistic joke that for Boolean values in Drupal YAML files quotes MUST be present in some uses and MUST NOT be present in other uses?

  requirements:
    _access: 'TRUE'
  options:
    no_cache: TRUE

If I am not mistaken in the value _access: 'TRUE' is actually treated as strings.
Real boolean in YML are never quoted.

Am I the only one who thinks this is a confusing and bizarre thing to do (use 'TRUE' as a string)?

Added reroll of patch #82 on Drupal 9.5.x and tried to fix failed test of patch #82 as well.

There is no patch here to review. What needs to be done here now is to make a coding standards issue to decide on the style to use.