Add wp-cli integration #458

matiasbenedetto · 2024-09-06T14:44:01Z

What?

Add wp-cli integration for theme-check plugin

Why?

To use the output of the theme-check plugin in automated environments like CI workflows.

How to use and how to test

In your WordPress install theme-check plugin using this branch.
Install wp-cli in your system.
Run in your console wp theme-check run.
Observe the output in your console

It should look like this:

Running wp theme-check run :

Running wp theme-check run --format=json:

Screencast

Screencast.from.06-09-24.11.39.55.webm

jffng

When the theme check fails, the error messages are not logged. All that is returned is:

$ wp theme-check run
Error: Theme check failed for twentytwentyfive.

wp-cli/class-theme-check-cli.php

jffng · 2024-09-06T17:29:54Z

Nice change! I can't push to your branch, but here is some suggestion changes to the readme:

Usage with wp-cli

To use with wp-cli, ensure the theme check plugin is active and wp-cli is installed. The theme-check subcommand is added to wp-cli and can be used as follows:

wp theme-check run [<theme>] [--format=<format>]

Options

Option	Accepts	Required	Default
`theme`	The slug of the theme to check	No	Current theme slug
`format`	`cli` or `json`	No	`cli`

Examples

wp theme-check run
wp theme-check run twentytwentyfour
wp theme-check run --format=json
wp theme-check run twentytwentyfour --format=json

Co-authored-by: Jeff Ong <jonger4@gmail.com>

matiasbenedetto · 2024-09-06T17:34:35Z

@jffng cool, it looks good. I added you as a collaborator in the fork so you can push :)

codestylist · 2024-09-06T19:30:08Z

What a cool feature @matiasbenedetto . Thank you for this! Hope it will be in the master soon!

wp-cli/class-theme-check-cli.php

ernilambar · 2024-09-07T16:13:41Z

wp-cli/class-theme-check-cli.php

+					'result'          => "Error: Theme check failed for {$check_theme_slug}.",
+					'messages'        => array(),
+				);
+				WP_CLI::log( wp_json_encode( $json_output, JSON_PRETTY_PRINT ) );


Note that output of log() is discarded to STDOUT when --quiet flag is passed. But here I think we should display JSON output whatever case if quiet flag is present or not.

Ok, I used line() to log the results.

I found 2 issues regarding wp-cli loggers while working on this:

Update log and warning logger functions comment according to the actual functionality wp-cli/wp-cli#5979

WP_CLI::line is the only method apart from error that's not muted by --quiet but is marked as just a retro-compat function wp-cli/wp-cli#5980

But here I think we should display JSON output whatever case if quiet flag is present or not.

Not sure I agree with bypassing --quiet, I would expect no output at all. An alternative is to make sure that the program always exits with a descriptive code; ie. 0 for OK, non-0 for errors.

edit: On reading @matiasbenedetto's comment below, seems like the checks are not meant to return a boolean.

It's a little bit counterintuitive, but the theme-check plugin doesn't work with the objective of outputting a boolean ("theme OK/ theme with errors"). The plugin is not designed like that. It just outputs a list of messages regarding the theme, but it never says whether this theme is OK or not. Maybe in the future, we could implement that, but that's not the case yet.

Still, I don't think bypassing --quiet is cool.

I think this was fixed on the latest commits.

wp-cli/class-theme-check-cli.php

.vscode/launch.json

ernilambar · 2024-09-10T06:20:02Z

When I run check on a theme with errors. wp theme-check run mazebox. Output is:

Error: Theme check failed for mazebox.

But when there is no errors, output includes recommendation and info items.

Success: Theme check completed for mazebox.

RECOMMENDED: No reference to register_block_pattern was found in the theme. Theme authors are encouraged to implement custom block patterns as a transition to block themes.

RECOMMENDED: No reference to register_block_style was found in the theme. Theme authors are encouraged to implement new block styles as a transition to block themes.
...

matiasbenedetto · 2024-09-10T18:44:55Z

When I run check on a theme with errors. wp theme-check run mazebox. Output is:
Error: Theme check failed for mazebox.

