[3.0.0] Sniff wish list

[jrfnl]

@diedexx, @enricobattocchi and me have had a brainstorm session in the office this week to validate previously made decisions about code style and best practices and to come up with a list of things we'd like to start enforcing now support for PHP < 7.2 has been dropped in the plugins.

The below is (very long) list of things which were approved during this meeting.

Not everything is "sniffable" and even when it is, some things require new sniffs to be written, so it may be (quite) a while before everything in this list has been actioned.

All in all, this ticket is a mix of an action list and a way to keep track of the decisions taken, even if not all of those can be enforced yet.

Planning

Work on YoastCS 3.0.0 will start after WordPressCS 3.0.0 has been released.

Those action items which can be actioned straight away will be included in YoastCS 3.0.0.

Everything else can be included in YoastCS 3.0.0 if available by then, or in later releases when prerequisites have been fulfilled.

Also see the older YoastCS 3.0.0 roadmap ticket.

Impact

A reasonable number of new rules are in the background already being enforced. For others, a significant amount of work will need to be done to clean up the code bases to comply.

We are fully aware of this and this work will be done over time.

The Free, Premium, Local and Video plugins currently already use a threshold mechanism and those threshold will initially be raised to allow for the new violations until they have been fixed up.

For the other plugins, it will be decided on a case by case basis whether they will be cleaned up in one go or whether the threshold mechanism will be added to these for the time being.

The list

Names in square brackets after a rule indicate the package a sniff exist in or should be added to.

No action needed

The following looks to already been included in WPCS since a long time:

• For functions which have type declarations, verify the type declaration against the documented type in the docblock. [PHPCS]

The following looks to have already been activated in YoastCS 2.2.0:

• The operator between conditions in multi-line conditions should always be at the start of the (next) line. [PHPCS]

