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

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

westonruter · 2024-05-31T18:35:58Z

package.json

+    "lint:php:plugins": "cross-env WPP_PLUGIN=$npm_config_plugin php -f bin/phpcs/lint.php",
+    "lint:php:plugins:fix": "cross-env WPP_FIX=1 WPP_PLUGIN=$npm_config_plugin php -f bin/phpcs/lint.php",


I guess these need to be updated?

Suggested change

"lint:php:plugins": "cross-env WPP_PLUGIN=$npm_config_plugin php -f bin/phpcs/lint.php",

"lint:php:plugins:fix": "cross-env WPP_FIX=1 WPP_PLUGIN=$npm_config_plugin php -f bin/phpcs/lint.php",

"lint:php:plugins": "cross-env WPP_PLUGIN=$npm_config_plugin php -f tools/phpcs/lint.php",

"lint:php:plugins:fix": "cross-env WPP_FIX=1 WPP_PLUGIN=$npm_config_plugin php -f tools/phpcs/lint.php",

westonruter · 2024-05-31T18:37:10Z

tools/phpcs/lint.php

+ * $ WPP_PLUGIN=plugin1,plugin2 php bin/phpcs/lint.php
+ * Would run phpcs on plugin1 and plugin2.
+ *
+ * $ WPP_PLUGIN=plugin1,plugin2 WPP_FIX=1 php bin/phpcs/lint.php
+ * Would run phpcbf on plugin1 and plugin2.
+ *
+ * $ php bin/phpcs/lint.php
+ * Would run phpcs on all plugins.
+ *
+ * $ WPP_FIX=1 php bin/phpcs/lint.php
+ * Would run phpcbf on all plugins.


Suggested change

* $ WPP_PLUGIN=plugin1,plugin2 php bin/phpcs/lint.php

* Would run phpcs on plugin1 and plugin2.

*

* $ WPP_PLUGIN=plugin1,plugin2 WPP_FIX=1 php bin/phpcs/lint.php

* Would run phpcbf on plugin1 and plugin2.

*

* $ php bin/phpcs/lint.php

* Would run phpcs on all plugins.

*

* $ WPP_FIX=1 php bin/phpcs/lint.php

* Would run phpcbf on all plugins.

* $ WPP_PLUGIN=plugin1,plugin2 php tools/phpcs/lint.php

* Would run phpcs on plugin1 and plugin2.

*

* $ WPP_PLUGIN=plugin1,plugin2 WPP_FIX=1 php tools/phpcs/lint.php

* Would run phpcbf on plugin1 and plugin2.

*

* $ php tools/phpcs/lint.php

* Would run phpcs on all plugins.

*

* $ WPP_FIX=1 php tools/phpcs/lint.php

* Would run phpcbf on all plugins.

westonruter · 2024-05-31T18:37:46Z

.github/workflows/php-lint.yml

@@ -22,6 +23,7 @@ on:
      - 'phpstan.neon.dist'
      - 'composer.json'
      - 'composer.lock'
+      - 'bin/phpcs'


Is this needed now that the script ends in .php (tools/phpcs/lint.php)?

Suggested change

- 'bin/phpcs'

westonruter · 2024-05-31T18:37:51Z

.github/workflows/php-lint.yml

@@ -13,6 +13,7 @@ on:
      - 'phpstan.neon.dist'
      - 'composer.json'
      - 'composer.lock'
+      - 'bin/phpcs'


Is this needed now that the script ends in .php (tools/phpcs/lint.php)?

Suggested change

- 'bin/phpcs'

thelovekesh · 2024-05-31T18:43:33Z

@westonruter I think we need to split this PR into two parts as well, similar to what we did in #1065 (comment)

Fix lint-staged per plugin
Running PHPCS within plugins.

For this PR, the main blocker was the second part. I think we can discuss that a little more and then raise a PR.

thelovekesh · 2024-05-31T18:48:25Z

Closing in favor of the above comment. cc @westonruter

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

Closed

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 mentioned this pull request May 22, 2024

Add zip-plugin-builds command #1228

Merged

westonruter reviewed May 22, 2024

View reviewed changes

thelovekesh and others added 22 commits May 31, 2024 23:52

Move lint and format command from composer to npm scripts

34512e5

Update PHP lint command in workflow

8f15cdf

Install npm-run-all globally

9471cc1

Bump lint-staged to latest version

ac37bda

Remove lint-staged config from package.json

9d91d43

Add micromatch

c451a3d

Add lint-staged.config.js

e7122ff

Update files path joining to escape filename with space in it

9bb9fff

Remove files length check in lint-staged config

2fec4e9

Update jsdoc type param

fd72c3d

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

Update micromatch options to include dotfiles

e6d0ddd

Add cross-env package

807571d

Add PHP lint script to run PHPCS and PHPCBF

4e9d161

Update lint:php scripts

1e97a2c

Revert lint command

ad7b776

Add logic to return with correct exit code

e7a0ea8

Add -f flag before passing PHP files

0bd76de

Update PHP lint commands in lint-staged config

e7f5efd

Fix PHPCS errors in bin/phpcs/lint.php

610ebfe

Install cross-env globally

baade8e

Update logger to use switch case

dc07f0d

Update lint-staged to run regex more precisely

4cd1af1

thelovekesh force-pushed the enhancement/composer-scripts branch from 0f69d3c to 4cd1af1 Compare May 31, 2024 18:27

westonruter reviewed May 31, 2024

View reviewed changes

thelovekesh closed this May 31, 2024

thelovekesh deleted the enhancement/composer-scripts branch June 3, 2024 13:22

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

Loading

github-actions bot commented Mar 26, 2024 •

edited

Loading

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

Loading

westonruter commented Apr 3, 2024

felixarntz commented Apr 3, 2024

thelovekesh commented Apr 4, 2024 •

edited

Loading

felixarntz commented Apr 4, 2024

westonruter May 22, 2024

westonruter May 31, 2024

westonruter May 31, 2024

westonruter May 31, 2024

westonruter May 31, 2024

thelovekesh commented May 31, 2024

thelovekesh commented May 31, 2024

		"lint:php:plugins": "cross-env WPP_PLUGIN=$npm_config_plugin php -f bin/phpcs/lint.php",
		"lint:php:plugins:fix": "cross-env WPP_FIX=1 WPP_PLUGIN=$npm_config_plugin php -f bin/phpcs/lint.php",

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

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

Conversation

thelovekesh commented Mar 26, 2024 • edited Loading

Summary

Relevant technical choices

github-actions bot commented Mar 26, 2024 • edited Loading

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 Loading

westonruter commented Apr 3, 2024

felixarntz commented Apr 3, 2024

thelovekesh commented Apr 4, 2024 • edited Loading

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

Choose a reason for hiding this comment

thelovekesh commented May 31, 2024

thelovekesh commented May 31, 2024

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

Loading

github-actions bot commented Mar 26, 2024 •

edited

Loading

felixarntz commented Apr 3, 2024 •

edited

Loading

thelovekesh commented Apr 4, 2024 •

edited

Loading