I don't know what kind of errors you are checking, but I think this is expected if the checking couldn't run. Theme-check will return an error message if the check can't be run for some reason (errors parsing theme code, for example). The success and error messages are only used to signal whether the plugin was able to run the checks or not, not to output the results of those checks.

But when there is no errors, output includes recommendation and info items.

It's a little bit counterintuitive, but the theme-check plugin doesn't work with the objective of outputting a boolean ("theme OK/ theme with errors"). The plugin is not designed like that. It just outputs a list of messages regarding the theme, but it never says whether this theme is OK or not. Maybe in the future, we could implement that, but that's not the case yet.

ernilambar · 2024-09-11T15:34:27Z

Example:

$ wp theme install twentysixteen --activate
Installing Twenty Sixteen (3.3)
Downloading installation package from https://downloads.wordpress.org/theme/twentysixteen.3.3.zip...
Using cached file '/Users/nilambarsharma/.wp-cli/cache/theme/twentysixteen-3.3.zip'...
Unpacking the package...
Installing the theme...
Theme installed successfully.
Activating 'twentysixteen'...
Success: Switched to 'Twenty Sixteen' theme.
Success: Installed 1 of 1 themes.

$ wp theme install twentyten --activate --quiet
$ echo $?
0

Another case:

$ wp theme list --format=json --quiet
[{"name":"twentytwelve","status":"inactive","update":"none","version":"4.3","update_version":"","auto_update":"off"}]

$ wp theme list --format=json
[{"name":"twentytwelve","status":"inactive","update":"none","version":"4.3","update_version":"","auto_update":"off"}]

There is no right or wrong approach. We can go for any approach we agree upon. But from my experience, general expectation in CLI is that if I pass --format=json then I would expect output in valid JSON format, whether quiet flag is passed or not.

vcanales · 2024-09-11T15:39:38Z

There is no right or wrong approach. We can go for any approach we agree upon. But from my experience, general expectation in CLI is that if I pass --format=json then I would expect output in valid JSON format, whether quiet flag is passed or not.

I see now. Yes, I agree that if we're explicitly asking for a format, it makes sense to bypass --quiet. On the other hand, these seem like incompatible flags to me; why would someone use both? This is an unrelated discussion, though.

jffng · 2024-09-11T15:45:33Z

wp-cli/class-theme-check-cli.php

+			if ( ! $success ) {
+				WP_CLI::error( "Theme check failed for {$check_theme_slug}." );
+			}
+			$this->display_themechecks_in_cli( $check_theme_slug );


Execution stops if a WP_CLI::error is thrown, so the errors are not being displayed. Could this line be moved above the $success check?

I'm not sure, i'm seeing this logic here:

// If the check process failed (if the checks were not able to run properly) raise an error. There's nothing else to do. if ( ! $success ) { WP_CLI::error( "Theme check failed for {$check_theme_slug}." ); } // If the checking process run print in the console the checks. This will also print the things marked as an error in the theme. $this->display_themechecks_in_cli( $check_theme_slug );

Could you explain why you think we should move it?

// If the check process failed (if the checks were not able to run properly) raise an error. There's nothing else to do.

I don't think this is correct. $success is the result of run_themechecks_against_theme which returns false if the theme fails a check.

Could you explain why you think we should move it?

Using twentytwentyfive, here is the output currently:

Error: Theme check failed for twentytwentyfive.

Moving call to display_themechecks_in_cli above the conditional, the result is:

