Coding Standards fixes for WP 5.3

[pento]

This here be a tracking ticket for coding standards fixes during this release cycle.

Here's where we're starting:

PHP CODE SNIFFER VIOLATION SOURCE SUMMARY
----------------------------------------------------------------------
SOURCE                                                           COUNT
----------------------------------------------------------------------
Squiz.PHP.DisallowMultipleAssignments.Found                      831
WordPress.PHP.YodaConditions.NotYoda                             655
WordPress.WP.I18n.MissingTranslatorsComment                      556
WordPress.NamingConventions.ValidVariableName.VariableNotSnakeC  405
WordPress.DB.PreparedSQL.InterpolatedNotPrepared                 197
WordPress.PHP.NoSilencedErrors.Discouraged                       159
WordPress.WhiteSpace.PrecisionAlignment.Found                    122
WordPress.DB.PreparedSQL.NotPrepared                             118
WordPress.NamingConventions.ValidHookName.UseUnderscores         69
WordPress.Files.FileName.InvalidClassFileName                    48
WordPress.NamingConventions.ValidFunctionName.MethodNameInvalid  44
WordPress.NamingConventions.ValidFunctionName.FunctionNameInval  17
WordPress.NamingConventions.ValidHookName.NotLowercase           14
PEAR.NamingConventions.ValidClassName.StartWithCapital           8
WordPress.NamingConventions.ValidFunctionName.FunctionDoubleUnd  8
WordPress.NamingConventions.ValidVariableName.UsedPropertyNotSn  8
PEAR.NamingConventions.ValidClassName.Invalid                    6
WordPress.DB.PreparedSQLPlaceholders.UnfinishedPrepare           5
WordPress.DB.PreparedSQLPlaceholders.UnquotedComplexPlaceholder  4
WordPress.DB.PreparedSQLPlaceholders.ReplacementsWrongNumber     3
WordPress.NamingConventions.ValidVariableName.PropertyNotSnakeC  2
WordPress.DB.PreparedSQLPlaceholders.QuotedDynamicPlaceholderGe  1
WordPress.DB.PreparedSQLPlaceholders.QuotedSimplePlaceholder     1
----------------------------------------------------------------------
A TOTAL OF 3281 SNIFF VIOLATIONS WERE FOUND IN 23 SOURCES
----------------------------------------------------------------------

[pento]

In 45580:

Coding Standards: Fix/ignore the WordPress.NamingConventions.ValidFunctionName violations.

See #47632

[pento]

In 45581:

Coding Standards: Fix the Squiz.PHP.DisallowMultipleAssignments violations in the default themes.

See #47632

[pento]

In 45582:

Coding Standards: Fix the Squiz.PHP.DisallowMultipleAssignments violations in the base directory.

See #47632.

[pento]

In 45583:

Coding Standards: Fix the Squiz.PHP.DisallowMultipleAssignments violations in wp-admin.

See #47632.

[pento]

In 45588:

Coding Standards: Fix the Squiz.PHP.DisallowMultipleAssignments violations in tests.

See #47632.

[pento]

In 45590:

Coding Standards: Fix the Squiz.PHP.DisallowMultipleAssignments violations in wp-includes.

See #47632.

[pento]

Progress!

PHP CODE SNIFFER VIOLATION SOURCE SUMMARY
----------------------------------------------------------------------
SOURCE                                                           COUNT
----------------------------------------------------------------------
WordPress.PHP.YodaConditions.NotYoda                             673
WordPress.WP.I18n.MissingTranslatorsComment                      557
WordPress.NamingConventions.ValidVariableName.VariableNotSnakeC  405
WordPress.DB.PreparedSQL.InterpolatedNotPrepared                 231
WordPress.PHP.NoSilencedErrors.Discouraged                       166
WordPress.WhiteSpace.PrecisionAlignment.Found                    129
WordPress.DB.PreparedSQL.NotPrepared                             125
WordPress.NamingConventions.ValidHookName.UseUnderscores         69
WordPress.Files.FileName.InvalidClassFileName                    48
WordPress.NamingConventions.ValidHookName.NotLowercase           14
WordPress.NamingConventions.ValidVariableName.UsedPropertyNotSn  8
PEAR.NamingConventions.ValidClassName.StartWithCapital           7
PEAR.NamingConventions.ValidClassName.Invalid                    6
WordPress.DB.PreparedSQLPlaceholders.UnfinishedPrepare           5
WordPress.DB.PreparedSQLPlaceholders.UnquotedComplexPlaceholder  4
WordPress.DB.PreparedSQLPlaceholders.ReplacementsWrongNumber     3
WordPress.NamingConventions.ValidVariableName.PropertyNotSnakeC  2
WordPress.DB.PreparedSQLPlaceholders.QuotedDynamicPlaceholderGe  1
WordPress.DB.PreparedSQLPlaceholders.QuotedSimplePlaceholder     1
----------------------------------------------------------------------
A TOTAL OF 2454 SNIFF VIOLATIONS WERE FOUND IN 19 SOURCES
----------------------------------------------------------------------

