Add YAML linting to core coding standards checks

I suggest yamlJS because it doesn't have a dependency on grunt, like grunt-yamllint does.

sorry, double post.

Addressing blockers for this issue: #2654650: [meta] Jobs for linting and coding standards

The symfony issue linked above seems to have been fixed with deprecation for symfony 2.8: https://github.com/symfony/symfony/pull/16285

Drupal 8's symfony dependency is 2.7.*, so this is an argument in favor of bumping up to 2.8.* #2611816: Update to symfony 2.8

We should still do YAML linting/CS here, though.

@Mixologic - This is the final code style check I believe.

Adjust title.

We can't really do much here until core has decided what the standard is and what tools we should use to enforce that standard. Our maybe wet know that already?

PECL extension yml sometimes wrong as well

Do we already test if the YAML is parsable?

The symfony issue quoted in the IS has been fixed.

This can be added directly in core now without requiring changes to DrupalCI.

#24: What changed?

Sorry that would have been useful information to add - since #3178845: Run same checks as committers do on DrupalCI.

Quick stab at this using https://www.npmjs.com/package/yaml-lint

PHP CodeSniffer claims to check YAML files but it doesn't seem to do a very good job, maybe we should remove it from the equation if we have something better now.

This looks good.

It won't help with issues like #3112452: Fix indentation consistency in core's yaml files. though where the YAML is valid but confusing - so I think we should open a follow-up to see if we can add formatting checks as well as the raw linting.

I wonder if it's worth exploring eslint-plugin-yml as an alternative as that seems to be able to handle coding style as well. We already use a number of eslint plugins.

https://www.npmjs.com/package/eslint-plugin-yml

Back to NR for that. If we can combine linting and coding style in eslint that'd be worth doing I think.

This looks like it might be a winner. Running eslint-plugin-yml with the strictest set of options across core reports many errors, mostly because we do not use consistent quoting. But we can switch all the rules off (including Prettier, which implements some YAML rules) and start with the "recommended" set, and get 9 errors:

core/core.services.yml
  1237:5  error  Empty mapping values are forbidden  yml/no-empty-mapping-value
  1241:5  error  Empty mapping values are forbidden  yml/no-empty-mapping-value

core/drupalci.yml
   6:5  error  Empty mapping values are forbidden  yml/no-empty-mapping-value
  48:7  error  Empty mapping values are forbidden  yml/no-empty-mapping-value

core/modules/system/config/install/system.authorize.yml
  1:1  error  Empty mapping values are forbidden  yml/no-empty-mapping-value

core/modules/system/tests/fixtures/HtaccessTest/access_test.yml
  1:1  error  Empty documents are forbidden  yml/no-empty-document

core/modules/system/tests/themes/test_theme_libraries_empty/test_theme_libraries_empty.info.yml
  6:1  error  Empty mapping values are forbidden  yml/no-empty-mapping-value

core/tests/Drupal/Tests/Core/Asset/library_test_files/empty.libraries.yml
  1:1  error  Empty documents are forbidden  yml/no-empty-document

core/tests/Drupal/Tests/Core/Asset/library_test_files/invalid_file.libraries.yml
  1:0  error  Parsing error: All collection items must start at the same column

As an example the two errors in core.services.yml are where we have a "tags" key but nothing inside it:

  ajax_response.attachments_processor:
    class: Drupal\Core\Ajax\AjaxResponseAttachmentsProcessor
    tags:
    arguments: ['@asset.resolver', '@config.factory', '@asset.css.collection_renderer', '@asset.js.collection_renderer', '@request_stack', '@renderer', '@module_handler']
  html_response.attachments_processor:
    class: Drupal\Core\Render\HtmlResponseAttachmentsProcessor
    tags:
    arguments: ['@asset.resolver', '@config.factory', '@asset.css.collection_renderer', '@asset.js.collection_renderer', '@request_stack', '@renderer', '@module_handler']

eslint-plugin-yml also only brings with it a single child dependency as we already have the rest of ESLint installed.

The attached patch builds on #33:

• The errors in core.services.yml are fixed
• The four deliberately malformed test fixtures are ignored
• drupalci.yml has the yml/no-empty-mapping-value ignored for the time being

This only leaves system.authorize.yml, which as far as I can see is no longer used in core, apart from a migration that writes to it? Probably worth ignoring this here and exploring in a followup issue?

Added a patch in #2715145: Remove system.authorize config to deal with system.authorize.yml.

Addressed #36 with an additional option when running eslint. Also added --quiet to match the other calls to eslint in commit-code-check.sh.

Whoops, thanks for fixing that. Another reason I should embrace the Gitlab workflow for all issues!

I also think we should drop YAML linting from PHPCS if we have a better tool doing it, PHPCS isn't doing a very good job anyway - let's open a followup for that if this is committed.

/Users/alex/dev/drupal/core/modules/system/config/install/system.authorize.yml
  1:1  error  Empty mapping values are forbidden  yml/no-empty-mapping-value

/Users/alex/dev/drupal/core/modules/user/config/install/user.mail.yml
  2:8  error  Parsing error: Multi-line double-quoted string needs to be sufficiently indented

We have a couple of new (i guess) errors to fix.

While working on this I found a major bug in the recent change to user.mail.yml: #3212034: Account emails are missing newlines due to malformed YAML

user.mail.yml was fixed in the linked issue.

system.authorize.yml will be deleted in #2715145: Remove system.authorize config but we can fix the syntax here temporarily.

Rerolled #37 and fixed the above.

Needs rerolling.

Rerolled as a merge request.

Could we update the IS with:

1. the chosen solution and why
2. how to run the added testing

I think we also should add a CR for the added test functionality.

Updated IS.

Added CR: https://www.drupal.org/node/3226497

The doc page should be updated as well, I think. https://www.drupal.org/docs/develop/development-tools/running-core-devel...

When I read the CR, which is fine, I thought it would be helpful to link to the doc page, so I added it. Took it straight from https://www.drupal.org/node/3185278.

Updated the IS with how to run eslint for yaml files.

I found out that after installing the MR and running the command from the IS that the changes for core/core.services.yml are needed to pass the new eslint for yaml files.

I think we need a followup for when there is a change to core/package.json that all files are tested to see if they pass. Just like we did for PHPCS in #3224583: The testbot does not run PHPCS on all files when core/phpcs.xml.dist is changed.

Changing the status to needs work for the unresolved threads.

Upgraded eslint-plugin-yml, responded to the other comments.

Just one open unresolved thread.

Need to check here that drupalci actually runs as expected - the default validate_codebase steps should be skipped and nightwatchjs should run as normal.

The validate_codebase step is missing entirely from the Jenkins output (as it does nothing, this is fine) and the Nightwatch tests ran as expected so to me this is an OK change to make. We probably should follow our own best practices and validate all our YAML except when we are explicitly testing invalid YAML.

The new yarn package for eslint-plugin-yml has been added in the MR.
The file core/scripts/dev/commit-code-check.sh has been updated for the package.
When I run the command to test for failures with the new plugin, I get a number of vialures which are fixed in the MR (core/drupalci.yml and core/core.services.yml).
The IS is in order and there is an CR.
After the MR has landed the page https://www.drupal.org/drupalorg/docs/drupal-ci/customizing-drupalci-tes... needs updating.
For me it is RTBC.

Committed b85fb24 and pushed to 9.3.x. Thanks!

Published change record. Noticed this in an unexpectedly broken contrib build…