[META][DX] Automate phpcs code checks (linting) against all backdrop/backdrop PRs.

[serundeputy]

Describe your issue or idea

Run phpcs Backdrop code standards checks against all backdrop/backdrop PRs.

• Devs get near real time feedback on code standards violations that can be addressed before a human gets the time to review it for technical implementation.

Actual behavior (if reporting a bug)

• Currently PRs are not checked for code standards except by humans.

Expected behavior (if reporting a bug)

• Automate the phpcs checks in ZenCI

---

There are two PRs on the table to enable this to happen:

• Add phpcs checks to PRs for backdrop/backdrop. backdrop-ops/backdropcms.org#460
  • This gets BackdropCMS to fire the ZenCI requests
• Check if we can fire phpcs in ZenCI. backdrop#2244
  • this one against core provides the test to run

---

To run the phpcs checks locally follow the instructions in the README.md here: https://packagist.org/packages/backdrop/coder

---

Here is an example of running phpcs locally against one file:

vendor/bin/phpcs --standard=./vendor/backdrop/coder/coder_sniffer/Backdrop core/modules/admin_bar/admin_bar.inc

and the output:

FILE: ...f/code/sites/backdrop-ops/backdrop/core/modules/admin_bar/admin_bar.inc
--------------------------------------------------------------------------------
FOUND 18 ERROR(S) AND 3 WARNING(S) AFFECTING 20 LINE(S)
--------------------------------------------------------------------------------
  35 | WARNING | Line exceeds 80 characters; contains 81 characters
 124 | WARNING | Line exceeds 80 characters; contains 81 characters
 126 | ERROR   | Missing parameter type at position 1
 168 | ERROR   | Missing parameter type at position 1
 172 | ERROR   | Data type of return value is missing
 215 | ERROR   | Inline comments must start with a capital letter
 252 | ERROR   | Doc comment for var A does not match actual variable name
     |         | $tree at position 1
 252 | ERROR   | Parameter comment must be on the next line at position 1
 254 | ERROR   | Missing parameter type at position 2
 256 | ERROR   | Missing parameter type at position 3
 367 | ERROR   | Missing parameter type at position 1
 369 | ERROR   | Missing parameter type at position 2
 396 | ERROR   | The first index in a multi-value array must be on a new line
 399 | ERROR   | Array indentation error, expected 6 spaces but found 4
 408 | ERROR   | Missing parameter type at position 1
 465 | ERROR   | Missing parameter type at position 1
 469 | ERROR   | Data type of return value is missing
 479 | ERROR   | Inline comments must end in full-stops, exclamation marks, or
     |         | question marks
 757 | ERROR   | Missing parameter type at position 1
 821 | WARNING | Format should be "* Implements hook_foo().", "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz_bar().",, "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz-bar.html.twig.", or "*
     |         | Implements hook_foo_BAR_ID_bar() for xyz-bar.tpl.php.".
 834 | ERROR   | Missing parameter type at position 1
--------------------------------------------------------------------------------
UPGRADE TO PHP_CODESNIFFER 2.0 TO FIX ERRORS AUTOMATICALLY
--------------------------------------------------------------------------------

---

• We can break this up into per module tasks.
• We should file the PRs against this branch https://github.com/backdrop/backdrop/tree/phpcs rather than the 1.x branch until we get complete coverge

Each participant should self assign themselves a core module and add it to the list here:

core/includes

• database @serundeputy
  • mysql @serundeputy
• filetransfer

core/modules

• admin_bar @serundeputy
• block @serundeputy
• book @serundeputy
• ckeditor
• color
• comment
• config
• contact
• contextual
• dashboard
• date
• dblog
• email
• entity
• field
• field_ui
• file
• filter
• image
• installer
• language
• layout
• link
• locale
• menu
• node
• path
• redirect
• search
• simpletest
• syslog
• system
• taxonomy
• translation
• update
• user
• views
• views_ui

[serundeputy]

@quicksketch for core/module/X on the X.info files phpcs reports:

geoff@yep backdrop (phpcs) exit code: [0] $ vendor/bin/phpcs --standard=./vendor/backdrop/coder/coder_sniffer/Backdrop core/modules/admin_bar/admin_bar.info

FILE: .../code/sites/backdrop-ops/backdrop/core/modules/admin_bar/admin_bar.info
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 1 | ERROR | "core" property is missing in the info file
--------------------------------------------------------------------------------
UPGRADE TO PHP_CODESNIFFER 2.0 TO FIX ERRORS AUTOMATICALLY
--------------------------------------------------------------------------------

