wearerequired / coding-standards Goto Github PK

EditorConfig helps developers define and maintain consistent coding styles between different editors and IDEs. This way it's easier to follow the given coding standards in a project, without the need to fiddle with IDE settings.

It would be great if we could add a section about this and perhaps even a suggestion for a .editorconfig to the README. This could be the reference for all new projects. We'd add it to our default project repository, too.

Example .editorconfig:

root = true

[*]
charset = utf-8
indent_style = tab
end_of_line = lf
insert_final_newline = true
trim_trailing_whitespace = true

[{*.md}]
trim_trailing_whitespace = false

[{*.json,*.yml,.prettierrc,.babelrc}]
indent_style = space
indent_size = 2

See https://eslint.org/docs/8.0.0/user-guide/migrating-to-8.0.0.

Depends on WordPress/gutenberg#36283 (already fixed) and a new version of @wordpress/eslint-plugin.

Rule: https://stylelint.io/user-guide/rules/function-url-quotes

    "function-url-quotes": [
      "always", {
        "except": [  "empty" ]
     }
    ]

The current <exclude-pattern>*.min.*</exclude-pattern> also matches the path of my home directory /Users/Dominik/... which means no files are processed.

I think we can remove that line since we only want to process PHP files:

This is an important release which makes significant changes to improve the accuracy, performance, stability and maintainability of all sniffs, as well as makes WordPressCS much better at handling modern PHP.

See https://make.wordpress.org/core/2023/08/21/wordpresscs-3-0-0-is-now-available/ and https://github.com/WordPress/WordPress-Coding-Standards/releases/tag/3.0.0

When running phpcs without a location your get this:

phpcs
E 1 / 1 (100%)

FILE: /Users/Dominik/Development/wearerequired/git/coding-standards/Required/bootstrap.php

Looks like <file>.</file> should only be defined in the project config.

Lerna is a tool for managing JavaScript projects with multiple packages. With eslint-config and stylelint-config we now have two projects. All configs, including PHCS, should always have the same version. Lerna will help us help us with that.

Previously: #23

Currently we're using 7.1 which is EOL. The same is true for 7.2 and active support for 7.3 ends in 4 days.
Therefore, and also as a preparation for 8.0, I'd like us to increase the minimum PHP version to 7.4.

This change should probably be go in with #35 and #58 and other type related rules.

Related: The Slevomat Coding Standard will also drop support for 7.1 soon, see slevomat/coding-standard#1082.

Thoughts? Feedback? Questions?

Once WordPress/gutenberg#22777 is done we should switch from stylelint-config-wordpress to @wordpress/stylelint-config.

I'm proposing to add add this exclude:

    <!-- Documentation is organized with the use of namespaces. -->
    <rule ref="WordPress-Docs">
        <exclude name="Squiz.Commenting.FileComment.MissingPackageTag"/>
    </rule>

To make the package PHPCompatibility/PHPCompatibility, which is a dependency of PHPCompatibility/PHPCompatibilityWP, work with PHP 8.2, the developers suggest using the develop-Branch as version 9.9.99 until there is a new release:

• PHPCompatibility/PHPCompatibility#1468
• PHPCompatibility/PHPCompatibility#1653

File: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/blob/develop/WordPress-Docs/ruleset.xml

See #2 (comment):

Also, at least to some degree I want to include the WordPress-Docs ruleset as well. Our inline docs are often times really inconsistent, ranging from superb to non-existent.

Rule: https://stylelint.io/user-guide/rules/font-family-no-missing-generic-family-keyword

    "font-family-no-missing-generic-family-keyword": [
      true,
      {
        "ignoreFontFamilies": [ "dashicons" ]
      }
    ],

Deprecation Warning: 'declaration-property-unit-whitelist' has been deprecated. Instead use 'declaration-property-unit-allowed-list'. See: https://github.com/stylelint/stylelint/blob/13.7.0/lib/rules/declaration-property-unit-whitelist/README.md

The rule is defined in stylelint-config-wordpress and is already fixed since WordPress-Coding-Standards/stylelint-config-wordpress#267 but no new version has been release yet, see WordPress-Coding-Standards/stylelint-config-wordpress#268.

The warning can be removed by using

'declaration-property-unit-whitelist': null, // Set to null to disable rule and to avoid deprecation warning.
'declaration-property-unit-allowed-list': {
	'line-height': [ 'px' ],
},

This works in CLI and VSCode, but Atom for some reason produces an Expected option value for rule "declaration-property-unit-allowed-list" 1:1 error.

• .eslintrc.js instead of .eslintrc
• stylelint.config.js instead of .stylelintrc