Success: Theme check completed for twentytwentyfive. REQUIRED: phpcs.xml.dist XML file found. This file must not be in the production version of the theme. WARNING: assets/images/image-from-rawpixel-id-2222755.webp is 997.6 KB in size. Large file sizes have a negative impact on website performance and loading time. Compress images before using them. WARNING: assets/images/image-from-rawpixel-id-2224378.webp is 879.6 KB in size. Large file sizes have a negative impact on website performance and loading time. Compress images before using them. WARNING: assets/images/location.webp is 778.1 KB in size. Large file sizes have a negative impact on website performance and loading time. Compress images before using them. WARNING: assets/images/poster-image-background.webp is 574.7 KB in size. Large file sizes have a negative impact on website performance and loading time. Compress images before using them. INFO Themes that use the tag accessibility-ready will need to undergo an accessibility review. See https://make.wordpress.org/themes/handbook/review/accessibility/ WARNING: Your theme appears to be in the wrong directory for the theme name. The directory name must match the slug of the theme. This theme's correct slug and text-domain is twenty-twenty-five. (If this is a child theme, you can ignore this error.) INFO: Only one text-domain is being used in this theme. Make sure it matches the theme's slug correctly so that the theme will be compatible with WordPress.org language packs. The domain found is twentytwentyfive. RECOMMENDED: Requires PHP is recommended to have major and minor versions only (e.g. 7.4). Patch version is not needed (e.g. 7.4.1). Error: Theme check failed for twentytwentyfive.

I think the latter is more useful because it reports what checks failed (the first one marked REQUIRED).

Also I am not sure we should throw an error here if $success isn't true, because the theme check successfully ran, but the result was it failed the checks.

This command's focus should be on pass/fail check for a theme, rather than run is successfully completed or not. And exit code should be returned based on that I believe.

I think I could implement your feedback, please see: #458 (comment)

ernilambar · 2024-09-11T17:43:14Z

Just an idea: What if we simply list check results like CLI displays. Below is one example.
Each message is splitted into type and message. This way we could support different format using WP_CLI\Formatter(), like table, json, csv, yaml.

When I need to check in my CI runner where I just need to check required items or not, I could use:

wp theme-check run theme-slug --type=error --format=json

matiasbenedetto · 2024-09-13T13:59:09Z

I went through the theme-check plugin code again, and I found that we can get a boolean signaling if the theme has required changes or not. Knowing that I was able to I implemented all the reamaining feedback (thanks you all!) on the latest commits:

Exit code depending on the existence of required changes or not. https://github.com/matiasbenedetto/theme-check/blob/d82740ea74277dd3e776019594ab10923cfbdd60/wp-cli/class-theme-check-cli.php#L86
Simpler code
Use the wp-cli table formatting by default.

These are the results using TT4 theme (I removed theme.json file and screenshot image to get REQUIRED changes):

wp theme-check run

wp theme-check run --format=json

wp theme-check run non-existing-theme

jffng

It works well in my testing.

One question that does not have to be a blocker, but do you know how to add the help text for the theme-check subcommand when a user runs wp help?

README.md

Co-authored-by: Jeff Ong <jonger4@gmail.com>

matiasbenedetto · 2024-09-16T20:58:09Z

One question that does not have to be a blocker, but do you know how to add the help text for the theme-check subcommand when a user runs wp help?

@jffng Thanks for the review. I added it here: d60d1e3

matiasbenedetto added 6 commits September 6, 2024 11:32

add wp-cli integration

4aa93bf

php lint fix

4b4b75c

lint fix

9508699

use wp_strip_all_tags

a9d4944

avoid coalescing operator (??)

b28cd1b

lint

b39c7bc

jffng reviewed Sep 6, 2024

View reviewed changes

wp-cli/class-theme-check-cli.php Outdated Show resolved Hide resolved

wp-cli/class-theme-check-cli.php Outdated Show resolved Hide resolved

matiasbenedetto mentioned this pull request Sep 6, 2024

Handle checks and results as a structured array instead of an array of strings. #459

Open

use the right slug variable

4bdcdd9

Co-authored-by: Jeff Ong <jonger4@gmail.com>

Add documentation about wp-cli usage.

745e077

ernilambar reviewed Sep 7, 2024

View reviewed changes

wp-cli/class-theme-check-cli.php Show resolved Hide resolved

ernilambar reviewed Sep 7, 2024

View reviewed changes

wp-cli/class-theme-check-cli.php Outdated Show resolved Hide resolved

ernilambar reviewed Sep 7, 2024

View reviewed changes

wp-cli/class-theme-check-cli.php Show resolved Hide resolved

matiasbenedetto added 2 commits September 9, 2024 11:41

rename class and extend WP_CLI_Command class

2218fdc

format comments

75c2f9c