should we be adding the core property to the info files or write some magic into the phpcs rules to ignore this for core modules` but use it for custom and contrib?

[serundeputy]

@quicksketch there are probably going to be a fair amount of these lower_case class names; here is what I did so far:

// @codingStandardsIgnoreStart
// @TODO: Make all classes CamelCase and class methods lowerComelCase in 2.x.
class views_plugin_argument_default_book_root extends views_plugin_argument_default_node {	class views_plugin_argument_default_book_root extends views_plugin_argument_default_node {
  function get_argument() {	  function get_argument() {
    // Use the argument_default_node plugin to get the nid argument.	    // Use the argument_default_node plugin to get the nid argument.
@@ -19,3 +21,4 @@ class views_plugin_argument_default_book_root extends views_plugin_argument_defa
    }	    }
  }	  }
}	}
// @codingStandardsIgnoreEnd

seem appropriate? what do you think?

[alexfinnarn]

I successfully ran the command vendor/bin/phpcs --standard=./vendor/backdrop/coder/coder_sniffer/Backdrop core/modules/admin_bar/admin_bar.inc

and got FOUND 18 ERROR(S) AND 3 WARNING(S) AFFECTING 20 LINE(S) plus the other output in that comment.

I like UPGRADE TO PHP_CODESNIFFER 2.0 TO FIX ERRORS AUTOMATICALLY if that works? I'm all about being lazy :) Can that happen for the subtasks on the meta-issue?

[serundeputy]

I like UPGRADE TO PHP_CODESNIFFER 2.0 TO FIX ERRORS AUTOMATICALLY if that works? I'm all about being lazy :) Can that happen for the subtasks on the meta-issue?

I know the php version for that version of phpcs is > php 5.3 so i'm not sure we could do that

[ghost]

what version number do put on it?

Does it need a version number...? As a "command line tool" and "not a module" (source: https://www.drupal.org/project/coder) can't we just have a 1.x branch that has Backdrop's up-to-date coding standards?

it's specifically for PHPCS

What if we called it something like backdrop_phpcs then?

[klonos]

As a "command line tool" and "not a module"

Good catch, ...but even drush has versions: https://github.com/backdrop-contrib/backdrop-drush-extension/releases

What if we called it something like backdrop_phpcs then?

I like that 👍

[jenlampton]

Why change the name from coder though, if it's the same thing? Won't it be easier for people coming from Drupal?

[ghost]

it will be used by less than 20% of actual Backdrop sites

Won't it be easier for people coming from Drupal?

What percentage of Backdrop sites using Coder will be coming from Drupal...? 😉

Edit: If it's easier to keep the name, that's fine. Was just a suggestion that made more sense to me.

[jenlampton]

In today's weekly meeting @quicksietch pointed out that in the future we may also want to include javascript testing, and that we might want the code for those js tests to live in the same project as the code for our PHP tests. That way, if we made a change to our coding standards that affected both, we could update the project with one PR.

So maybe including phpcs in the project is too specific. What about something like backdrop_standards? Though my preference is still to stick with coder.

What percentage of Backdrop sites using Coder will be coming from Drupal...?

Right now: 100% of people using coder are coming from Drupal. But as Backdrop grows that number should decrease :)

[ghost]

Here're some thing that might be of interest:

• https://github.com/marketplace/actions/phpcs-code-review (defaults to Wordpress standards, but can be customised)
• https://github.com/marketplace/actions/phpcs-drupal-check-with-annotations (uses Drupal standards, but might be hard-coded...)

If they don't suit, there are plenty of others: https://github.com/marketplace?type=actions&query=phpcs

[ghost]

It seems we got hung up on the name. But that aside...

I was thinking that we might want to fork that one (instead of using what's in coder_review) because Drupal's project already works with PHPCS 3.x and with Drupal 7.

We need to decide if we fork Drupal's Coder module, or if we just pull the coder_sniffer directory out of coder_review and make it its own project that others can depend on.

@docwilmot is the current maintainer for coder_review. Thoughts?

[ghost]

I'm asking again now because I'm wanting to add PHPCS checks to Bee, and I'd rather not have to download the entire coder_review module when all I need is what's in the coder_sniffer subdirectory...

[docwilmot]

Just including the directory makes sense.

[ghost]

@docwilmot Do you want to do that, or you'd rather someone else maintain that part of the code...?

[indigoxela]

I'm not sure, where to add the PR, but it seems that I got some basic linter setup working, which only fires on updated/added files and adds annotations.

Example:
https://github.com/backdrop/backdrop/pull/3749/files

[quicksketch]

That looks great @indigoxela! I'm a fan of the GitHub Actions to avoid a lot of custom scripting in our own repository. Now if we can work out applying Backdrop-specific code style.

[serundeputy]

@quicksketch is this https://github.com/backdrop/backdrop/pull/3749/files#diff-a036b60d22557d7f21abeaecaa42088f12680e7d72056f9d7de8c0db426984a6R32 not backdrop specific?

idk ... it's been a few years lolz;

[ghost]

Since this was originally raised for Zen.CI and since phpcs has been implemented as a GitHub action, I'm closing this issue. The fixing of existing coding issues throughout core will be handled in #6004