ORCA update displaying errors for javascript

[zrpnr]

After the recent ORCA update, our branches started failing because our javascript files are being checked by PHPCS. I'm not sure what in ORCA changed to start checking them, but, regardless I don't think that ORCA should be using PHPCS to test JS at all. Drupal core uses eslint, not PHPCS.

If you set up ORCA locally and use the command orca qa:static-analysis --phpcs ../YOUR_PROJECT/web/core/modules/quickedit/js you can demonstrate that not even core passes the same set of standards checks. I absolutely agree that ORCA should be checking the code quality of javascript but it should be using eslint or prettier to match how Drupal core works.

[wimleers]

To unblock acquia_migrate, pinned to the previous ORCA version, per @TravisCarden's advice: https://github.com/acquia/acquia_migrate/commit/d8e8071bdcc055b7fef4e02a3903de519e69dce5

[wimleers]

Related: #63.

[TravisCarden]

Thank you, @zrpnr. Allow me to explain the current behavior, provide a workaround, and provide options for a longterm solution.

1. First, the reason it behaves as it does is because ORCA (via acquia/coding-standards, just inherits the rules provided by the Coder module, which is stricter than core in general. (Core's PHP fails many of its sniffs, too, which is why its phpcs.xml file enables them so selectively.) This is consistent with ORCA's current policy, which acknowledges [the Coding Standards] committee as the authority and treats Coder Sniffer as the canonical codification of policy.
2. In the short term, you have a few options to address the new failures:
   1. Fix them - Obviously. ¯\_(ツ)_/¯
   2. Allow the job to fail - If you don't mind coding standards errors or just want some time to fix them without them failing your whole builds, you can continue to run the job but allow it to fail. (Of course, if you really don't care about standards, you could eliminate the job altogether.)
   3. Use a relaxed standard - ORCA provides a configuration option for choosing between three approved rulesets. AcquiaDrupalTransitional or AcquiaPHP might get you what you need.
   4. Patch ORCA - You can use composer create-project --prefer-source in your .travis.yml to get ORCA as a working Git repository and then do anything you want to it--for example, modify its phpcs.xml file. You could commit a patch file to your repo for easy application. Or you could downgrade the version of acquia/coding-standards with composer require.
   5. Override the job - You can replace or supplement the default Coding Standards job to do whatever you want, such as installing and running eslint. ORCA itself does this in its own builds.
3. In terms of long term solutions, i.e., making ORCA do what you want out-of-the-box, there would need to be a policy change and corresponding development effort. Both would need to be approved. If you'd like to submit a feature request with your rationale, I'll gladly bring it to the product owners for consideration.

I hope that gets you unblocked! I'm going to presume to close this issue. Reopen it if you need to. Thanks.

Now I need to convert this comment to documentation. 😃

[gabesullice]

Travis, I think your presumption that the Coder module is the "canonical codification of policy" is misinformed. See: https://www.drupal.org/node/2873849

Your proposed short-term solutions will result in Acquia adopting deprecated core standards instead of the most up-to-date, official standards.

The long term solution is not to change policy, but to acknowledge that the official coding standards for JavaScript are codified in core's eslint configuration, not the Coder module.

[zrpnr]

To add to this, many of the specific "violations" relate to jsx, which means any React code will be rejected outright. In Drupal core package.json their eslint includes react and jsx plugins so that it can recognize valid code that coder sniffer does not.

[wimleers]

2.iii: AFAICT AcquiaDrupalTransitional is nearly identical to AcquiaDrupalStrict which we currently use. The only difference is <rule ref="DrupalPractice"/> versus <rule ref="DrupalPractice.Commenting.ExpectedException"/> . I don't see how that's going to help with JS coding standards violations? Those only affect PHP coding standards.

Note that https://packagist.org/packages/acquia/coding-standards says

AcquiaDrupalTransitional provides a relaxed standard for legacy Drupal codebases or teams new to Drupal coding standards. It incorporates AcquiaPHP and adds a more or less straight copy of Drupal core's own phpcs configuration, making it sufficient for core contribution.

… but that doesn't seem to be true based on what we're observing here? Maybe it is no longer true?

[gabesullice]

@TravisCarden, bump :) not sure if you got notified about our replies given that the issue is closed and none of us actually tagged you.

[TravisCarden]

Thanks, @gabesullice. @phenaproxima also brought this up today. Let's reopen the issue for further discussion, shall we?

The change notice you linked to is super helpful; thanks. It's adequate justification for making the switch ourselves. We probably want to exclude JavaScript files in acquia/coding-standards (we might also want to file an issue upstream with Coder Sniffer asking them to remove the obsolete sniffs at the source) and add an orca qa:static-analysis --eslink command option and build it into the Travis CI scripts with all its prerequisite dependencies.

My focus is divided at the moment as I hustle to catch up on a large backlog while servicing new business priorities. Am I missing anything here? Also, as I'm not very familiar with npm, any implementation advice anyone can provide would be appreciated.

[gabesullice]

@TravisCarden, sounds good :)

I think it's already been handled upstream: https://www.drupal.org/project/coder/issues/2746449

That issue might explain why JS files were being ignored prior to the latest ORCA release.

[gabesullice]

Also related: https://www.drupal.org/project/coder/issues/3100537

[TravisCarden]

Thanks, everybody. I've tested and committed a fix that prevents PHPCS from sniffing JavaScript files. Until the next release comes out, you may use ORCA's develop branch (dev-develop) if you want. I'll speak with my project team about prioritizing the addition of eslint.