Add PHP `lint` scripts in npm; Fix `lint-staged` config #1090

thelovekesh · 2024-03-26T05:31:25Z

Summary

As mentioned by @westonruter in #1012 (comment), for standalone plugins, PHPCS always takes phpcs.xml.dist into account since it is explicitly set to phpcs.xml.dist:

performance/composer.json

Line 39 in 5e570fa

    
           "lint:auto-sizes": "@lint -- ./plugins/auto-sizes --standard=./plugins/auto-sizes/phpcs.xml.dist",

This PR aims to move lint and format scripts from composer.json to package.json scripts to fix the above behavior and running commands from any directory will be easy with npm run.

Also it adds the lint-staged config to run the correct linting commands on staged files based on the correct PHPCS config.

Closes #1089

Relevant technical choices

Use the https://docs.npmjs.com/cli/v10/using-npm/config#command-line-flags feature to cd into a specific plugin directory and let PHPCS decide which specific config file to use.

github-actions · 2024-03-26T05:31:47Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: thelovekesh <thelovekesh@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: felixarntz <flixos90@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

lint-staged.config.js

westonruter · 2024-03-26T18:43:45Z

lint-staged.config.js

+		plugins.forEach( ( plugin ) => {
+			const pluginFiles = micromatch(
+				files,
+				`**/plugins/${ plugin }/**`


Naturally this would need to wait for #1065 to be merged first so that plugin-specific PHPCS rules are checked for test files.

Currently all the tests files adheres to root PHPCS config. Once they are moved to respective plugin directories, they will use plugin specific PHPCS config. In any case this file will remain same so #1065 isn't a blocker for this.

I don't think that's the case. For example, webp-uploads has a plugin-specific translation string:

performance/tests/plugins/webp-uploads/helper-tests.php

Line 345 in 2bc2f96

return new WP_Error( 'image_additional_generated_error', __( 'Additional image was not generated.', 'webp-uploads' ) );

Ahh, interesting. At this moment neither of the PHPCS configs are linting the tests/plugins directory as the plugins directory is excluded in the root PHPCS config. So they will be linted once #1065 is merged but no changes will be required in lint staged config. So we can merge it right?

lint-staged.config.js

.github/workflows/php-lint.yml

lint-staged.config.js

westonruter · 2024-03-28T22:45:17Z

Oh, and lint-staged.config.js needs to be added to .gitattributes.

felixarntz

@thelovekesh While this PR appears to solve the problem of plugin-specific phpcs.xml files not being respected, I'm not sure about the approach. Why are we moving PHP specific logic to require NPM? That seems a bit backwards to me, especially since it still requires writing a custom script to make it work.

Why not solve it with a PHP solution, which IMO would be more adequate for the problem? If we need to dynamically support plugin-specific phpcs.xml that a developer may have placed, that should be possible in a much less disruptive way by writing a custom PHP script.

By doing that, we'd keep the PHP linting tied to PHP (with NPM merely being a wrapper if anything), and this PR would contain fewer changes.

felixarntz · 2024-04-02T20:15:42Z

composer.json

-    "format": "build-cs/vendor/bin/phpcbf --report-summary --report-source",
-    "lint": "build-cs/vendor/bin/phpcs",
-    "lint:all": [
-      "@lint",
-      "@lint:auto-sizes",
-      "@lint:dominant-color-images",
-      "@lint:embed-optimizer",
-      "@lint:optimization-detective",
-      "@lint:speculation-rules",
-      "@lint:webp-uploads"
-    ],
-    "lint:auto-sizes": "@lint -- ./plugins/auto-sizes --standard=./plugins/auto-sizes/phpcs.xml.dist",
-    "lint:dominant-color-images": "@lint -- ./plugins/dominant-color-images --standard=./plugins/dominant-color-images/phpcs.xml.dist",
-    "lint:embed-optimizer": "@lint -- ./plugins/embed-optimizer --standard=./plugins/embed-optimizer/phpcs.xml.dist",
-    "lint:optimization-detective": "@lint -- ./plugins/optimization-detective --standard=./plugins/optimization-detective/phpcs.xml.dist",
-    "lint:speculation-rules": "@lint -- ./plugins/speculation-rules --standard=./plugins/speculation-rules/phpcs.xml.dist",
-    "lint:webp-uploads": "@lint -- ./plugins/webp-uploads --standard=./plugins/webp-uploads/phpcs.xml.dist",


Why are all of these being removed from here? Similar to @joemcgill's feedback in #1065 (comment), I think PHP based tooling makes sense to be accessible as a Composer script.

Can't this be solved in a PHP way? What's the benefit of moving all of these to NPM?

At a minimum, I think they should remain accessible via composer.json like before, while NPM could effectively be a "wrapper".

@felixarntz I have added #1065 (comment) in response to #1065 (comment).

At a minimum, I think they should remain accessible via composer.json like before, while NPM could effectively be a "wrapper".

Wouldn't that cause duplication and maintenance burden? Also, I would like to know why we want it on composer specifically while NPM has the same feature, a better build to handle mono-repo use cases.

@thelovekesh There would only be duplication if we have plugin specific scripts in both composer.json and package.json. What I'm proposing though is something like this:

We abstract the PHPCS specific call in a custom PHP script that takes the plugin slug as a parameter and takes the potential phpcs.xml from the plugin folder into account.

The composer.json can then use that instead of manually calling phpcs with the correct parameters.

No changes to package.json would be needed.

PHPCS is PHP specific tooling, and composer.json is the appropriate location to manage such PHP specific tooling.

@felixarntz Can you please take a look at the new changes? As suggested by @joemcgill, I have added a PHP script to handle linting in plugins and also added an NPM wrapper in composer commands.

thelovekesh · 2024-04-03T19:12:38Z

Why not solve it with a PHP solution, which IMO would be more adequate for the problem? If we need to dynamically support plugin-specific phpcs.xml that a developer may have placed, that should be possible in a much less disruptive way by writing a custom PHP script.

@felixarntz Why do we need a custom PHP script while NPM has it out of the box? And even if we keep it in composer, the number of commands would be the same. I can also argue about using PHP since all the CLI-based tasks are being handled by NodeJS CLI in this repo.

felixarntz · 2024-04-03T19:24:11Z

@thelovekesh

Why do we need a custom PHP script while NPM has it out of the box?

My point is that NPM doesn't have it out of the box. If it did, maybe that would be a good reason. But you implemented a custom NPM specific script https://github.com/WordPress/performance/pull/1090/files#diff-29c15f0b5d8d1144bd22153c302cde16c2de092d65b122a618cbdb3023336346 to make it work. At that point, it would be better to solve the problem in PHP and composer.json, as the affected tooling is purely for PHP, and NPM requires a custom script as well.

westonruter · 2024-04-03T19:59:41Z

My point is that NPM doesn't have it out of the box. If it did, maybe that would be a good reason. But you implemented a custom NPM specific script https://github.com/WordPress/performance/pull/1090/files#diff-29c15f0b5d8d1144bd22153c302cde16c2de092d65b122a618cbdb3023336346 to make it work.

@felixarntz Actually, no. This is unrelated. This is exclusively to fix the lint-staged issue I mentioned in #1012 (comment). It could be moved into a separate PR.

felixarntz · 2024-04-03T20:33:40Z

@westonruter

My point is that NPM doesn't have it out of the box. If it did, maybe that would be a good reason. But you implemented a custom NPM specific script https://github.com/WordPress/performance/pull/1090/files#diff-29c15f0b5d8d1144bd22153c302cde16c2de092d65b122a618cbdb3023336346 to make it work.

@felixarntz Actually, no. This is unrelated. This is exclusively to fix the lint-staged issue I mentioned in #1012 (comment). It could be moved into a separate PR.

Ah I see, thank you for clarifying. Looking at the new package.json scripts, what's preventing us from leaving that in composer.json? Can't we use cd there and run PHPCS that same way?

thelovekesh · 2024-04-04T04:05:13Z

@felixarntz there are a few scenarios here:

The current directory is the root,
The current directory is plugins/{some-plugin}
Command can handle custom args so we don't need to write our scripts.

By default, the composer works well in the first case. If you run any script from another directory it will ask:

No composer.json in current directory, do you want to use the one at /home/thelovekesh/Desktop/work/wpdev/public/content/plugins/performance? [Y,n]? y
Always want to use the parent dir? Use "composer config --global use-parent-dir true" to change the default.

but we can change this behavior by setting use-parent-dir to true in the global config which I don't think is a good idea.

Coming to the third point, if you pass some custom argument to NPM scripts like npm run foo --bar=baz, NPM CLI will convert it to $npm_config_bar which can be used to determine the value of --bar argument anytime. To achieve this in composer we have to write a custom script. I hope it explains the reason behind choosing NPM over composer.

felixarntz · 2024-04-04T16:11:00Z

Thanks for outlining this @thelovekesh.

What I'm curious about is why we need to support running these commands from a specific /plugins/{plugin} directory. Which benefits does developing from a subdirectory like this actually provide? I know @westonruter had mentioned this before somewhere, but I'm unsure why it's useful.

As this is a monorepo, IMO it makes sense to develop from the root folder, just like in any other repo, especially because some tooling is shared, and should be shared to avoid duplication. You probably wouldn't develop or run CI from a specific subdirectory of an individual plugin repository either.

westonruter · 2024-04-08T23:31:41Z

bin/phpcs/lint.php

+	$str = match ( $type ) {
+		'e' => "\033[31m$str \033[0m\n",
+		's' => "\033[32m$str \033[0m\n",
+		'w' => "\033[33m$str \033[0m\n",
+		'i' => "\033[36m$str \033[0m\n",
+		default => $str,
+	};


Ooo. Fancy. But match requires PHP8. Should this use switch instead for now?

Humm makes sense. I will make it a switch case, and it will not be fancy anymore 😢

We can make it fancy again in the future 😄

Co-authored-by: Weston Ruter <westonruter@google.com>

westonruter · 2024-05-22T20:10:26Z

lint-staged.config.js

+module.exports = {
+	'**/*.js': ( files ) => `npm run lint-js ${ joinFiles( files ) }`,
+	'**/*.php': ( files ) => {
+		const commands = [ 'composer phpstan' ];


As noted by @thelovekesh in #1228 (comment), this intentionally does not include the list of changed files as arguments as PHPStan should be run across the entire codebase.

thelovekesh added [Type] Enhancement A suggestion for improvement of an existing feature Infrastructure Issues for the overall performance plugin infrastructure no milestone PRs that do not have a defined milestone for release labels Mar 26, 2024

thelovekesh mentioned this pull request Mar 26, 2024

DX improvements in new monorepo #1012

Open

3 tasks

thelovekesh requested review from mukeshpanchal27 and felixarntz as code owners March 26, 2024 05:35

thelovekesh requested a review from westonruter March 26, 2024 05:50

thelovekesh mentioned this pull request Mar 26, 2024

Fix lint-staged running phpcs checks for standalone plugins #1089

Closed

westonruter reviewed Mar 26, 2024

View reviewed changes

westonruter reviewed Mar 27, 2024

View reviewed changes

lint-staged.config.js Outdated Show resolved Hide resolved

westonruter reviewed Mar 27, 2024

View reviewed changes

lint-staged.config.js Outdated Show resolved Hide resolved

westonruter reviewed Mar 27, 2024

View reviewed changes

lint-staged.config.js Outdated Show resolved Hide resolved

thelovekesh force-pushed the enhancement/composer-scripts branch from ff13e42 to e537663 Compare March 28, 2024 14:30

thelovekesh changed the title ~~Move lint and format scripts from composer to npm~~ Move lint and format scripts to npm; Fix lint-staged config Mar 28, 2024

thelovekesh force-pushed the enhancement/composer-scripts branch from e537663 to 5e0a487 Compare March 28, 2024 14:34

westonruter requested changes Mar 28, 2024

View reviewed changes

thelovekesh force-pushed the enhancement/composer-scripts branch 2 times, most recently from 02d3757 to 018aeb6 Compare March 29, 2024 04:48

thelovekesh requested a review from westonruter March 29, 2024 17:26

thelovekesh force-pushed the enhancement/composer-scripts branch from 018aeb6 to bfcdcdd Compare April 2, 2024 18:52

westonruter approved these changes Apr 2, 2024

View reviewed changes

felixarntz requested changes Apr 2, 2024

View reviewed changes

westonruter reviewed Apr 8, 2024

View reviewed changes

thelovekesh force-pushed the enhancement/composer-scripts branch from 8bb4f69 to 3247524 Compare April 9, 2024 18:08

thelovekesh and others added 24 commits April 19, 2024 15:57

Move lint and format command from composer to npm scripts

5dc07fb

Update PHP lint command in workflow

e1ecd20

Install npm-run-all globally

99a32f9

Bump lint-staged to latest version

729e5b2

Remove lint-staged config from package.json

1981cc0

Add micromatch

44bec33

Add lint-staged.config.js

8ad9583

Update files path joining to escape filename with space in it

3216d86

Remove files length check in lint-staged config

ceb7483

Update jsdoc type param

60580e7

Co-authored-by: Weston Ruter <westonruter@google.com>

Update micromatch options to include dotfiles

b0afaa1

Add lint-staged.config.js

e5f5d3e

Add cross-env package

e8ab1c0

Add PHP lint script to run PHPCS and PHPCBF

556e550

Update lint:php scripts

b963968

Revert lint and format scripts

9dd4ec3

Revert lint command

0def810

Add logic to return with correct exit code

104eec0

Add -f flag before passing PHP files

b299bd0

Update PHP lint commands in lint-staged config

8912d55

Fix PHPCS errors in bin/phpcs/lint.php

80fea20

Install cross-env globally

16e984f

Update logger to use switch case

6e8e5cf

Update lint-staged to run regex more precisely

0f69d3c

thelovekesh force-pushed the enhancement/composer-scripts branch from 3247524 to 0f69d3c Compare April 19, 2024 10:31

westonruter added this to the performance-lab n.e.x.t milestone May 20, 2024

westonruter mentioned this pull request May 22, 2024

Add zip-plugin-builds command #1228

Merged

westonruter reviewed May 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PHP `lint` scripts in npm; Fix `lint-staged` config #1090

Add PHP `lint` scripts in npm; Fix `lint-staged` config #1090

thelovekesh commented Mar 26, 2024 •

edited

github-actions bot commented Mar 26, 2024 •

edited

westonruter Mar 26, 2024

thelovekesh Mar 26, 2024

westonruter Mar 26, 2024

thelovekesh Mar 27, 2024

westonruter commented Mar 28, 2024

felixarntz left a comment

felixarntz Apr 2, 2024

thelovekesh Apr 3, 2024

felixarntz Apr 3, 2024

thelovekesh Apr 8, 2024

thelovekesh commented Apr 3, 2024

felixarntz commented Apr 3, 2024 •

edited

westonruter commented Apr 3, 2024

felixarntz commented Apr 3, 2024

thelovekesh commented Apr 4, 2024 •

edited

felixarntz commented Apr 4, 2024

westonruter Apr 8, 2024

thelovekesh Apr 9, 2024

westonruter Apr 9, 2024

westonruter May 22, 2024

Add PHP lint scripts in npm; Fix lint-staged config #1090

Are you sure you want to change the base?

Add PHP lint scripts in npm; Fix lint-staged config #1090

Conversation

thelovekesh commented Mar 26, 2024 • edited

Summary

Relevant technical choices

github-actions bot commented Mar 26, 2024 • edited

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

westonruter commented Mar 28, 2024

felixarntz left a 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

Choose a reason for hiding this comment

thelovekesh commented Apr 3, 2024

felixarntz commented Apr 3, 2024 • edited

westonruter commented Apr 3, 2024

felixarntz commented Apr 3, 2024

thelovekesh commented Apr 4, 2024 • edited

felixarntz commented Apr 4, 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

Add PHP `lint` scripts in npm; Fix `lint-staged` config #1090

Add PHP `lint` scripts in npm; Fix `lint-staged` config #1090

thelovekesh commented Mar 26, 2024 •

edited

github-actions bot commented Mar 26, 2024 •

edited

felixarntz commented Apr 3, 2024 •

edited

thelovekesh commented Apr 4, 2024 •

edited