Enhancement: Add Plugin Check workflow

[adamsilverstein]

Impacted plugin

Jetpack

What

Add the WordPress core plugin plugin check as a GitHub action for new PRs and releases

How

Description

Plugin Check is a plugin checking tool developed by the WordPress core team (see original proposal). This tool is used as part of the plugin review process and will help ensure this plugin’s codebase remains up-to-date with the base required standards from the Plugin Review team. The tool includes both static and runtime checks and an ever-growing list of checks for various plugin development requirements and best practices.

Details

The plugin check can be run automatically using a GitHub action and the results of the Plugin Check analysis will be added in the form of annotations on new pull requests. Specific groups of checks can be enabled or disabled using the checks attribute. For an initial implementation, I suggest enabling all checks then disabling what you want to ignore. As the Plugin Check grows to include more tests, the list run in the automated action can be trimmed to whatever is preferred here.

[anomiex]

What does this have that is missing from our many existing checks?

[jeherve]

For reference, here is the output of the tests when running against the current stable version of the Jetpack plugin (I only have a screenshot, I couldn't get the CLI command to work unfortunately):

It outlines a few things we can (and should) fix, and a few others that I don't think we'll be able to change.

Here is a list of the things I think we could change after this first test:

• [x] Exclude some of the dotfiles that are still shipped with the stable versions of some of our packages. #34105
• [ ] Add comments to explain the phpcs:ignore comments listed above, to provide more context about why the exceptions were added.
• [x] Add a "License" info to the plugin's readme.txt, in addition to the existing LICENSE.txt that is shipped with the plugin. #34106

I wonder if this could also be an opportunity to discuss a few changes to the Plugin Check plugin itself, or to the configuration of the action:

1. Should the plugin avoid reporting issues for things that are explicitly ignored in the code with phpcs:ignore and phpcs:disable?
2. The plugin currently reports errors for uses of site_transient_update_plugins and auto_update_plugin, even though I believe those uses are legitimate in the Jetpack plugin. I suppose this is one of those checks that would need to be disabled in the action?
3. I don't know what The following readme parser warnings were detected: contributor_ignored was referring to.

I'm especially curious about 1, as it brings up a point that @anomiex mentioned above, about the value of running the analysis on a plugin like Jetpack which is already in the repo and already relies on phpcs compatibility checks on its PRs.

Overall, I think that such an analysis would be very valuable when a plugin is first submitted to the WordPress.org plugin directory, to introduce plugin authors to WordPress coding standards. But once a plugin has adopted the WordPress coding standards and has an existing setup so phpcs errors are a blocker for new code to be added to the codebase, maybe we would not need the extra set of checks, since it would report on errors that would already be caught by the phpcs checks?

@adamsilverstein What do you think about this?

[anomiex]

I don't know what The following readme parser warnings were detected: contributor_ignored was referring to.

I see the wporg readme validator has this to say:

One or more contributors listed were ignored. The Contributors field should only contain WordPress.org usernames. Remember that usernames are case-sensitive.

So probably that, some unspecified names in the list apparently aren't wporg usernames. Comparing the list in the readme to the list that's output in the sidebar at https://wordpress.org/plugins/jetpack/#developers, I see jasmussen is missing from the sidebar and two appear to link to different forms of the name, MichaelArestad→michael-arestad and Viper007Bond→viper007bond.

[adamsilverstein]

Great comments and feedback, thank you both for taking the time to review the PR and the results of the plugin check run!

What does this have that is missing from our many existing checks?

The checks are aimed at ensuring best practices and indeed may be duplicative of some checks you already run. The setup includes runtime checks in addition to static checks and may offer additional checks that aren't a part of phpcs for example.

It outlines a few things we can (and should) fix, and a few others that I don't think we'll be able to change.

Here is a list of the things I think we could change after this first test:

Exclude some of the dotfiles that are still shipped with the stable versions of some of our packages. General: do not ship development config files in production build #34105

it should also be possible to exclude these files if you feel they belong in the folders where the check found em

Add comments to explain the phpcs:ignore comments listed above, to provide more context about why the exceptions were added.

Sounds reasonable!

Add a "License" info to the plugin's readme.txt, in addition to the existing LICENSE.txt that is shipped with the plugin. Jetpack: add license information to the readme #34106

Nice, it caught an actual issue! @anomiex I guess these are examples of what this might catch that your existing checks didn't. The goal here is to run checks on the "built" version of your plugin which is typically a bit different than phpcs runs for example.

Should the plugin avoid reporting issues for things that are explicitly ignored in the code with phpcs:ignore and phpcs:disable?

I agree: if you already use PHPCS running and have decided to ignore something, this should respect that. I think the current approach may be because this tool is designed in part to be used by the plugin review team, and they may not want people just ignoring a bunch of issues. I think @swissspidy and I discussed, he may have learned more since.

The plugin currently reports errors for uses of site_transient_update_plugins and auto_update_plugin, even though I believe those uses are legitimate in the Jetpack plugin. I suppose this is one of those checks that would need to be disabled in the action?

Yes, we probably just want to exclude those checks, I'll update the PR to remove this and any other errors I see that we could ignore.

Overall, I think that such an analysis would be very valuable when a plugin is first submitted to the WordPress.org plugin directory, to introduce plugin authors to WordPress coding standards. But once a plugin has adopted the WordPress coding standards and has an existing setup so phpcs errors are a blocker for new code to be added to the codebase, maybe we would not need the extra set of checks, since it would report on errors that would already be caught by the phpcs checks?

I suspect you are right and I value the feedback which I will share with the Performance team. One of the goals for the tool is to make it generally useful for plugin authors, not just for initial submission (where it could be most useful), but also on an ongoing basis to avoid regressions. The differentiator is probably the runtime check capabilities.

I definitely see overlap with tools like PHPCS and that reduces the value for plugins like Jetpack that already run these types of tools. That calculus could change as we add more checks that aren't readily tested by static checks (for example performance or db use metrics), and for plugins that don't already have a linting pipeline. It signals we need to better articulate the value of using plugin check over existing solutions.

[swissspidy]

The Plugin Check plugin is still in early days. For example, v1.0 is not even in the repo yet, just on GitHub. Right now it contains mostly PHPCS checks useful for plugin review, which of course are less relevant for you because a) your plugin is already in the repo and b) you already run PHPCS. So those are the checks that you can easily disable.

As Adam said, for continuous usage, what matters is what's coming next: the runtime checks for various (performance-related) best practices.

[adamsilverstein]

I tried updated the PR attached to this issue to break out the configuration on a per plugin basis which will enable fine tuning over exactly which folder are included or excluded or which checks are ignored on a per-plugin basis