Problem/Motivation

There are two outdated exclude paths in our phpcs.xml.dist.

In #2573377: Add our own phpcs.xml file, we originally didn't need ./core at the start of our paths, but somewhere along the line we changed how the CI ran and required that path prefix. But that path prefix was never added to the assets path.

In #3299678: Deprecate DiffEngine and replace with sebastian/diff, we replaced crufty Diff code with proper code, but forgot to turn ON the phpcs linting we had turned off because Diff was crufty.

Steps to reproduce

  1. Add the ./core path prefix to ./assets/vendor/*
  2. Remove the <exclude-pattern>./core/lib/Drupal/Component/Diff/</exclude-pattern> from phpcs.xml.dist
  3. Run composer phpcs and watch it throw linting errors.

Proposed resolution

  1. Add the ./core path prefix to ./assets/vendor/*
  2. Remove the <exclude-pattern>./core/lib/Drupal/Component/Diff/</exclude-pattern> from phpcs.xml.dist
  3. Run composer phpcbf and commit changes to git
  4. Document any obvious bits of PHP and mark the rest with // phpcs:ignoreFile

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

n/a

CommentFileSizeAuthor
#5 3586835-nr-bot_xkw5wbxw.txt7.02 KBneeds-review-queue-bot

Issue fork drupal-3586835

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

johnalbin created an issue. See original summary.

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented
Status: Active » Needs review

I'm going to fix the first path and ensure phpcs doesn't report errors. Then fix the second path which will definitely throw linting errors.

johnalbin opened merge request !15572

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented

Ok. PHPCS is green when add core/ to the assets path. On to the next path.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3586835-nr-bot_xkw5wbxw.txt7.02 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

quietone’s picture

quietone commented
Issue tags: +Coding standards
johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented
Issue summary: View changes
Status: Needs work » Needs review

I added docs in:

  • Drupal\Component\Diff\DiffFormatter
  • Drupal\Component\Diff\Engine\DiffOp
  • Drupal\Component\Diff\Engine\DiffOpAdd
  • Drupal\Component\Diff\Engine\DiffOpChange
  • Drupal\Component\Diff\Engine\DiffOpCopy
  • Drupal\Component\Diff\Engine\DiffOpDelete

…because it was pretty clear what everything did.

I added // phpcs:ignoreFile to Drupal\Component\Diff\Engine\HWLDFWordAccumulator and Drupal\Component\Diff\WordLeveldiff (which uses HWLDFWordAccumulator) because I have no idea what "HWLDF" stands for. So even though I could half-way reason what the code was doing, I didn't feel comfortable writing docs for them.