Separate phpcs.xml.dist Files for Each Plugin to Isolate Text Domains

[mukeshpanchal27]

Follow-up: #972 (comment)

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 separate phpcs.xml.dist files for each plugin.

cc. @joemcgill

[joemcgill]

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

[mukeshpanchal27]

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

[thelovekesh]

@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)

Config

<?xml version="1.0"?>
<ruleset name="WordPress Coding Standards for Performance Plugin">
	<description>Sniffs for WordPress plugins, with minor modifications for Performance</description>

	<rule ref="PHPCompatibility"/>
	<config name="testVersion" value="7.0-"/>

	<rule ref="WordPress-Docs"/>
	<rule ref="WordPress-Extra" />
	<rule ref="WordPress.WP.I18n"/>

	<arg value="ps"/>
	<arg name="extensions" value="php"/>

	<!-- Do not require file headers on generated files -->
	<rule ref="Squiz.Commenting.FileComment.WrongStyle">
		<exclude-pattern>default-enabled-modules.php</exclude-pattern>
		<exclude-pattern>module-i18n.php</exclude-pattern>
	</rule>

	<!-- Other rules -->

	<rule ref="SlevomatCodingStandard.Functions.StaticClosure" />
</ruleset>

phpcs.xml.dist

Config

<?xml version="1.0"?>
<ruleset name="WordPress Coding Standards for Performance Lab Auto Sizes Plugin">

	<rule ref="./phpcs.ruleset.xml"/>

	<config name="text_domain" value="performance-lab,default,auto-sizes"/>

	<file>./admin</file>
	<file>./load.php</file>
	<file>./modules</file>
	<file>./server-timing</file>
	<file>./tests</file>
</ruleset>

./plugins/auto-sizes/phpcs.xml.dist

Config

<?xml version="1.0"?>
<ruleset name="WordPress Coding Standards for Performance Plugin">

	<rule ref="./phpcs.ruleset.xml"/>

	<config name="text_domain" value="default,auto-sizes"/>

	<file>.</file>
</ruleset>

[mukeshpanchal27]

@thelovekesh, thank you for sharing. I've opened a draft PR for testing. Would you please take a look when you have a moment?

[felixarntz]

@mukeshpanchal27 @thelovekesh Similar to my feedback in #1012 (comment), 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.

[westonruter]

@felixarntz I think what you're describing here is exactly what has been implemented in #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.

[felixarntz]

Thanks @westonruter, I just spotted the PR and noticed it's what I was suggesting :)

@mukeshpanchal27 @thelovekesh In that case, all good 👍

[mukeshpanchal27]

Fixed in #1002