[pento]

In 45599:

Coding Standards: Mark the handful of hook names with uppercase characters or hyphens as ignored.

See #47632.

[pento]

In 45600:

Coding Standards: Upgrade WPCS to 2.1.1.

Noteable changes:

• WPCS now throws warnings when non-strict comparisons are used. There are quite a few of them in Core. 🙃
• WPCS now detects and warns for assignments in loop conditions.

See #47632.

[pento]

@jrf: I haven't upgrade PHPCS, as version 3.4.2 makes a mess of the indenting in some files, I suspect it's something to do with ​this fix.

[pento]

In 45601:

Coding Standards: Fix all WordPress.CodeAnalysis.AssignmentInCondition issues.

WordPress.CodeAnalysis.AssignmentInCondition.FoundInWhileCondition can be ignored, as this is allowed in Core.

See #47632.

[pento]

While working through the WordPress.DB.PreparedSQL.NotPrepared errors, I found that the vast majority of them are ​WPCS1331.

[pento]

In 45602:

Coding Standards: Fix all WordPress.WhiteSpace.PrecisionAlignment issues.

See #47632.

[pento]

In 45603:

Coding Standards: Fix all WordPress.DB.PreparedSQLPlaceholders issues.

See #47632.

[pento]

#44395 was marked as a duplicate.

[pento]

In 45604:

Coding Standards: Add missing translator comments.

Twentys Eleven, Sixteen, and Seventeen now have translator comments for all their strings.

Also, WPCS now doesn't show missing translator comment warnings for test files.

See #47632.

[jrf]

Replying to pento:

@jrf: I haven't upgrade PHPCS, as version 3.4.2 makes a mess of the indenting in some files, I suspect it's something to do with ​this fix.

Understood, though I suspect it may have more to do with ​the fix for this issue. Any particular file you can point me to which shows the incorrect fixes well ?

I can run some tests to check if it has been corrected again for PHPCS 3.5.0 and if not report the fall-out of the fix upstream.

[jrf]

Replying to pento:

In 45580:

Coding Standards: Fix/ignore the WordPress.NamingConventions.ValidFunctionName violations.

See #47632

Just checking: Function names in PHP are case-insensitive, so most of these could be fixed without breaking anything. Any particular reason why these functions with case issues are whitelisted instead of fixed ?

For the underscore prefixed functions, a deprecation and new method with a better name could be considered.

[pento]

Replying to jrf:

Understood, though I suspect it may have more to do with ​the fix for this issue. Any particular file you can point me to which shows the incorrect fixes well ?

src/wp-includes/theme-compat/embed.php shows it pretty well. src/wp-admin/includes/media.php is also an interesting example of indenting the ?> correctly, but not fixing the corresponding <?php.

Just checking: Function names in PHP are case-insensitive, so most of these could be fixed without breaking anything. Any particular reason why these functions with case issues are whitelisted instead of fixed ?

WP_Filesystem could probably be fixed like this, but everything else (eg, dbDelta) would need to be deprecated and renamed to db_delta. Given the age of these functions, I didn't think it was worth the trouble of deprecating them for all the thousands of plugins or themes that are using them.

For the underscore prefixed functions, a deprecation and new method with a better name could be considered.

Similarly, these are all really old functions that could possibly be fixed, but (particularly in the case of the __return_*() functions, it would just be a hassle for the plugins and themes that use them.

[pento]

In 47632.diff​, I've tried to split out the custom_test_class_whitelist property of the WordPress.Files.FileName rule into individual elements, instead of having an ever-growing comma delimited string, but it doesn't seem to be working properly.

[pento]

It weirdly seems to be working now. Nevermind. 🙃

[pento]

In 45607:

Coding Standards: Fix the remaining issues in /tests.

All PHP files in /tests now conform to the PHP coding standards, or have exceptions appropriately marked.

Travis now also runs phpcs on the /tests directory, any future changes to these files must conform entirely to the WordPress PHP coding standards. 🎉

See #47632.

[pento]

r45609, r45611, and r45612 all belong on this ticket, I just typoed the ticket number. 🙂

[pento]

10 steps forward, two giant leaps back. 🙃

PHP CODE SNIFFER VIOLATION SOURCE SUMMARY
----------------------------------------------------------------------
SOURCE                                                           COUNT
----------------------------------------------------------------------
WordPress.PHP.StrictComparisons.LooseComparison                  2514
WordPress.PHP.YodaConditions.NotYoda                             653
WordPress.PHP.StrictInArray.MissingTrueStrict                    511
WordPress.WP.I18n.MissingTranslatorsComment                      364
WordPress.NamingConventions.ValidVariableName.VariableNotSnakeC  321
WordPress.DB.PreparedSQL.InterpolatedNotPrepared                 177
WordPress.DB.PreparedSQL.NotPrepared                             116
WordPress.Files.FileName.InvalidClassFileName                    21
PEAR.NamingConventions.ValidClassName.Invalid                    5
PEAR.NamingConventions.ValidClassName.StartWithCapital           5
----------------------------------------------------------------------
A TOTAL OF 4687 SNIFF VIOLATIONS WERE FOUND IN 10 SOURCES
----------------------------------------------------------------------

[SergeyBiryukov]

In 45613:

Filesystem API: Check correct variable in WP_Filesystem_Direct::dirlist() after [45611].

Props zinigor.
Fixes #47668. See #47632.

[SergeyBiryukov]

In 45616:

Coding Standards: Remove extra whitespace in wp-admin/includes/image-edit.php.

See #47632.

[SergeyBiryukov]

In 45617:

Coding Standards: Remove extra whitespace in list tables' column_cb() methods.

See #47632.

[pento]

I've been testing the ​Slevomat Coding Standard auto-fixer for Yoda conditions, unfortunately it doesn't seem to treat Yoda conditions exactly how WPCS expects them, I've submitted a ​bug report.

[pento]

47632.2.diff​ is aimed at addressing the handful of invalid class names.

@jrf: As I understand it, class names are entirely case insensitive, ja? There aren't hidden catches to just changing the case?

[ocean90]

This will likely break any (third-party) usage of get_class() since it returns the user-defined name of the class. For example get_class( $GLOBALS['wpdb'] ) === 'wpdb' wouldn't be true anymore.

[pento]

Good point, thanks @ocean90. I'll leave these classes as is.

[pento]

In 45653:

Coding Standards: Exclude some class names from checks when they can't be renamed.

Renaming the classes would likely cause back compat issues, so the safest option is to allow this handful to continue unchanged.

See #47632.

[pento]

For renaming files that violate WordPress.Files.FileName.InvalidClassFileName, I think the procedure will look something like this:

1. svn mv src/wp-admin/custom-header.php src/wp-admin/includes/class-custom-image-header.php

2. Create a new file at src/wp-admin/custom-header.php, with this content:

<?php
/**
 * Custom header image script.
 *
 * This file is deprecated, use 'wp-admin/includes/class-custom-image-header.php' instead.
 *
 * @deprecated 5.3.0
 * @package WordPress
 * @subpackage Administration
 */

_deprecated_file( basename( __FILE__ ), '5.3.0', 'wp-admin/includes/class-custom-image-header.php' );

/** Custom_Image_Header class */
require_once( ABSPATH . 'wp-admin/includes/class-custom-image-header.php' );

3. Replace anywhere that's referring to src/wp-admin/custom-header.php with the new location (including phpdocs, for example).

4. svn add src/wp-admin/custom-header.php

5. Then svn ci as normal.

The svn mv will link the history of custom-header.php to class-custom-image-header.php, but I'm not sure if adding a new file with the same name in the same commit will break that. I don't suppose any SVN experts want to weigh in here?

[pento]

In 45654:

Coding Standards: Move wp-admin/custom-header.php to wp-admin/includes/class-custom-image-header.php

This renames the file containing the Custom_Image_Header class to conform to the coding standards.

This commit also includes:

• A new custom-header.php that includes the new file, for anyone that may've been including the file directly.
• Replaces references to the old filename with the new filename.

See #47632.

[pento]

In 45661:

Coding Standards: Exclude a handful of incorrectly named files that won't be renamed.

See #47632.

[pento]

In 45662:

Coding Standards: Move wp-admin/custom-background.php to wp-admin/includes/class-custom-background.php

This renames the file containing the Custom_Background class to conform to the coding standards.

This commit also includes:

• A new custom-background.php that includes the new file, for anyone that may've been including the file directly.
• Replaces a reference to the old filename with the new filename.

See #47632.

[pento]

In 45663:

Coding Standards: Move wp-includes/class-oembed.php to wp-includes/class-wp-oembed.php.

This renames the file containing the WP_oEmbed class to conform to the coding standards.

This commit also includes:

• A new class-oembed.php that includes the new file, for anyone that may've been including the file directly.
• Replaces references to the old filename with the new filename.

See #47632.

[pento]

In 45664:

Coding Standards: Fix a filename replacement missed in [45663].

See #47632.

[pento]

In 45665:

Coding Standards: Move the remaining PHPCS errors to report as warnings, and add Travis tests.

The remaining error-level coding standards issues (specifically, associated with the sniffs WordPress.PHP.YodaConditions.NotYoda, WordPress.NamingConventions.ValidVariableName.VariableNotSnakeCase, WordPress.DB.PreparedSQL.InterpolatedNotPrepared, WordPress.DB.PreparedSQL.NotPrepared, and WordPress.Files.FileName.InvalidClassFileName) are marked as warnings, until they're all addressed.

This change allows us to run linting on Travis across the entire codebase, ensuring no other error-level violations can be introduced.

Additionally, PHPCS will now cache results locally, drastically improving performance for subsequent checks: scanning the entire codebase takes 1-2 minutes the first time, and less than one second for subsequent checks.

See #47632.

[pento]

Here's where we're at now:

PHP CODE SNIFFER VIOLATION SOURCE SUMMARY
----------------------------------------------------------------------
SOURCE                                                           COUNT
----------------------------------------------------------------------
WordPress.PHP.StrictComparisons.LooseComparison                  2511
WordPress.PHP.YodaConditions.NotYoda                             652
WordPress.PHP.StrictInArray.MissingTrueStrict                    511
WordPress.WP.I18n.MissingTranslatorsComment                      365
WordPress.NamingConventions.ValidVariableName.VariableNotSnakeC  321
WordPress.DB.PreparedSQL.InterpolatedNotPrepared                 177
WordPress.DB.PreparedSQL.NotPrepared                             116
WordPress.Files.FileName.InvalidClassFileName                    12
----------------------------------------------------------------------
A TOTAL OF 4665 SNIFF VIOLATIONS WERE FOUND IN 8 SOURCES
----------------------------------------------------------------------

I'm excited that the total number of violations is higher than when I started this ticket. 🙃

Violations of note:

• WordPress.PHP.YodaConditions.NotYoda: There's still a discussion to be had over whether we keep using Yoda Conditions or not. I don't intend to tackle these violations en masses until after that discussion.
• WordPress.DB.PreparedSQL.NotPrepared: I don't think we need to look at these until after ​WPCS1331 is addressed.
• WordPress.DB.PreparedSQL.InterpolatedNotPrepared: These violations are mostly more complex versions of NotPrepared, where queries are built up, then intentionally interpolated at the end. See WP_*_Query for examples.
• WordPress.Files.FileName.InvalidClassFileName: Fixing these is tedious. I'll do one or two at a time until they're finished.

[johnbillion]

@pento That cache flag is new to me and seems useful. Can the cache location be specified and then added to the list of directories that Travis CI caches between jobs?

[pento]

It's possible to pass a folder location to the --cache flag. I didn't do it for Travis, since it runs pretty quickly, and this was a 💯 YOLOFRIDAY commit.

It would shave 3 minutes off the 5 minutes the Travis job takes to run, so it's worth exploring.

[SergeyBiryukov]

In 45678:

Coding Standards: Move wp-includes/date.php to wp-includes/class-wp-date-query.php.

This renames the file containing the WP_Date_Query class to conform to the coding standards.

This commit also includes:

• A new date.php that includes the new file, for anyone that may've been including the file directly.
• Replaces references to the old filename with the new filename.

Fixes #47775. See #47632.

[SergeyBiryukov]

In 45816:

Coding Standards: In wlwmanifest_link(), use concatenation instead of commas for echo, for consistency with rsd_link() and the rest of core.

See #47632.

[SergeyBiryukov]

In 45854:

Coding Standards: Use KB_IN_BYTES in get_file_data().

See #22405, #47632.

[SergeyBiryukov]

In 46466:

Tests: Rename tests methods in tests/pluggable.php per the handbook's naming convention.

See #47632.

[desrosj]

With 5.3 RC 1 tomorrow, let's close this out and start looking forward to the coding standards fixes that can be made in 5.4.

[Otto42]

Replying to pento:

In 45611:

(The changeset message doesn't reference this ticket)

Hey @pento, this is a bug. Potentially a bad one.

if ( ! $this->is_dir( $path ) || ! $this->is_readable( $dir ) ) {
    return false;
}

$dir = dir( $path );

$dir isn't set in the check here. is_readable $dir is always false.

Edit:

https://core.trac.wordpress.org/changeset/45611/trunk/src/wp-admin/includes/class-wp-filesystem-direct.php

[SergeyBiryukov]

Replying to Otto42:

Hey @pento, this is a bug. Potentially a bad one.

if ( ! $this->is_dir( $path ) || ! $this->is_readable( $dir ) ) {
    return false;
}

$dir = dir( $path );

$dir isn't set in the check here. is_readable $dir is always false.

It was fixed the same day in [45613] / #47668.

[Otto42]

Ah, fair enough. Still, got a report that some change here is causing problems deleting directories. Will investigate further.

[SergeyBiryukov]

#44248 was marked as a duplicate.