matiasbenedetto mentioned this pull request Sep 9, 2024

WP_CLI::line is the only method apart from error that's not muted by --quiet but is marked as just a retro-compat function wp-cli/wp-cli#5980

Open

simplify logging

37542b3

ernilambar reviewed Sep 9, 2024

View reviewed changes

wp-cli/class-theme-check-cli.php Outdated Show resolved Hide resolved

matiasbenedetto added 2 commits September 9, 2024 16:41

format function comment to make wp help command work as expected

e998dc0

lint fix

097bfc8

ernilambar reviewed Sep 9, 2024

View reviewed changes

.vscode/launch.json Show resolved Hide resolved

remove ide file commited accidentally

8d61a59

matiasbenedetto mentioned this pull request Sep 10, 2024

update gitignore file #460

Merged

jffng reviewed Sep 11, 2024

View reviewed changes

matiasbenedetto added 3 commits September 13, 2024 10:40

add table format, simplify code and right exit code

92a3b76

correct validation boolean

98b127b

validation value

d82740e

lint php fix

6879e1f

matiasbenedetto requested review from vcanales, jffng and ernilambar September 16, 2024 12:50

remove custom VALIDATION message

8660260

jffng approved these changes Sep 16, 2024

View reviewed changes

README.md Show resolved Hide resolved

matiasbenedetto and others added 2 commits September 16, 2024 17:56

add a help comment

d60d1e3

Update README.md

4131d01

Co-authored-by: Jeff Ong <jonger4@gmail.com>

fix lint

5b520e1

matiasbenedetto mentioned this pull request Sep 17, 2024

Add theme-check action Automattic/themes#8170

Merged

matiasbenedetto merged commit 0b2c2a3 into WordPress:master Sep 17, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add wp-cli integration #458

Add wp-cli integration #458

matiasbenedetto commented Sep 6, 2024 •

edited

Loading

jffng left a comment

jffng commented Sep 6, 2024

matiasbenedetto commented Sep 6, 2024 •

edited

Loading

codestylist commented Sep 6, 2024

ernilambar Sep 7, 2024

matiasbenedetto Sep 9, 2024

vcanales Sep 11, 2024 •

edited

Loading

matiasbenedetto Sep 17, 2024

ernilambar commented Sep 10, 2024

matiasbenedetto commented Sep 10, 2024 •

edited

Loading

ernilambar commented Sep 11, 2024

vcanales commented Sep 11, 2024

jffng Sep 11, 2024

matiasbenedetto Sep 11, 2024

jffng Sep 11, 2024

ernilambar Sep 11, 2024

matiasbenedetto Sep 13, 2024

ernilambar commented Sep 11, 2024

matiasbenedetto commented Sep 13, 2024 •

edited

Loading

jffng left a comment

matiasbenedetto commented Sep 16, 2024 •

edited

Loading

Add wp-cli integration #458

Add wp-cli integration #458

Conversation

matiasbenedetto commented Sep 6, 2024 • edited Loading

What?

Why?

How to use and how to test

Screencast

jffng left a comment

Choose a reason for hiding this comment

jffng commented Sep 6, 2024

Usage with wp-cli

Options

Examples

matiasbenedetto commented Sep 6, 2024 • edited Loading

codestylist commented Sep 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vcanales Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ernilambar commented Sep 10, 2024

matiasbenedetto commented Sep 10, 2024 • edited Loading

ernilambar commented Sep 11, 2024

vcanales commented Sep 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ernilambar commented Sep 11, 2024

matiasbenedetto commented Sep 13, 2024 • edited Loading

jffng left a comment

Choose a reason for hiding this comment

matiasbenedetto commented Sep 16, 2024 • edited Loading

matiasbenedetto commented Sep 6, 2024 •

edited

Loading

matiasbenedetto commented Sep 6, 2024 •

edited

Loading

vcanales Sep 11, 2024 •

edited

Loading

matiasbenedetto commented Sep 10, 2024 •

edited

Loading

matiasbenedetto commented Sep 13, 2024 •

edited

Loading

matiasbenedetto commented Sep 16, 2024 •

edited

Loading