Sniffs covering the below rules have been included in WPCS 3.0 (and this wasn't necessarily clear yet at the time this list was made).

• No reserved keyword in param names. [PHPCSExtra]
• Forbid unused function parameters (after last used and only when the method doesn't overload a parent method) [PHPCS]
• Forbid using nested dirname() function calls, use the $levels parameter instead. [PHPCSExtra]
• Flag duplicate array keys. [PHPCSExtra]
• Flag useless late static binding in final class. [PHPCSExtra]
• Encourage use of self:: over Classname::. [PHPCS]

Non-sniff actions needed

• [ ] Announce that the Yoast plugins explicitly do NOT support the use of the PHP 8.0+ function calls using named parameters functionality.

Rules for which sniffs are already available and which will be included in YoastCS 3.0

• [x] Forbid import use statements for classes from the same namespace. [Slevomat]
• [x] Add import use statements for classes which are only used in docblocks. [Slevomat] #201
• [x] Forbid unused use statements. [Slevomat]
• [x] Import use statements should be ordered alphabetically (per group). [Slevomat] #213
• [x] Import all used classes using import use statements. [Slevomat]
• [x] For namespaced functions/constants: import the namespace, not the function/constant. Enforceable via disallowing use function and use constant statements. [PHPCSExtra] #214 / #215
• [x] Classes should have a consistent element order. [Slevomat] The order should be:
  • Trait use statements
  • Constant declarations
  • Property declarations
  • Method declarations.
• [x] Enforce all class constants to have visibility declared. [Slevomat] Initial auto-fix everything to public.
• [x] Use fully qualified names for global functions/constants used in namespaced files. [Slevomat]
• [x] Forbid the use of alternative control structure syntax, except when there is inline HTML nested within the control structure body. [PHPCSExtra]
• [x] Forbid long closures, in that case named functions should be used. [PHPCSExtra] (with the sniff default settings)
• [x] Use modern classname references, i.e. self::class, Name::class combined with use statements instead of string names. [Slevomat, move to PHPCSExtra when that version is available]
• [x] Filters should always return a value. [VIPCS]
• [x] Disallow the logical and and or operators (without exceptions). [PHPCSExtra]
• [x] Disallow double not, i.e. !!. [PHPCSExtra 1.2.0]
• [x] The concatenation operator for multi-line concatenated text strings should always be at the start of the (next) line. [PHPCSExtra 1.2.0]
• [x] Require nullable type for parameters with a null default value or where null is mentioned as one of the allowed types in the docblock. [??]
• [x] Disallow implicit array creation. [Slevomat] Note: The sniff needs a bug fix to take global statements into account.
• [x] Static closures should be declared as static. [Slevomat]
• [x] Forbid declaring variables which are subsequently never used. [VariableAnalysis]
• [x] Flag use of undefined variables. [VariableAnalysis] Select directories, like the views directory, may need to be excluded.
• [x] Forbid multiple consecutive blank lines within a function. [PHPCS]
• [x] Docs: Enforce correct alignment in docblocks, i.e. @param type $var Description to be aligned across lines. [PHPCS] This will likely give some issues with WP array format annotations, but those aren't used that frequently in the Yoast codebases.
• [x] Docs: When there are multiple types, null should always be listed last, i.e. typeA|typeB|null. [?]
• [x] Docs: Forbid use of mixed as data type, except in the test suites. [Slevomat] - #196
• [x] Docs: Forbid plain array types. These should be made more specific. [Slevomat]
• [x] Tests: All test files should be namespaced. [PHPCS]
• [x] Tests: every class should be either abstract (TestCase) or final (Tests/fixtures). [PHPCSExtra]

Rules for which a sniff would need to be written

• [ ] Functions hooked into an action are not allowed to return a value. [WPCS]
• [ ] Forbid using parameter type declarations on functions which hook into filters/actions or are used as callbacks for shortcodes, settings validation etc. [WPCS]
• [ ] Require multi-line conditions if there are more than 2 conditions in a control structure. [PHPCSExtra]
• [ ] Disallow the use of empty(). [PHPCSExtra] Future scope: allow it only when combined in the same condition with an is_array() check or if applied to a variable which was received as a function parameter with an array type declaration and the variable hasn't been touched between receiving it and the call to empty().
• [ ] Forbid duplicate import use statements. [WebImpress/PHPCSExtra ?]
• [ ] Use the correct case for PHP native classes. [WebImpress/PHPCSExtra ?]
• [ ] Use the correct case for PHP native functions. [PHPCSExtra]
• [ ] Forbid anything from being returned by functions with a void or never return type. [PHPCSExtra] There may be a sniff for this already, needs checking.
• [ ] Don't reference Throwable/Exception/Error in catch statements. [PHPCSExtra] Prefer more specific over less specific. They are allowed to be used, but only as a secondary separate catch statement, not as the primary catch, nor in multi-catch statements. There may be a sniff for this already, needs checking.
• [ ] Only allow Throwable/Exception/Error in type declarations when referring to exceptions. [PHPCSExtra]
• [ ] Docs: Forbid the use of @inheritDoc. [?]
• [ ] Tests: Forbid using strict_types in test files. [PHPCSExtra]
• [ ] Tests should always use the strictest assertion possible. [PHPUnitQA]
• [ ] Test method naming conventions: Data provider method names must start/be prefixed with either data or provide. [PHPUnitQA]
• [ ] Tests naming conventions: base classes from which test classes extend must be declared as abstract and the class name must be suffixed with TestCase. [PHPUnitQA] Note: the class name should not be prefixed with Abstract.
• [ ] Tests: all data provider methods should be declared as static (for compatibility with PHPUnit 10). [PHPUnitQA]

Rules which cannot be enforced via sniffs in the near future (or ever), but which have manual tasks attached which should be actioned

• [ ] Every structural element at the "top" level, i.e. classes, global/namespaced functions, filters/actions etc, should be marked with either @api or @internal to indicate whether they are part of the public API or not. [YoastCS] A @since tag should be added indicating in which version the designation was added, i.e. @since 20.2.1 This class is now internal. Everything in test directories should be explicitly excluded from this rule. Follow up plan:
  • Once all elements have been marked with either @api or @internal, blogposts/release notes/changelogs should start drawing attention to this and announce that use of anything @internal from outside the plugin is now deprecated.
  • Once a reasonably period of time has passed, it will be allowed to make breaking changes in elements marked with @internal, like adding return type declarations in non-private methods and adding strict-types declarations.
• [ ] Tests naming conventions: Tests classes should be named after what they are testing. Examples:
  • For a class testing a specific other class, the test class should be called ClassUnderTestTest.
  • For a class testing a specific method in a class, the test class should be placed in a ...\ClassUnderTest namespace and the test class should be called MethodUnderTestTest.
• [x] Filter doc blocks should use @param for their arguments instead of @api. [WPCS] For now a one-time search & replace should fix any remaining instances.
• [x] Tests: the test suite should be made compatible with PSR4 (file names). [?] Note: Once done, PSR4 should also be used for the autoloading

Rules which need further investigation

• [ ] Do not allow inline @var docblocks, but require assertions - assert() statements - instead. [Slevomat]
  • Investigation is needed on how we can prevent issues for end-users of the plugin which may be on a misconfigured PHP server with zend.assertions not set to -1.
  • Investigation is needed on how we can prevent any options which we may need to set to work around the first issue affecting assertions which don't come from the Yoast plugins.
  • Repos which do not publish distributable packages (Shopify, platform) can enable the rule in their local PHPCS ruleset and don't need to wait for the above investigations.
• [ ] Check the impact of possibly strict enforcing the order of constants/properties/methods in classes to follow visibility public - protected - private. This could possibly be checked by adding metric to the Slevomat sniff ?
• [ ] Maybe enable the Slevomat UselessParameterDefaultValue sniff.

[jrfnl]

During the hackathon, the suggestion was raised by multiple people to rename the "integration-tests" directories to something more representative of what's inside. I fully concur and propose to call those "wp-unit-tests". I suggest we do this directory rename at the same time as the namespacing and PSR4 naming of those tests.

[jrfnl]

I'm going to leave this ticket open for now and move it out of the 3.0 milestone.

A significant number of the action items here have been actioned and can be considered fixed with the release of YoastCS 3.0.

However, there is still more to be done and this ticket contains lots of information about future scope action items too.