search

backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 39 forks source link

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

Closed serundeputy closed 1 year ago

serundeputy commented 6 years ago

Describe your issue or idea

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

Actual behavior (if reporting a bug)

Expected behavior (if reporting a bug)

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

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
--------------------------------------------------------------------------------

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

core/includes

core/modules

serundeputy commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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

klonos commented 5 years ago

Mentioning #659

klonos commented 5 years ago

@drupol in https://github.com/backdrop/backdrop-issues/issues/3896#issuecomment-506693859:

Hi,

I started to do this project: drupol/backdrop-conventions, maybe it would be something to evaluate ?

It's based on Grumphp.

klonos commented 5 years ago

I commented re this on Gitter, but I was sure that we had an issue someplace for this 😄

I just came across https://github.com/marketplace/hound which seems to be free for open-source, public projects

Hound reviews your code then comments directly in Pull Requests, helping your team maintain consistent coding style and high code quality. ...integrates with your existing workflow by reviewing changes and commenting the moment a Pull Request is created or updated.

image

image

Supports phpcs: http://help.houndci.com/en/articles/2461415-supported-linters

...seems all we need is a subscription, and adding a .hound.yml file to the repo.

klonos commented 5 years ago

...also https://github.com/marketplace/imgbot which is free!

image

ghost commented 4 years ago

@serundeputy What is needed to make this happen? i.e. Can/how can I help?

quicksketch commented 4 years ago

@serundeputy Part of the recent changes to the packager on BackdropCMS.org allows us to now exclude certain files from the ZIP package files on releases. It already excludes .zenci.yml and .gitlc.yml files.

I'd love to have us switch to a .zenci.yml file (just copy the current .gitlc.yml off of backdropcms.org) and put it in the repository. And that way we'll be able to verify that the changes in the YML file work as expected all in one PR, since the code style checks will apply to the PR on which the new YML file is included. Could you rework this into a single PR just against Backdrop core?

quicksketch commented 4 years ago

On further thought, for something like this where we're just running code checks, could we use GitHub actions? We have actions enabled on the Backdrop repository, so you should be able to put scripting into a .github/workflows/phpcs.yml file (or similarly named) and have it run. @jenlampton tried an example one on the backdrop-issues repository a while back: https://github.com/backdrop/backdrop-issues/commit/fd6058d8a4476eaa9f039db9ec803a128b244c82

jenlampton commented 4 years ago

@quicksketch I don't think I ever got any actions working, but back then actions were new and maybe still had some issues.

Here's a PHPCS example file: https://github.com/marketplace/actions/phpcs-code-review

jenlampton commented 4 years ago

I started working on PHPCS reviews via github actions last week, and I was eventually able to get them working.

I jotted down some quick notes on the changes that were required in Let's get PHP_CodeSniffer working with Backdrop #10 since @alanmels was having the same problems.

edit: If it's of value, here's the yaml file my contrib project was using.

jenlampton commented 4 years ago

@serundeputy started looking at GH actions for this, but need more research … I’ve not determined where and how they actually load PHPCS into the action, so that I can inject backdrop/coder into it as well

The project is now named coder_review for Backdrop, and from my testing a git clone did not work, but a wget did. I think I read somewhere that a curl would also work, so to remove the specific version and grab latest, I was going to try something like curl --silent "https://api.github.com/repos/$1/releases/latest" | grep -Po '"tag_name": "\K.*?(?=")' instead, but I didn't get there last week.

quicksketch commented 4 years ago

In the rapidly changing landscape of GitHub Linters, GitHub has published their own official linter for use through GitHub Actions: https://github.blog/2020-06-18-introducing-github-super-linter-one-linter-to-rule-them-all/

Unfortunately it does not yet support PHP, but there's a lot of activity on that front: https://github.com/github/super-linter/issues/122

However regardless of which approach we use, we'll need to make sure that our linting rules for phpcs are up-to-date.

quicksketch commented 4 years ago

And the "super-linter" now supports PHP. Yay! But it's extremely rudimentary, just syntax checking, not code style. We may want to still pursue our own linter after all.

ghost commented 4 years ago

Heard some discussion about this in the dev meeting, and I was wondering, if we're going to be using this for all Backdrop core PRs etc., should the code styleguide be in Backdrop core rather than a contrib module...?

@serundeputy is doing this with Drush, and he uses his own repo: https://github.com/serundeputy/coder He basically forked the contrib module, then removed all the other stuff and just left the code sniffer stuff (the contrib module does a lot of different things it seems).

I was thinking we should standardise this by merging @serundeputy's repo into core (then Drush can use that too), and then removing that part of it from the contrib module. So the contrib module does everything else it currently does, and just the code sniffer part is in core.

Make sense?

klonos commented 4 years ago

Make sense?

It does to me.

jenlampton commented 4 years ago

The problem with having it in core is that it will be used by less than 20% of actual Backdrop sites, which goes against our principles. Maybe we can have the packager strip it out?

Also, the "code styleguide" isn't a generic set of instructions for Backdrop, it's specifically for PHPCS... (not sure if that matters, other than to say "it's useless" by itself)

ghost commented 4 years ago

Ok, so not core, but what about its own repo in backup-ops then?

My point being basically that as far as I'm aware, the code we need to integrate with PHP Code Sniffer (like all the Backdrop-specific styleguide stuff) in lumped in to a module that does other things too. Related things, sure, but not necessary, for example, when you're testing a core PR against our coding standards. I believe this is why @serundeputy forked the contrib module and pulled out all that other stuff, because it wasn't necessary to include all that each time you just want to check code on the command line.

So I suggest we pull that PHPCS stuff out of the Coder Review module, and then put it in its own repo (either backdrop-ops or its own contrib module). Then anyone that needs it (Backdrop core when testing PRs, Drush, Coder Review, etc.) can depend on it.

jenlampton commented 4 years ago

So I suggest we pull that PHPCS stuff out of the Coder Review module, and then put it in its own repo (either backdrop-ops or its own contrib module). Then anyone that needs it (Backdrop core when testing PRs, Drush, Coder Review, etc.) can depend on it.

I think that makes sense. This is essentially what the Drupal coder contrib project is right now (there's no module-stuff in there anymore)

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. Maybe we even call ours "coder" (to match Drupal) and leave it in backdrop-contrib?

Then comes the other question I've been struggling with: what version number do put on it? If we wanted to match the Drupal version numbering it would end up 8.3.9 -- which I have mixed feelings about.

ghost commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

ghost commented 4 years ago

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...? :wink:

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

jenlampton commented 4 years ago

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 commented 3 years ago

Here're some thing that might be of interest:

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

ghost commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

Just including the directory makes sense.

ghost commented 3 years ago

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

indigoxela commented 3 years ago

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 commented 3 years ago

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 commented 2 years ago

@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 commented 1 year ago

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 https://github.com/backdrop/backdrop-issues/issues/6004