#47632 closed task (blessed) (fixed)
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)
Change History (57)
#7
@
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 ----------------------------------------------------------------------
#10
follow-up:
↓ 17
@
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.
#12
@
5 years ago
While working through the WordPress.DB.PreparedSQL.NotPrepared
errors, I found that the vast majority of them are WPCS1331.
#17
in reply to:
↑ 10
;
follow-up:
↓ 19
@
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.
#18
in reply to:
↑ 1
@
5 years ago
Replying to pento:
In 45580:
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
@
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.
#20
@
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.
#24
@
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 ----------------------------------------------------------------------
#28
@
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.
#29
@
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
@
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.
#33
@
5 years ago
For renaming files that violate WordPress.Files.FileName.InvalidClassFileName
, I think the procedure will look something like this:
svn mv src/wp-admin/custom-header.php src/wp-admin/includes/class-custom-image-header.php
- 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' );
- Replace anywhere that's referring to
src/wp-admin/custom-header.php
with the new location (including phpdocs, for example).
svn add src/wp-admin/custom-header.php
- 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
#41
@
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 ofNotPrepared
, where queries are built up, then intentionally interpolated at the end. SeeWP_*_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
@
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
@
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.
This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.
5 years ago
#49
@
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:
↓ 52
@
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:
This ticket was mentioned in Slack in #forums by otto42. View the logs.
5 years ago
#53
@
5 years ago
Ah, fair enough. Still, got a report that some change here is causing problems deleting directories. Will investigate further.
In 45580: