Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 21 months ago

#47632 closed task (blessed) (fixed)

Coding Standards fixes for WP 5.3

Reported by: pento's profile pento Owned by: pento's profile pento
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: General Keywords:
Focuses: coding-standards Cc:

Description

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

Attachments (2)

47632.diff (1.7 KB) - added by pento 5 years ago.
47632.2.diff (39.8 KB) - added by pento 5 years ago.

Download all attachments as: .zip

Change History (57)

#1 follow-up: @pento
5 years ago

In 45580:

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

See #47632

#2 @pento
5 years ago

In 45581:

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

See #47632

#3 @pento
5 years ago

In 45582:

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

See #47632.

#4 @pento
5 years ago

In 45583:

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

See #47632.

#5 @pento
5 years ago

In 45588:

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

See #47632.

#6 @pento
5 years ago

In 45590:

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

See #47632.

#7 @pento
5 years ago

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

#8 @pento
5 years ago

In 45599:

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

See #47632.

#9 @pento
5 years ago

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.

#10 follow-up: @pento
5 years ago

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

#11 @pento
5 years ago

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.

#12 @pento
5 years ago

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

#13 @pento
5 years ago

In 45602:

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

See #47632.

#14 @pento
5 years ago

In 45603:

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

See #47632.

#15 @pento
5 years ago

#44395 was marked as a duplicate.

#16 @pento
5 years ago

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.

#17 in reply to: ↑ 10 ; follow-up: @jrf
5 years ago

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 issue](https://github.com/squizlabs/PHP_CodeSniffer/issues/2396). 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.

Version 0, edited 5 years ago by jrf (next)

#18 in reply to: ↑ 1 @jrf
5 years ago

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.

#19 in reply to: ↑ 17 @pento
5 years ago

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
5 years ago

#20 @pento
5 years ago

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.

#21 @pento
5 years ago

It weirdly seems to be working now. Nevermind. 🙃

#22 @pento
5 years ago

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.

#23 @pento
5 years ago

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

#24 @pento
5 years ago

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

#25 @SergeyBiryukov
5 years ago

In 45613:

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

Props zinigor.
Fixes #47668. See #47632.

#26 @SergeyBiryukov
5 years ago

In 45616:

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

See #47632.

#27 @SergeyBiryukov
5 years ago

In 45617:

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

See #47632.

#28 @pento
5 years ago

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
5 years ago

#29 @pento
5 years ago

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?

#30 @ocean90
5 years ago

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.

#31 @pento
5 years ago

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

#32 @pento
5 years ago

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.

#33 @pento
5 years ago

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
  1. 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' );
  1. Replace anywhere that's referring to src/wp-admin/custom-header.php with the new location (including phpdocs, for example).
  1. svn add src/wp-admin/custom-header.php
  1. 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?

This ticket was mentioned in Slack in #core-committers by pento. View the logs.


5 years ago

#35 @pento
5 years ago

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.

#36 @pento
5 years ago

In 45661:

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

See #47632.

#37 @pento
5 years ago

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.

#38 @pento
5 years ago

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.

#39 @pento
5 years ago

In 45664:

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

See #47632.

#40 @pento
5 years ago

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.

#41 @pento
5 years ago

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.

#42 @johnbillion
5 years ago

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

#43 @pento
5 years ago

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.

#44 @SergeyBiryukov
5 years ago

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.

#45 @SergeyBiryukov
5 years ago

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.

#46 @SergeyBiryukov
5 years ago

In 45854:

Coding Standards: Use KB_IN_BYTES in get_file_data().

See #22405, #47632.

This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.


5 years ago

#48 @SergeyBiryukov
5 years ago

In 46466:

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

See #47632.

#49 @desrosj
5 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#50 follow-up: @Otto42
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

Last edited 5 years ago by Otto42 (previous) (diff)

This ticket was mentioned in Slack in #forums by otto42. View the logs.


5 years ago

#52 in reply to: ↑ 50 @SergeyBiryukov
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

#53 @Otto42
5 years ago

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

This ticket was mentioned in Slack in #core-coding-standards by jrf. View the logs.


5 years ago

#55 @SergeyBiryukov
21 months ago

#44248 was marked as a duplicate.

Note: See TracTickets for help on using tickets.