This allows to validate/lint them with our ESLint rules.

Follow-up to #32.

Another rule for type hints: https://github.com/slevomat/coding-standard#slevomatcodingstandardtypehintstypehintdeclaration-

	<!-- Checks for missing property types, missing type hints . -->
	<rule ref="SlevomatCodingStandard.TypeHints.TypeHintDeclaration">
		<exclude name="SlevomatCodingStandard.TypeHints.TypeHintDeclaratiaon.MissingTraversableReturnTypeHintSpecification"/>
		<exclude name="SlevomatCodingStandard.TypeHints.TypeHintDeclaration.MissingTraversableParameterTypeHintSpecification"/>
	</rule>

I disabled hints for traversable types as it would mean we have to use array<string> instead of string[] which what the WordPress core is using. The rule also requires to use : void for functions without a return value. Do we want this? The good thing is that phpcbf can again fix them all automatically.

The WordPress coding standard for docs includes the Squiz.Commenting.FunctionCommentThrowTag rule by default. This ensures that a @throws tag is added to the DocBlock otherwise you get Missing @throws tag in function comment (Squiz.Commenting.FunctionCommentThrowTag.Missing).

Once added this currently triggers another error: Incorrect order of annotations groups. (SlevomatCodingStandard.Commenting.DocCommentSpacing.IncorrectOrderOfAnnotationsGroup). This is due to #62.

@wearerequired/developers Should @throws tags be added to the DocBlock? If yes, in which group should they be added?

