Closed mukeshpanchal27 closed 4 months ago
I spent a bit of time experimenting with some options for this, and while extending rulesets using relative paths is pretty straightforward (see docs), overriding configuration set in one ruleset with another is not as straightforward due to this issue.
To address this, we would need to create a new ruleset in the relative directory that includes the base rules from performance lab, and also includes an override ruleset that updates the config value for text_domain
.
Ex ./plugins/auto-sizes/phpcs.xml.dist
<?xml version="1.0"?>
<ruleset name="WPP Auto-sizes">
<description>PHPCS rules for the WPP: Auto Sizes plugin</description>
<rule ref="../../phpcs.xml.dist"/>
<rule ref="./override.phpcs.xml"/>
</ruleset>
Ex ./plugins/auto-sizes/override.phpcs.xml
<?xml version="1.0"?>
<ruleset name="WPP Auto-sizes Overrides">
<description>PHPCS overrides for the WPP: Auto Sizes plugin</description>
<config name="text_domain" value="auto-sizes"/>
</ruleset>
This starts to seem a bit over complicated, and makes configuring PHPCS locally more difficult. Instead, we might want to prefer updating our workflow action to configure the text_domain
value for each standalone plugin as an added precaution. For example, for the Auto Sizes plugin, we could run this to catch any instances of text_domains that have been copied from other bundled plugins:
npm run lint-php -- -- --runtime-set text_domain auto-sizes ./plugins/auto-sizes
Thanks @joemcgill. Jetpack use custom package for PHPCS filters for different modules:
https://github.com/Automattic/jetpack/blob/trunk/.phpcs.config.xml https://github.com/Automattic/jetpack/blob/trunk/projects/plugins/social/.phpcs.dir.xml
@joemcgill One more thought that comes to my mind is to split the current PHPCS config into a different ruleset file. Later import that ruleset with required overrides at all other places including the root phpcs.xml.dist
.
Ex: ./phpcs.ruleset.xml
(considering the default config names)
phpcs.xml.dist
./plugins/auto-sizes/phpcs.xml.dist
@thelovekesh, thank you for sharing. I've opened a draft PR for testing. Would you please take a look when you have a moment?
@mukeshpanchal27 @thelovekesh Similar to my feedback in https://github.com/WordPress/performance/issues/1012#issuecomment-1965508645, I would much prefer to not have individual config files for each plugin, as this creates a maintenance overhead. Part of the benefits of a monorepo is that the "bootstrap code" can be shared, instead of having to duplicate a file that looks almost the same in tons of projects - which also means updating n files instead of 1 file when e.g. something relevant in PHPCodeSniffer or PHPUnit changes.
I agree that there may certainly be room for individual tweaks to the config that should only apply to a single plugin. For those cases though, I much prefer the ideas outlined by @joemcgill above. At a minimum, if we create individual config files, they should mostly inherit the overall config file rather than duplicating everything it contains.
Last but not least, this also ensures our plugins follow a consistent set of requirements.
@joemcgill Regarding your idea of ./plugins/auto-sizes/phpcs.xml.dist
and ./plugins/auto-sizes/override.phpcs.xml
: Rather than having an override file, wouldn't it be possible to put the changes that are specific to the individual plugin directly into that PHPCS config file (i.e. ./plugins/auto-sizes/phpcs.xml.dist
in the example)? I think that would greatly simplify things and it would be an implementation I could get behind as it avoids duplicating a bunch of config.
@felixarntz I think what you're describing here is exactly what has been implemented in https://github.com/WordPress/performance/pull/1002. There is a root phpcs.ruleset.xml
ruleset which is reused by all the plugins. They import it but then add their own specific overrides, which is currently only the text domain.
Thanks @westonruter, I just spotted the PR and noticed it's what I was suggesting :)
@mukeshpanchal27 @thelovekesh In that case, all good 👍
Fixed in #1002
Follow-up: https://github.com/WordPress/performance/pull/972#discussion_r1480801832
Currently, our project utilizes a single
phpcs.xml.dist
file containing text domains for multiple plugins. However, this setup poses a risk of accidental misuse of text domains, especially during copy/paste operations between plugins. To mitigate this risk and enhance code quality, we propose maintaining separatephpcs.xml.dist
files for each plugin.cc. @joemcgill