WordPress.org

Make WordPress Core

Opened 3 weeks ago

Last modified 18 hours ago

#47632 assigned task (blessed)

Coding Standards fixes for WP 5.3

Reported by: pento Owned by: 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 12 days ago.
47632.2.diff (39.8 KB) - added by pento 10 days ago.

Download all attachments as: .zip

Change History (45)

#1 follow-up: @pento
3 weeks ago

In 45580:

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

See #47632

#2 @pento
3 weeks ago

In 45581:

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

See #47632

#3 @pento
3 weeks ago

In 45582:

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

See #47632.

#4 @pento
3 weeks ago

In 45583:

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

See #47632.

#5 @pento
3 weeks ago

In 45588:

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

See #47632.

#6 @pento
2 weeks ago

In 45590:

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

See #47632.

#7 @pento
2 weeks 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
2 weeks ago

In 45599:

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

See #47632.

#9 @pento
2 weeks 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
2 weeks 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
2 weeks 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
2 weeks ago

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

#13 @pento
2 weeks ago

In 45602:

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

See #47632.

#14 @pento
2 weeks ago

In 45603:

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

See #47632.

#15 @pento
2 weeks ago

#44395 was marked as a duplicate.

#16 @pento
2 weeks 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
2 weeks 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 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.

Last edited 2 weeks ago by jrf (previous) (diff)

#18 in reply to: ↑ 1 @jrf
2 weeks 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
12 days 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
12 days ago

#20 @pento
12 days 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
12 days ago

It weirdly seems to be working now. Nevermind. 🙃

#22 @pento
12 days 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
11 days ago

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

#24 @pento
11 days 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
11 days ago

In 45613:

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

Props zinigor.
Fixes #47668. See #47632.

#26 @SergeyBiryukov
10 days ago

In 45616:

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

See #47632.

#27 @SergeyBiryukov
10 days ago

In 45617:

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

See #47632.

#28 @pento
10 days 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
10 days ago

#29 @pento
10 days 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
10 days 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
9 days ago

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

#32 @pento
3 days 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
3 days 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.


3 days ago

#35 @pento
3 days 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
24 hours ago

In 45661:

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

See #47632.

#37 @pento
24 hours 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
24 hours 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
23 hours ago

In 45664:

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

See #47632.

#40 @pento
20 hours 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
20 hours 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
19 hours 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
18 hours 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.

Note: See TracTickets for help on using tickets.