As the autoload setting has been added in a later state, the argument is not required.
Though this is a setting that should be set explicitly, as it impacts performance and memory.

The convention for the Yoast plugins is to have deprecation methods at the end of the class they are in (preceded by a /* *** DEPRECATED METHODS *** */ placeholder) and for deprecated global functions to be at the end of the file or in a separate deprecated_functions.php file.

A sniff could be written which checks for this and warns or throws an error if a method is not moved to the end of the class/file when being deprecated.

Discussed on Slack with @herregroen

Add a sniff to enforce that import use statements are sorted alphabetically per group, i.e. use ..., use func ... and use const ....

There is a Slevomat sniff available which we could probably use for this. Needs review & testing.

The current ruleset - in contrast to WPCS - already does not demand Yoda conditions.
All the same, they are not explicitly forbidden either.

PHPCS 3.5.0 introduces a new Generic.ControlStructures.DisallowYodaConditions sniff which can throw an error when the use of Yoda conditions is detected.

I'd like to suggest adding this sniff to YoastCS to improve consistency in how conditions are written.

I noticed that the PHPCompatibility ruleset has been disabled as it - at the time - did not (yet) support PHPCS 2.x. This has been fixed for over a year now and since then a fairly complete suite of tests has been added to the PHPCompatibility library for PHP 7.

I'd like to suggest re-enabling it.

To that end, I've set up the basics for this in a feature branch, but this will need testing, preferably by someone a lot better versed in Composer than me ;-) hint hint nudge nudge

See: https://github.com/Yoast/yoastcs/tree/feature/re-enable-phpcompatibility

As discussed with @moorscode and by @moorscode discussed with @omarreiss and @atimmer, I'm working on a YoastCS native filename sniff.

There are a couple of things I need to get a decision on before the sniff can be finalized, so Jip, Omar, Anton, please discuss and let me know the outcome.

Currently implemented rules:

• (WP) Filenames should be lowercase and words should be separated by dashes (not underscores).
• All class files should only contain one class (enforced by another sniff in WPCS) and the file name should reflect the class name without the plugin specific prefix.
• All interface and trait files should only contain one interface/trait (enforced by another sniff in WPCS) and the file name should reflect the interface/trait name without the plugin specific prefix and with an -interface or -trait suffix.
• Files which don't contain an object structure, but do contain one or more function declarations should have a -functions suffix.

Currently implemented exception:

• It will be possible to pass one or more file names to the sniff to exclude.
  This has been done to allow for "main" plugin files which may contain functions, but don't have the -functions suffix. Renaming the file would lead to deactivation of the plugin on upgrade which is probably not a good idea.

Open questions:

• For the file names to be excluded, should this just be a file name or should this be a path from the root of a plugin and strictly checked against the path ? If it's just a file name, you run the risk that another file in another directory would use the same name and be excluded from this sniff as well.
• Should this sniff be set up to be able to also check the filenames of CSS and JS files ?
• Should the "functions" rule be applied when a file only contains functions (no side-effects) or also when the file contains functions as well as miscellaneous code ?
  Think: WPSEO Free admin/pages/metas.php.
• What about interfaces/classes/traits which have the word Interface/Class/Trait in the object name ?
  Think: WPSEO Free WPSEO_Sitemap_Cache_Data_Interface
• Should there be a rule about abstract classes ? Should they have an abstract prefix/suffix or should this just be the same as the class name ?
• Keeping in mind the intention to move to a PSR based namespace folder structure - should a potential overlap with the directory name a class lives in be removed ?
  Think: WPSEO Free class WPSEO_Metabox_Tab_Section which currently lives in file admin/metabox/class-metabox-tab-section.php:
  Should this become metabox-tab-section.php as per the above rules or should this become tab-section.php ?
  And if the latter: what about classes which are named the same as their directory path ? Think WPSEO_Tracking in admin/tracking.
• Test class files, now all start with test-class. For PHPUnit, having the test- prefix (or a -test suffix) is needed.
  • Should the class- bit stay or go ?
  • Should the filename match the class name more closely ? The order is reversed at the moment.
    Class names for test classes generally follow the following pattern: Prefix_Classname_Test with the Prefix and the Test suffix being fairly consistently applied and Classname reflecting the name of the class under test.
    File names for test classes generally follow the following pattern at this time: test-class-classname.php.
    Using the "normal" rules for classes, the file name should/would become: classname-test.php.
    Example:
    - Current file name: test-class-redirect-table.php
    - Class name: WPSEO_Redirect_Table_Test
    - Expected file name using "normal" rules: redirect-table-test.php
  • In other word; how would you like me to handle test files ?
  • And what to do about test classes which are inconsistently named ? Think: Test_Yoast_Notification.
• And what about test Mock/Double classes ? Should they follow the file name rules for "normal" classes ?

Reading back over my own questions, I'm thinking that maybe there should be a YoastCS ClassName sniff as well to prevent some of the classname inconsistencies I've listed about.

Maybe something along the lines of:

• Don't use interface, class, trait or abstract in the object name.
• OO Structures in the plugin should follow the following pattern: Prefix_[Optional:Subdir_]Classname where a couple of "basepaths" can be taken into account, such as admin or premium which don't need to be in the name.
• Test OO Structures do not need a prefix (as they won't be shipped with the plugin) and should follow the following pattern: [Optional:Subdir_]Classname_Test.

What do you think ?

It won't be much work to write this sniff, providing the specs are clear.

Attachments

Current file rename proposal based on the above:
wpseo-file-rename-proposal.xlsx

Notes:

• Take note of the two tabs - one for the WPSEO Free plugin, one for the Premium plugin.
• For this list, I've taken two class prefixes into account: WPSEO and Yoast.
• For the Free plugin, the list will not be entirely correct yet as there are still 26 files (2 in the plugin, 24 in the tests) in Free which contain two or more classes and need to be split.
• For the Premium plugin, the above mentioned split has already been done.

Raw data in CSV format in case you want to have a play with it (.csv files can't be uploaded here, so renamed to txt):
wpseo-free-file-rename.txt
premium-file-rename.txt

As the default is RAW INPUT, this does not add anything.

This is an issue with a high-level write-up and proposal for YoastCS 3.0.0, so the intentions for YoastCS 3.0.0 are documented and can be discussed.

TL;DR

New minimum requirements:

• PHP 7.2 🆕
• PHP_CodeSniffer 3.5.4 3.8.0
• WordPressCS 3.0.0
• PHPCompatibility 10.0.0

Add new dependencies and implement use of/selectively add rules from:

• PHPCSUtils
• PHPCSExtra
• VariableAnalysis
• SlevomatCodingStandard

This includes removing some YoastCS native sniffs in favour of sniffs from the above mentioned standards, including WPCS.

Note: New rule additions will be pulled in separate PRs so these proposals can be reviewed and evaluated individually.

The planning is for YoastCS 3.0.0 to be released soon after WPCS 3.0.0 and PHPCompatibility 10.0.0 have been released.

Developments in the wider PHP_CodeSniffer sphere

PHPCSUtils

The new PHPCSUtils library offers a number of sniff helper utilities and PHPCS cross-version compatibility utilities which can be useful for some of the sniffs in YoastCS.

PHPCSUtils has been set up to support and recognize all PHP syntaxes, including modern PHP, from namespaced code to arrow functions and is fully unit tested.

PHPCSUtils does not contain any sniffs (other than abstract base sniffs to use as building blocks).

Originally the intention was that this library would be added to PHPCS itself (without the cross-version helpers), but that bounced off.

PHPCSUtils supports PHPCS 2.6.0 3.7.1 - current, including the upcoming PHPCS 4.x and has a minimum PHP requirement of PHP 5.4.

Also see: https://phpcsutils.com/

PHPCSExtra

The new PHPCSExtra standard is developed on top of PHPCSUtils and offers some new sniffs which we may want to add to YoastCS.

A few sniffs from that standard can also be used to replace custom sniffs in the YoastCS standard.

PHPCSExtra has a minimum PHPCS requirement of PHPCS 3.3.1 3.7.1 and has a minimum PHP requirement of PHP 5.4.

WordPressCS 3.0.0

WordPressCS 3.0.0 will add dependencies to both PHPCSUtils as well as PHPCSExtra.

A number of the more generically useful sniffs which are also useful outside of a WP context will be moved from WPCS to PHPCSExtra to make them more easily discoverable by the wider PHP community.

PHPCSUtils will be used to help make the WPCS sniffs better able to handle modern PHP.

WPCS 3.0.0 is expected to include quite some new sniffs, either WPCS native or from PHPCSExtra, to implement the proposals outlined in the recent Make post.

WordPressCS 3.0.0 will have a minimum PHPCS requirement of PHPCS 3.5.0 3.7.1 and has a minimum PHP requirement of PHP 5.4.

WordPressCS 3.0.0 is expected to be released some time over the next three months.

Also see: WordPressCS 3.0.0 planning and roadmap

PHPCompatibility 10.0.0

PHPCompatibility 10.0.0 will add a dependency to and implement PHPCSUtils, both for improved handling of modern PHP as well as for improved PHPCS cross-version compatibility.

PHPCompatibility 10.0.0 will have a minimum PHPCS requirement of PHPCS 2.6.0 3.7.1 and will have a minimum PHP requirement of PHP 5.4. (was 5.3).

PHPCompatibility 10.0.0 is expected to be released some time over the next three months.

Also see: https://github.com/PHPCompatibility/PHPCompatibility/milestone/26

Other interesting standards

There are a number of other interesting external standards for PHP_CodeSniffer which I have been auditing over the past few months as candidates for inclusion in YoastCS.

VariableAnalysis

The VariableAnalysis standard is an old standard abandoned by the original author and currently maintained by Payton Swick (Automattic).

This standard reports on uses of undefined variables as well as unused variables.

The standard has started implementing PHPCSUtils as well and I've been working on fixing a number of bugs and, together with the maintainer, making the historically grown code better maintainable.

VariableAnalysis has a minimum PHPCS requirement of PHPCS 3.1 3.5.4 and has a minimum PHP requirement of PHP 5.6.

ImportDetection

The ImportDetection standard - also maintained by Payton Swick - shows warnings if a symbol (function, constant, class) is used and is not defined directly, imported explicitly, nor has its namespace imported.

This library is useful, but not very configurable which makes it - at this time - not the preferred choice for inclusion in YoastCS.

Security

The Security standard is aimed at finding vulnerabilities and weaknesses related to security in PHP code.

The standard is problematic to use as it doesn't comply with some PHPCS basics (expected to be fixed in the next release).

The standard is also currently too prone to false positives to be considered for inclusion in YoastCS.

CognitiveComplexity

The CognitiveComplexity standard is a one-sniff fork of a sniff originally in the Simplify standard and improved and maintained by Rarst.

It analyses code for Cognitive Complexity metric by SonarSource and alerts if the cognitive complexity is too high.

This standard would be a useful inclusion for YoastCS, but would require code refactoring in a number of plugins to allow the code to pass the checks.

I propose to keep this standard in mind for future addition.

CognitiveComplexity has a minimum PHPCS requirement of PHPCS 3.5 and has a minimum PHP requirement of PHP 7.2.

Object Calisthenics

The Object Calisthenics standard set of rules in object-oriented code, that focuses on maintainability, readability, testability and comprehensibility.

The standard is problematic to use as it doesn't comply with some PHPCS basics.

The rules in the standard are also too fiddly for the Yoast codebases, so for that reason, this standard has been rejected.

Slevomat Coding Standard

The Slevomat Coding Standard was originally developed for the Slevomat company, but has gained a large following and is actively maintained.

The standard has (had) some issues with it not complying with some PHPCS basics making it problematic to use, but I've been fixing these and all such known issues should be fixed by the next release.

The Slevomat standard can never be used as a standard as it contains conflicting rules. Individual sniffs from the standard should be included instead.

The standard contains a lot of sniffs for modern PHP formatting and best practices and most of these sniffs are highly configurable.
The majority of sniffs from the Slevomat standard include fixers to auto-fix code which doesn't comply.

While some sniffs are buggy at times, the team behind it is very responsive and bugs are fixed promptly.

As the minimum PHP requirement for this standard is PHP 7.1 7.2, it couldn't previously be used as it would cause a conflict with the minimum requirements on Composer install, however, as some of the other dev requirements for the plugins now have higher PHP requirements too and --ignore-platform-reqs is now used by default, this standard is now a viable candidate for inclusion in YoastCS and has my recommendation.

Slevomat Coding Standard has a minimum PHPCS requirement of PHPCS 3.5.4 and has a minimum PHP requirement of PHP 7.1.

Beyond YoastCS 3.0.0

I'm working on creating a number of additional external PHPCS standards which will be candidates for inclusion in YoastCS once released.

Think along the lines of:

• PHPUnitCompatibility (helpers to upgrade PHPUnit code for higher PHPUnit versions)
• PHPUnit Best Practices (using the right assertions, using annotations correctly etc)
• PSR5 (documentation analysis based on PSR5)
• PSR19 (documentation analysis based on PSR19)
• PHPModernizer (finding opportunities to modernize code)

When a method or function is deprecated we don't want it to be present in the codecoverage, just because it's deprecated and thus not used.

We should have a sniff that checks if a @codeCoverageIgnore flag is present in the docbloc.

As discussed today with @omarreiss @herregroen @IreneStr, a new sniff needs to be created which will check that the names of classes declared within a namespace have a maximum of two underscores in them, i.e they can consist of maximum three words.

The sniff should contain separate properties for recommended (warning) and maximum (error) number of underscores.
The default will be 2 in both cases.
Individual plugins can use the properties to deviate.

Other than that, the normal WPCS class names rules (Camel_Caps, acronyms allowed) apply.

Valid code samples

class Non_Namespaced_Long_Class_Name {}

namespace Yoast\WP\Plugin;

class Short_Class_Name {}

Invalid code samples

namespace Yoast\WP\Plugin;

class Much_Too_Long_Class_Name {}

All strings with placeholders should have a translator comment to explain what the placeholders are replaced with.

This is incorrect:

$scoreBodyGoodLength = __( 'There are %d words contained in the body copy, this is more than the %d word recommended minimum.', 'wordpress-seo' );
$scoreBodyPoorLength = __( 'There are %d words contained in the body copy, this is below the %d word recommended minimum. Add more useful content on this topic for readers.', 'wordpress-seo' );
$scoreBodyOKLength   = __( 'There are %d words contained in the body copy, this is slightly below the %d word recommended minimum, add a bit more copy.', 'wordpress-seo' );
$scoreBodyBadLength  = __( 'There are %d words contained in the body copy. This is far too low and should be increased.', 'wordpress-seo' );

This is correct:

/* translators: %1$s and %2$s expand to a link to Facebook Insights */
printf( __( 'To be able to access %1$sFacebook Insights%2$s, you need to add a user here. The name is used for reference only, the ID is used for verification.', 'wordpress-seo' ), '<a target="_blank" href="https://www.facebook.com/insights">', '</a>' );
echo '</p>';
echo '<p>';
/* translators: %1$s and %2$s expand to a link to the Yoast Knowledge Base */
printf( __( 'If you don\'t know where to find the needed ID, see %1$sthis knowledge base article%2$s.', 'wordpress-seo' ), '<a target="_blank" href="http://kb.yoast.com/article/254-gaining-access-to-facebook-insights">', '</a>' );

An advanced check would also be able to check if all the placeholders present in the string are also present in the translator comment. Strings can be checked for placeholders using the following regex: %(\d+\$(?:\d+)?)?[bcdefgosuxEFGX].

This could also be added upstream in wpcs.

WPCS 0.12.0 was released last week.

A number of new sniffs were introduced and some more custom properties which YoastCS could benefit from.

Filtering SERVER_INPUT variables using filter_input is disabled on some PHP configurations. (And fails silently)

Once WPCS 0.13.0 has been released (expected soon), WPCS will be compatible with PHPCS 3.x.

To upgrade YoastCS to start using PHPCS 3.x, the custom sniff will need to be adjusted.

Sniff specification:

Goal:

Prevent functions from being declared with:

• A void return in combination with another return type. (Forbidden)
  * @return \My_Object|void
• Three or more different return types. (Refactor advised)
  * @return object|int|bool

Rationale:

Each of the above situations makes unit testing functions more difficult and should be avoided.

Notes for implementation:

• This may need to be implemented in separate sniffs checking the various points. Whether this should be one sniff or several will clarify itself once working on it.
• The sniff(s) would need to do several things:
  • Check if the function declared contains return statements and whether the type of each return can be determined.
  • Check if the function declared has a function DocBlock, if the function DocBlock contains a @return tag and if so, what the documented return types are.
  • [TO BE IMPLEMENTED LATER] Check if the function declared has a PHP 7+ return type declaration.
  • Combine and compare the results of the above two (three) checks to see if an error should be thrown.

Open questions:

• Should an explicit null return when combined in a function which can also return different types, trigger an error ?
• Is an explicit null return allowed and should it be documented as null or as void ?
• Will PHP 7.1 nullable return types be allowed (if and when) ?

Refs:

• https://yoast.slack.com/archives/C0361PEK4/p1535629425000100

To Do:

• Check if a sniff already exists for this in PHPCS itself or in another external standard which could be added as a dependency.
• If so, add the dependency and add the sniff to the ruleset.
  • Check that the sniff is written in a code-style independent way and if not, improve the external sniff.
  • Check if the sniff covers all three aspects mentioned above and if not, either see about improving the external sniff or create a secondary YoastCS sniff to address the remaining aspects.
• If not, write the sniff.

The WordPress.Variables.GlobalVariables sniff was originally part of the intended ruleset, but turned off because of its buggyness as reported in WordPress/WordPress-Coding-Standards#300.
See: https://github.com/Yoast/yoastcs/blob/master/Yoast/ruleset.xml#L48

All the currently known bugs are expected to be fixed in the near future once WordPress/WordPress-Coding-Standards#736 is merged.

It is expected that the PR will be part of the next WPCS release 0.11.0.

So I'd like to suggest re-enabling the sniff once WPCS 0.11.0 has been released and the minimum WPCS requirement in the Yoast CS composer.json has been upped to 0.11.0.

Add a sniff to disallow @return type mixed in the documentation.

WPCS currently has five rulesets:

• WordPress-Core - stand-alone, only rules as documented for core
• WordPress-Docs - stand-alone, only documentation rules as documented for core
• WordPress-Extra - WordPress-Core + a number of extra programming/WP best practice kind of rules
• WordPress-VIP - WordPress-Core + a number of specific rules which plugins and themes have to comply with to be allowed on to the WP VIP platform.
• WordPress - WordPress-Core + WordPress-Docs + WordPress-Extra + WordPress-VIP (all current rulesets)

Currently Yoastcs includes the WordPress-VIP ruleset and the WordPress-Docs ruleset.

As far as I know, the Yoast plugins are not deployed on the VIP platform, so do not have to comply with the VIP rules - though they do contain some hints/best practices which may be useful to check against.

So having said that, to me, it would seem more logical to check against either WordPress (everything) or against WordPress-Extra + WordPress-Docs.

Opinions ?

Updating the sniffs should be a manual task that should be done once every release cycle, preferably right after release. Let's make sure that every composer install gives the same result in terms of code style...

I'd like to see a WordPress plugin sniff that reads the plugin version and detects functions and classes that have been @deprecated more than 2 major versions ago. Let's also throw an error when @deprecated tags don't contain a parsable version.

The current iteration of ruleset mandates @subpackage tag in file headers.

However, it is deprecated in PSR–5 draft.

I think it would make sense to omit it as hard requirement. Thoughts?

cc @jrfnl

Following up on the comment placed on this PR: #52

We should re-evaluate if we can optimize the build script by only loading and configuring the elements we really need for a run.

We have our own WPSEO_Options framework. This should be used for all interactions with options.
So any calls to get_option or update_option with a specific prefix should result in an error in the sniffs.

Whenever we want to allow these specifically, sniff-excludes should be used to mark them.

When we want to output HTML, we usually use presenters like this.

echo new Meta_Fields_Presenter( $this->get_metabox_post(), 'general' );

This triggers the output escaping sniff everywhere the presenter is used. Can we make sure we don't trigger the sniff on presenters like this? Instead we could analyze the present method on every presenter. That is were data should be escaped before it is included in the html output.

The WordPress-Docs ruleset had been added to WPCS upstream.

Rules should be adjusted in lines with it (a lot of our custom docs rules are likely superseded by it).

See WordPress-Docs/ruleset.xml.

A lot of the utility functions for PHPCS which I've written over the years and are included in WPCS and/or PHPCompatibility will soon be published in a separate repo called PHPCSUtils.

Once that repo has been published and the first version is tagged, this repo should become a require dependency for YoastCS and use of the utility methods in that repo should be implemented.

Both WPCS as well as PHPCompatibility will start using that repo too in their next major release.

The YoastCS version which implements PHPCSUtils should be a new major release and should at the same time update the WPCS and PHPCompatibility dependencies to the versions using PHPCSUtils as well.

As discussed today with @omarreiss @herregroen @IreneStr, a new sniff needs to be created which will check that namespace declarations comply with the following rules:

A namespace should be build up of:

1. A plugin specific prefix. This should be the namespace prefix as already specified for the WPCS PrefixAllGlobals sniff, like for instance Yoast\WP\Free.
2. The directory path to the file.
   If the plugin root is not the starting point for this, the sniff should allow for setting one or more subdirectories as starting points.
   These starting points should be provided as relative paths in relation to the project root.
   For example:
   • Standard setup: root/admin/domain/file.php would translate to the following namespace: Yoast\WP\Plugin\Admin\Domain.
   • A setup with all plugin files in a src directory, where the src directory has been indicated as a starting point:
     root/src/admin/domain/file.php would also translate to that same namespace: Yoast\WP\Plugin\Admin\Domain.
3. The namespace depth - excluding the plugin prefix - should be no more than 3 deep. If it is, an error will be thrown.
4. The namespace depth - excluding the plugin prefix - is recommended to be no more than 2 deep. If it is between the recommended and the maximum value, a warning will be thrown.

Both the recommended as well as the maximum namespace depth should be configurable properties of the sniff, so individual plugins have the ability to deviate from the "namespace depth" rules.

Not covered by the sniff

Other than that, the namespace declaration should comply with the following additional rules which will not be covered by this sniff.

1. Each leaf of a namespace name should be in Camel_Caps, i.e. each word should start with a capital and words should be separated by an underscore.
   Consecutive caps, like for acronyms, will be allowed.
2. There should be no whitespace or comments within the name part of the namespace declaration.
3. There should be exactly one space between the namespace keyword and the start of the namespace name in a namespace declaration.
4. There should be exactly one blank line before a namespace declaration.
5. There should be at least one blank line after a namespace declaration.

These rules will be proposed for WPCS shortly and are expected to be implemented in a sniff in WPCS upstream.

Valid code example

// file: plugin-root/admin/domain-scheme/file.php
<?php
/**
 * File docblock.
 */

namespace Yoast\WP\Free\Admin\Domain_Scheme;

use ...

Invalid code examples

// file: plugin-root/admin/domain-scheme/file.php
<?php
/**
 * File docblock.
 */
namespace NonYoastPrefix \
	/* Here comes the real namespace */ Admin\DomainScheme\SubDomain\TooLong;
use ...

When we deprecate a method, function or hook we use the build in WordPress functions for it. One of the arguments these functions should receive the version number and that's the location where different kind of forms are use. We should have a sniff that checks if the plugin prefix is in there as well as the version number. Just to prevent this:
_deprecated_function( __METHOD__, '11.0');

We want:
_deprecated_function( __METHOD__, 'WPSEO 11.0');

As we have the Options framework to handle options properly and apply defaults for options that aren't saved yet, we need to make sure this framework is always used instead of directly calling get_options with the option name.

We decided that the use of @inheritDoc should only be allowed for implementations of interfaces and abstract methods. If @inheritDoc is used to document any other method/function/class, Yoast CS should throw an error.

Slightly related: I've also +1ed this issue on WPCS to ensure no warnings about missing short descriptions will be thrown when we use @inheritDoc: WordPress/WordPress-Coding-Standards#1429

Typehints in comments should, if they refer to a namespaced class, always use a use statement and use the short form in the comment.

The following docblock would break this rule:

/**
 * A function.
 * 
 * @param \Yoast\WP\SEO\Input $input The input.
 *
 * @returns \Yoast\WP\SEO\Output The output.
 */

And the following docblock would be a correct replacement:

use Yoast\WP\SEO\Input;
use Yoast\WP\SEO\Output;

/**
 * A function.
 * 
 * @param Input $input The input.
 *
 * @returns Output The output.
 */

Ideally an auto-fixer would insert new use statements in alphabetical order at the top of the file and fix the comments directly.

We are aware not all WordPress tools would be able to correctly parse this notation but believe that the better way forward here would be to support this notation. Especially given that in the wider PHP open source environment this is also the standard.

Related to WordPress/WordPress-Coding-Standards#993 which proposes a sniff to check various other aspects of deprecated functions/methods.

In addition to the WPCS sniff, I would like to suggest a sniff to check that any function/method which has a @deprecated tag in the Yoast plugins, also has a @codeCoverageIgnore tag.

Files which are in a deprecated folder could be ignored for this sniff (presuming that the PHPUnit code coverage configuration ignores those anyway).

Opinions ?

With the 14.0 release we ran into unfortunate issues where we were not catching all our own exceptions, thus leading to fatals.

Because of this we'd like to explore options for sniffs that forbid the usage of functions that can throw exceptions without using try and catch.

So the following would be invalid code:

function_that_can_throw_exception();

With the following being a valid replacement;

try {
  function_that_can_throw_exception();
}
catch( Exception $e ) {
  // Anything, possibly even rethrowing the exception with added context.
  // In which case this function can't be used without catching it.
}

Even rethrowing the exception would help with developer awareness that exceptions have to be dealt with and can't simply be left.

In PR #81, I updated the WPCS dependency to ^1.0.0:

WPCS will be using semantic versioning. In practice, this means that:

• Patch versions will only contain bug fixes and changes which do not impact sniffing (readme updates and such).
• Minor version can contain new features, such as new sniffs, either WPCS native ones or adding upstream ones via the ruleset.
• Major versions can contain BC-breaks, i.e. removed support for previously deprecated behaviour and such.

Most of the repos which use YoastCS have a compose.lock file committed in the repo. For those the above is fine, as updates to the composer.lock will be managed.

A few repos - IIRC i18-module and WHIP, though possibly one or two more - don't have a committed composer.lock file. For those repos, the above version setting could cause unexpected build breaking when a new (minor) version of WPCS has been released and Travis would use the latest version when doing a composer install and the subsequent phpcs run.

So my question is:
👉 Should the WPCS dependency in YoastCS be limited to patch versions ? i.e. use ~1.0.0 instead.

The same could, of course, be said about the PHP_CodeSniffer and PHPCompatibilityWP dependencies.
For those, I strongly suggest leaving the dependency setting as is, i.e. ^#.#.

• For PHP_CodeSniffer: while higher versions of PHPCS may include new sniffs, those sniffs will not automatically be included as that is managed by WPCS. So allowing higher versions should be fine as these will contain bug fixes which the projects benefit from.
• For PHPCompatibilityWP: while higher minors will contain new sniffs, if this leads to breaking builds, that's a good thing as it generally means that a cross-version compatibility bug in the code has been discovered which should be fixed with high priority anyway.

The @covers tag in docblocks is use to denote what method/class a unit test tests and to prevent "incidental" coverage from being recorded.

"Incidental coverage" in this context, means tests calling methods other than the method under test to set up/prepare test. The resulting "lines covered" from this test set up/preparation should not be recorded.

Refs:

• https://phpunit.readthedocs.io/en/8.0/code-coverage-analysis.html
• https://phpunit.readthedocs.io/en/8.0/annotations.html#covers

These tags are only effective when used correctly and various versions of PHPUnit may have varying degrees of tolerance for incorrectly used @covers tags.

If a @covers tag is ignored because it is incorrect, code coverage for incidental coverage could be recorded, skewing the code coverage figures.

I would like to propose a new sniff to check the following:

• That all unit tests have a @covers tag either at the class or the method level.
  A warning should be thrown if the @covers tag is missing.
• That the content of the @covers tag is in a valid format as specified by the PHPUnit documentation.
  An error should be thrown when an invalid format is encountered and whenever possible this format should be auto-fixed.

Implementation notes:

• What classes/methods should be considered test classes/methods can be derived from the PHPUnit config file, if available.
  If not available, the sniff should either bow out or use sensible defaults which can be adjusted via the PHPCS config file.
• If a test method/class does not have a docblock, bow out. That is covered by another sniff.

If I remember correctly, files within the Yoast projects which have a namespace and contain an OO object declaration:

• do need a class/interface/trait docblock;
• but do not need a file docblock.

The current FileComment sniff, which WPCS includes, however, demands a file docblock for all files.

A wrapper sniff can be created which extends the existing FileComment sniff and bows out if both a namespace as well as an OO object declaration are found in a file. If either one isn't found, the normal sniff logic of the upstream sniff can be triggered.

@atimmer Could you please check this issue over and confirm whether I remembered this correctly (or comment to correct me) ?

With reference to the discussion had in #96 (comment), it may be a good idea to add a CONTRIBUTING.md file with some guidelines on:

• What belongs in this repo and what should be pulled upstream (WPCS/PHPCS)
• Basic guidelines for sniffs
• How to run the unit tests and such

See for details: #106

As previously discussed with @moorscode, this sniff should:

1. Check that only the new PHPCS annotations are used and forbid use of the old annotations.
   I.e. the following will no longer be allowed:
   • @codingStandardsIgnoreFile
   • @codingStandardsIgnoreStart / @codingStandardsIgnoreEnd
   • @codingStandardsIgnoreLine
   • @codingStandardsChangeSetting
2. Detect & forbid blanket PHPCS ignores.
   The new annotations allow to indicate which Standard/Category/Sniff/Errorcode should be temporarily ignored/disabled.
   • Sniff/Errorcode ignore/disables will be allowed. Errorcode based ones are preferred.
   • Standard/Category ignores/disables will not be allowed.
   • File ignores will not be allowed. These should be done from within the ruleset and documented there.
3. All temporary disables should be accompanied by a (re-) enable.
4. All ignores/disables should be accompanied by a reason for the ignore/disable.

This sniff will only be able to work when PHPCS is run with --ignore-annotations as otherwise errors on annotation tokens will be, well, ignored (default behaviour).

This means a secondary PHPCS run will be added to the Travis script if & when this sniff has been added to YoastCS and a plugin upgrades to the YoastCS version which includes it. This secondary run will only check against this particular sniff.

@moorscode In your opinion, does this summarize correctly what we discussed ?

Refs:

• https://github.com/squizlabs/PHP_CodeSniffer/releases/tag/3.2.0
• https://github.com/squizlabs/PHP_CodeSniffer/wiki/Advanced-Usage#ignoring-parts-of-a-file

In Yoast/wordpress-seo#12537, we're adding a util WPSEO_Utils:format_json_encode() which uses wp_json_encode() under the hood, adds a JSON_UNESCAPED_SLASHES flag if the PHP version > 5.4 and pretty prints if WP_DEBUG is enabled.

I want to enforce the util to be used everywhere instead of wp_json_encode and json_encode. We can also autofix.

As there is more active development going on and expected in the near future for this repo, it may be advisable to add a changelog.md file (or add it to the readme).

Once the initial changelog is made, tagged releases should be updated with the relevant change info.

Discussed on Slack with @herregroen.

For namespaced functions and constants, disallow importing these via use func/ use const.
Instead, the namespace should be imported and the function name/constant name should be qualified with the namespace name.

Enforce:

use Package\Level;

Level\function_name();

Disallow:

use func Package\Level\function_name;

function_name();

String concat sniff goes completely crazy over constructs like these:

'<h3>' . __( 'General settings', 'wordpress-seo' ) . '</h3><p>' . __( 'These are the General settings for WordPress SEO, here you can restart this tour or revert the WP SEO settings to default.', 'wordpress-seo' ) . '</p>'
. '<p><strong>' . __( 'Tracking', 'wordpress-seo' ) . '</strong><br/>' . __( 'To provide you with the best experience possible, we need your help. Please enable tracking to help us gather anonymous usage data.', 'wordpress-seo' ) . '</p>'
. '<p><strong>' . __( 'Tab: Your Info / Company Info', 'wordpress-seo' ) . '</strong><br/>' . __( 'Add some info here needed for Google\'s Knowledge Graph.', 'wordpress-seo' ) . '</p>'
. '<p><strong>' . __( 'Tab: Webmaster Tools', 'wordpress-seo' ) . '</strong><br/>' . __( 'You can add the verification codes for the different Webmaster Tools programs here, we highly encourage you to check out both Google and Bing&#8217;s Webmaster Tools.', 'wordpress-seo' ) . '</p>'
. '<p><strong>' . __( 'Tab: Security', 'wordpress-seo' ) . '</strong><br/>' . __( 'Determine who has access to the plugins advanced settings on the post edit screen.', 'wordpress-seo' ) . '</p>'
. '<p><strong>' . __( 'More WordPress SEO', 'wordpress-seo' ) . '</strong><br/>' . sprintf( __( 'There&#8217;s more to learn about WordPress &amp; SEO than just using this plugin. A great start is our article %1$sthe definitive guide to WordPress SEO%2$s.', 'wordpress-seo' ), '<a target="_blank" href="' . esc_url( 'https://yoast.com/articles/wordpress-seo/#utm_source=wpseo_dashboard&utm_medium=wpseo_tour&utm_campaign=tour' ) . '">', '</a>' ) . '</p>'
. '<p><strong style="font-size:150%;">' . __( 'Subscribe to our Newsletter', 'wordpress-seo' ) . '</strong><br/>'
. __( 'If you would like us to keep you up-to-date regarding WordPress SEO and other plugins by Yoast, subscribe to our newsletter:', 'wordpress-seo' ) . '</p>' .
'<form action="http://yoast.us1.list-manage1.com/subscribe/post?u=ffa93edfe21752c921f860358&amp;id=972f1c9122" method="post" id="newsletter-form" accept-charset="' . esc_attr( get_bloginfo( 'charset' ) ) . '">' .
'<p>' .
'<input style="margin: 5px; color:#666" name="EMAIL" value="' . esc_attr( $current_user->user_email ) . '" id="newsletter-email" placeholder="' . __( 'Email', 'wordpress-seo' ) . '"/>' .
'<input type="hidden" name="group" value="2"/>' .
'<button type="submit" class="button-primary">' . __( 'Subscribe', 'wordpress-seo' ) . '</button>' .
'</p></form>'

While it can be rewritten to satisfy the sniff, it severely compromises underlying HTML logic of strings. :(

Dependency Version

When the PHP_CodeSniffer dependency is at least version 3.3.2 (not yet released at the time of writing), then take the actions below.

Action checklist

• Add the XSD schema attributes to the various rulesets, like so:

  <ruleset name="My Project"
      xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
      xsi:noNamespaceSchemaLocation="https://raw.githubusercontent.com/squizlabs/PHP_CodeSniffer/master/phpcs.xsd">

  Note: we should probably use a relative schema location like ../vendor/squizlabs/php_codesniffer/phpcs.xsd as this library is expected to be installed via Composer anyway, so that location can be considered stable and should always be in-line with the PHPCS version used.
• Validate the current ruleset against the schema and update it to comply if necessary.
• Verify whether the Travis XML validation checks will take the newly added schema into account and if not, adjust them so they will.
• Add the schema to the custom rulesets in the individual Yoast repos.

Rationale:

An XSD schema definition file defines the expected format of an XML file. It defines the elements and attributes which can be used in the XML file and in which order those can be used.

Adding an XSD schema reference to an XML file will allow for stricter validation of the file to ensure it is valid for the purpose for which it is intended to be used, in this case: that it is a valid ruleset for use with PHPCS.

References:

• The XSD schema file was added in PHPCS 3.2.0 via PR squizlabs/PHP_CodeSniffer#1433
• The new array format for properties was introduced in PHPCS 3.3.0 / PR squizlabs/PHP_CodeSniffer#1665
• The XSD was updated to allow for the new array format in PHPCS 3.3.2 via squizlabs/PHP_CodeSniffer#2151

every do_action and apply_filters should be documented.

To determine whether a sniff is a double/mock, there are a couple of things which can be checked:

1. Does the class (interface/trait) name have double/mock in it ?
2. Does the class extend one of the known unit test classes ? If not and it is a file in the test(s) directory, this may be a helper class or an incorrectly named mock/double class.
   If so, this should probably trigger an error/warning too.

Next would be a check that those class files reside in a tests/doubles/ directory. However, this path should be one which can be overruled by a property in the ruleset to allow - for instance - the premium plugin, to have their mock/doubles in a tests/premium/doubles/ directory.

I have the bare bones of this sniff ready already, but there are some open questions (most notably see point 2 above) and it will need clean up & unit tests.

As discussed today with @herregroen @IreneStr and with some changes after further discussion with @omarreiss:

• Hook names in the Yoast plugin repos should start with the plugin prefix in namespace format, followed by the hook name in lowercase with words separated by underscores.
  I.e. Yoast\WP\Free\hook_name, not yoast_free_hook_name.
  To that end, the configuration of the WPCS ValidHookName sniff in YoastCS should be adjusted to allow for the \ character as a word separator as well as for underscores.
• The hook_name part should be descriptive for the (dev-)user and does not need to follow the namespace or file path of the file they are in.
• The hook_name part should have a maximum of three underscores in them, i.e they can consist of maximum four words.

The sniff should contain separate properties for recommended (warning) and maximum (error) number of underscores.
The default will be 3 in both cases.
Individual plugins can use the properties to deviate.

Valid code sample

do_action( 'Yoast\WP\Plugin\hook_name' );

Invalid code samples

do_action( 'yoast_plugin_hook_name' );
do_action( 'Yoast\WP\Plugin\hook_name_which_is_too_long' );

In our integrations directory we have all our classes that hook into WordPress filters and actions. As any exceptions thrown here in public functions wouldn't be able to be caught by us these would likely lead to fatals.

So ideally we'd be able to specify directories where throwing exceptions would be forbidden in public functions under the expectations that these would be hooked into WordPress filters ( if it's possible to detect which functions are hooked into filters or actions that would be excellent as well but I can see that being rather more difficult ).

So the following code would be forbidden only in public functions in a given directory:

throw new Exception( "Error!" );

While the following would be allowed:

return false;

Once upstream PR squizlabs/PHP_CodeSniffer#2364 has been merged, we should add the check to Travis.

Also see: #146 (comment)

Discussed on Slack with @herregroen

Disallow import use statements for functions and constants from the global namespace:

use func esc_attr;

use const PHP_VERSION_ID;

in favour of enforcing fully qualified names in the inline code:

\esc_attr( $something );

\version_compare( \PHP_VERSION_ID, '70300' );