WPCS 1.0 (see #8) sets the minimum supported WordPress version to 4.6. I think we should make this at least 4.8.

If a project of ours (e.g. an open source plugin) needs to lower this, they can override it. But for internal stuff I want to make this as high as possible.

We should make it as easy as possible for every team member to integrate coding standards check into their workflow.

There's nothing more disappointing than creating a PR and seeing hundreds of CS errors pop up.

Things that should be considered:

• EditorConfig #1
• PhpStorm Integration
• Visual Studio Code Integration
• Atom Integration
• Pre-Commit Hooks
• Continuous Integration

All things that can be documented or made easier with tooling.

Worth noting: Human Made have CLI commands to run coding standards and apply these to older projects: https://www.npmjs.com/package/@humanmade/cli

From the documentation standards:

@return: Should contain all possible return types, and a description for each. Use a period at the end. Note: @return void should not be used outside of the default bundled themes.

We should add a rule to enforce this.

Example file: inc/Blocks/namespace.php

With current rules:

$ phpcs inc/Blocks/namespace.php
Time: 11.11 secs; Memory: 12MB

Without any SlevomatCodingStandard rules:

$ phpcs inc/Blocks/namespace.php
Time: 511ms; Memory: 12MB

This is also noticeable in VS Code where checks are delayed and two PHP processes constantly running with high CPU usage.

@wearerequired/eslint-config should be updated to support ESLint 7.x

@wordpress/eslint-plugin isn't updated yet but I'm also working on that, see WordPress/gutenberg#22771.

squizlabs/PHP_CodeSniffer has been abandoned, see squizlabs/PHP_CodeSniffer#3932. It has been forked to https://github.com/PHPCSStandards/PHP_CodeSniffer.

From https://github.com/sirbrillig/phpcs-variable-analysis:

Plugin for PHP_CodeSniffer static analysis tool that adds analysis of problematic variable use.

• Performs static analysis of variable use.
• Warns on use of undefined variables.
• Warns if variables are set or declared but never used within that scope.
• Warns if variables are redeclared within same scope.
• Warns if $this, self::$static_member, static::$static_member is used outside class scope.

As a first step it should be based on https://www.npmjs.com/package/@wordpress/eslint-plugin.

Previously: https://github.com/wearerequired/eslint-config

Currently when I install our coding standard globally and run phpcs -i our coding standards is named as coding-standards which is not very helpful.

Currently we have <config name="testVersion" value="5.6-"/>. I would like to increase the default to 7.0. I would prefer to set it down 5.6 as the exception.

If all are agreed I can create a PR.

The default for the Arrays.MultipleStatementAlignment sniff is 1000 but the WordPress-Core setting for this property is 60. That means when an array key + indentation is longer than 60 chars, the arrows aren't aligned anymore.

We had this problem in one of our projects while testing out our custom rulesets. I think we should increase this to something like 100, or even back to the default of 1000.

<rule ref="WordPress.Arrays.MultipleStatementAlignment">
	<properties>
		<property name="maxColumn" value="100" />
	</properties>
</rule>

Documentation: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/wiki/Customizable-sniff-properties#array-alignment-maximum-column

See https://stylelint.io/ and https://github.com/WordPress-Coding-Standards/stylelint-config-wordpress.

PHPCompatibility is a set of sniffs for PHP CodeSniffer that checks for PHP version compatibility.

The PHPCompatibilityWP ruleset is based on PHPCompatibility, but specifically crafted to prevent false positives for projects which expect to run within the context of WordPress, i.e. core, plugins and themes.

We should add it to our custom ruleset as finding these kinds of issues is very helpful, especially for our open source plugins.

Explanation by WPCS: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards#recommended-additional-rulesets

Idea:
For PRs, detect coding standard violations via PHP_CodeSniffer. Errors should prevent the PR from being merged and shown to the user.

Implementation:

• A GitHub Action to set up PHP with Composer and to run phpcs
• To prevent the branch from being merged, use protected branches with phpcs as a required status check.
• Display errors via PR annotations in Checks via cs2pr

Result:

name: PHP_CodeSniffer

on: pull_request

jobs:
  phpcs:
    runs-on: ubuntu-latest
    steps:
      - name: Checkout
        uses: actions/checkout@v2

      - name: Setup PHP
        uses: shivammathur/setup-php@v2
        with:
          php-version: '7.3'
          coverage: none
          tools: composer, cs2pr

      - name: Get Composer cache directory
        id: composer-cache
        run: |
          echo "::set-output name=dir::$(composer config cache-files-dir)"

      - name: Setup cache
        uses: pat-s/[email protected]
        with:
          path: ${{ steps.composer-cache.outputs.dir }}
          key: ${{ runner.os }}-composer-${{ hashFiles('**/composer.lock') }}
          restore-keys: |
            ${{ runner.os }}-composer-

      - name: Install dependencies
        run: composer install --prefer-dist --no-suggest --no-progress

      - name: Detect coding standard violations
        run: vendor/bin/phpcs -nq --report=checkstyle | cs2pr

Limits:

• Only 10 warning annotations and 10 error annotations per step (source)

See zend_try_compile_special_func() in https://github.com/php/php-src/blob/PHP-7.4/Zend/zend_compile.c for the list of special functions. The functions will be replaced by opcodes at compile time, but only if it's clear that the global native functions will be used.

Related: https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/v2.16.1/src/Fixer/FunctionNotation/NativeFunctionInvocationFixer.php

See WordPress/WordPress-Coding-Standards#403

Something like

/** @var \Required\Foo\bar $bar */
$bar = bar();

returns a Missing short description in doc comment (Generic.Commenting.DocComment.MissingShort) error.

This quite annoying since the code should be valid. I think we should exclude the rule for now or maybe lower it to a warning?

We have this rule:

	<rule ref="WordPress-Extra">
		<!-- _e() and _ex() are considered safe for internal translations -->
		<exclude name="WordPress.Security.EscapeOutput.UnsafePrintingFunction" />
		
	</rule>

But __() and _e() are still causing a warning.

File: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/blob/develop/WordPress-Extra/ruleset.xml

We should use the more stricter ruleset which is defined in WordPress-Extra. But not that strict like escaping all translations. Those should be excluded.

I'm proposing to borrow the sniffs from the PSR-2-R standard: https://github.com/php-fig-rectified/psr2r-sniffer/tree/master/PSR2R/Sniffs/Namespaces

• NamespaceDeclarationf: Ensures namespaces are declared correctly. (Already part of WordPress-Extra?)
• UnusedUseStatement: Checks for "use" statements that are not needed in a file.
• UseInAlphabeticalOrder: Ensures all the use are in alphabetical order.
• UseDeclaration : Ensures "use" blocks are declared correctly.

The rule to add:

	<!-- Allow the use of filesystem functions. -->
	<rule ref="WordPress.WP.AlternativeFunctions">
		<properties>
			<property name="exclude" type="array">
				<element value="file_get_contents"/>
			</property>
		</properties>
	</rule>

We don't use the class-xy.php file name format in our plugins, so we don't need that sniff.

<rule ref="WordPress.Files.FileName">
	<properties>
		<property name="strict_class_file_names" value="false" />
	</properties>
</rule>

See https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/wiki/Customizable-sniff-properties#disregard-class-file-name-rules

Found another ruleset at https://github.com/slevomat/coding-standard which has some rules (and fixers) that I'd like to introduce in our ruleset to enhance linting of PHP >= 7.0 code.

Type Hits

	<!-- Checks that there's no whitespace between a nullability symbol and a typehint. -->
	<rule ref="SlevomatCodingStandard.TypeHints.ParameterTypeHintSpacing"/>
	<!-- Checks whether the nullablity ? symbol is present before each nullable and optional parameter. -->
	<rule ref="SlevomatCodingStandard.TypeHints.NullableTypeForNullDefaultValue"/>
	<!-- Enforce no space between closing brace and colon of return typehint. -->
	<rule ref="SlevomatCodingStandard.TypeHints.ReturnTypeHintSpacing">
		<properties>
			<property name="spacesCountBeforeColon" value="0"/>
		</properties>
	</rule>

These align with the official coding standards.

Namespaces

This will be a replacement for PSR2.Namespaces and PSR2R.Namespaces.

	<!-- Disallows grouped use declarations. -->
	<rule ref="SlevomatCodingStandard.Namespaces.DisallowGroupUse"/>
	<!-- Disallows leading backslash in use statement. -->
	<rule ref="SlevomatCodingStandard.Namespaces.UseDoesNotStartWithBackslash"/>
	<!-- Enforces fully qualified names of classes and interfaces in annotations. -->
	<rule ref="SlevomatCodingStandard.Namespaces.FullyQualifiedClassNameInAnnotation"/>
	<!-- Checks whether uses at the top of a file are alphabetically sorted. -->
	<rule ref="SlevomatCodingStandard.Namespaces.AlphabeticallySortedUses"/>
	<!-- Enforces one space after namespace, disallows content between namespace name and semicolon and disallows use of bracketed syntax. -->
	<rule ref="SlevomatCodingStandard.Namespaces.NamespaceDeclaration"/>
	<!-- Prohibits multiple uses separated by commas. -->
	<rule ref="SlevomatCodingStandard.Namespaces.MultipleUsesPerLine"/>
	<!-- Enforces one blank line before and after namespace. -->
	<rule ref="SlevomatCodingStandard.Namespaces.NamespaceSpacing">
		<properties>
			<property name="linesCountBeforeNamespace" value="1"/>
			<property name="linesCountAfterNamespace" value="1"/>
		</properties>
	</rule>
	<!-- Enforces one blank before first use, after last use and none between two different types of use. -->
	<rule ref="SlevomatCodingStandard.Namespaces.UseSpacing">
		<properties>
			<property name="linesCountBeforeFirstUse" value="1"/>
			<property name="linesCountBetweenUseTypes" value="0"/>
			<property name="linesCountAfterLastUse" value="1"/>
		</properties>
	</rule>
	<!-- Prohibits uses from the same namespace. -->
	<rule ref="SlevomatCodingStandard.Namespaces.UseFromSameNamespace"/>

See https://github.com/stylelint/stylelint/blob/14.0.0/docs/migration-guide/to-14.md for the changes introduced in v14.

Depends on WordPress/gutenberg#38091 and a new version of @wordpress/stylelint-config.

• https://github.com/hudochenkov/stylelint-order#stylelint-order
• https://github.com/hudochenkov/stylelint-order#example-configs

WPCS 1.0.0 is scheduled to be released on July 2nd, see WordPress/WordPress-Coding-Standards#1388.

That's a great opportunity for this project to adopt a specific version.

Tasks needed:

• Rename some rules, see #6 (comment)

Our stylelint-config basically works with SCSS files but there are some errors when it comes to at-rules like @include or @mixin. I was able to fix them by customizing the at-rule-no-unknown rule:

module.exports = {
	extends: [ '@wearerequired/stylelint-config' ],
	rules: {
		'at-rule-no-unknown': [ true, { ignoreAtRules: [ 'extend', 'mixin', 'include', 'function', 'return' ] } ],
	},
};

I'm still checking if we'd need more changes or if the above is the only required change. Since we don't have many projects using Sass we could also close this as wontfix and remove the (S) bit from the config name.

Related: https://github.com/WordPress-Coding-Standards/stylelint-config-wordpress/blob/master/scss.js

Option 1:

sprintf(
	'Foo %s',
	'bar',
);

Sniffs:
SlevomatCodingStandard.Functions.RequireTrailingCommaInCall
SlevomatCodingStandard.Functions.RequireTrailingCommaInClosureUse
SlevomatCodingStandard.Functions.RequireTrailingCommaInDeclaration

Option 2:

sprintf(
	'Foo %s',
	'bar'
);

Sniffs:
SlevomatCodingStandard.Functions.DisallowTrailingCommaInCall
SlevomatCodingStandard.Functions.DisallowTrailingCommaInClosureUse
SlevomatCodingStandard.Functions.DisallowTrailingCommaInDeclaration

Related: https://php.watch/versions/8.0/trailing-comma-parameter-use-list

Rule: https://stylelint.io/user-guide/rules/at-rule-no-unknown

@define-mixin and @mixin are used by PostCSS Mixins, one of our default PostCSS plugins.