Context Navigation

← Previous Ticket
Next Ticket →

#45934 closed task (blessed) (fixed)

Coding Standards: Manual Fixes

Reported by:	pento	Owned by:	pento
Milestone:	5.1	Priority:	low
Severity:	minor	Version:	5.1
Component:	General	Keywords:
Focuses:	coding-standards	Cc:

Description

Here's the current output of composer run lint.

PHP CODE SNIFFER VIOLATION SOURCE SUMMARY
----------------------------------------------------------------------
SOURCE                                                           COUNT
----------------------------------------------------------------------
Squiz.PHP.DisallowMultipleAssignments.Found                      906
WordPress.PHP.YodaConditions.NotYoda                             692
WordPress.WP.I18n.MissingTranslatorsComment                      590
WordPress.NamingConventions.ValidVariableName.NotSnakeCase       423
WordPress.DB.PreparedSQL.NotPrepared                             334
Generic.PHP.NoSilencedErrors.Discouraged                         288
WordPress.NamingConventions.ValidVariableName.NotSnakeCaseMembe  268
WordPress.WhiteSpace.PrecisionAlignment.Found                    155
WordPress.NamingConventions.ValidHookName.UseUnderscores         69
WordPress.Files.FileName.InvalidClassFileName                    52
WordPress.NamingConventions.ValidFunctionName.MethodNameInvalid  43
WordPress.NamingConventions.ValidFunctionName.FunctionNameInval  18
Squiz.ControlStructures.ControlSignature.SpaceAfterCloseBrace    16
WordPress.NamingConventions.ValidHookName.NotLowercase           14
WordPress.NamingConventions.ValidVariableName.MemberNotSnakeCas  11
WordPress.WP.I18n.LowLevelTranslationFunction                    11
WordPress.WP.I18n.NonSingularStringLiteralText                   11
WordPress.WP.I18n.NonSingularStringLiteralDomain                 10
WordPress.NamingConventions.ValidFunctionName.FunctionDoubleUnd  9
PEAR.NamingConventions.ValidClassName.StartWithCapital           8
PEAR.NamingConventions.ValidClassName.Invalid                    6
WordPress.DB.PreparedSQLPlaceholders.UnfinishedPrepare           5
PSR2.ControlStructures.SwitchDeclaration.TerminatingComment      4
WordPress.DB.PreparedSQLPlaceholders.UnquotedComplexPlaceholder  4
WordPress.Files.FileName.NotHyphenatedLowercase                  4
WordPress.PHP.DontExtract.extract_extract                        4
WordPress.WP.I18n.MismatchedPlaceholders                         4
WordPress.WP.I18n.MissingSingularPlaceholder                     4
WordPress.DB.PreparedSQLPlaceholders.ReplacementsWrongNumber     3
Generic.NamingConventions.UpperCaseConstantName.ConstantNotUppe  2
WordPress.WP.I18n.NonSingularStringLiteralPlural                 2
WordPress.WP.I18n.NonSingularStringLiteralSingle                 2
WordPress.DB.PreparedSQLPlaceholders.QuotedDynamicPlaceholderGe  1
WordPress.DB.PreparedSQLPlaceholders.QuotedSimplePlaceholder     1
WordPress.Files.FileName.InvalidTemplateTagFileName              1
WordPress.NamingConventions.ValidVariableName.StringNotSnakeCas  1
WordPress.PHP.RestrictedPHPFunctions.create_function_create_fun  1
----------------------------------------------------------------------
A TOTAL OF 3977 SNIFF VIOLATIONS WERE FOUND IN 37 SOURCES
----------------------------------------------------------------------

There's a long tail of sniffs with only a handful of violations, so I'm going to go through them. This is a catch-all ticket for them.

Attachments (1)

45934.diff (38.9 KB) - added by pento 6 years ago.

Download all attachments as: .zip

Change History (23)

#1 @pento
6 years ago

In 44560:

Coding Standards: Ignore the single WordPress.PHP.RestrictedPHPFunctions.create_function_create_function violation.

This is intentional, for older PHP support.

See #45934.

#2 @pento
6 years ago

In 44561:

Coding Standards: template.php isn't a template tag file.

WordPress.Files.FileName.InvalidTemplateTagFileName shouldn't apply to it.

See #45934.

#3 @pento
6 years ago

In 44562:

Coding Standards: Fix the minor WordPress.WP.I18n violations.

WordPress.WP.I18n.MissingTranslatorsComment is in progress in #44360.

See #45934.

#4 @pento
6 years ago

In 44563:

Coding Standards: Ignore the violations of Generic.NamingConventions.UpperCaseConstantName.ConstantNotUpperCase.

See #45934.

#5 @pento
6 years ago

In 44564:

Coding Standards: Add exceptions for WordPress.Files.FileName.NotHyphenatedLowercase.

See #45934.

#6 @pento
6 years ago

In 44565:

Coding Standards: Document intentional case block fall-throughs.

Fixes PSR2.ControlStructures.SwitchDeclaration.TerminatingComment violations.

See #45934.

#7 @pento
6 years ago

In 44566:

Coding Standards: Fix the Squiz.ControlStructures.ControlSignature.SpaceAfterCloseBrace violations.

See #45934.

#8 @pento
6 years ago

Progress!

PHP CODE SNIFFER VIOLATION SOURCE SUMMARY
----------------------------------------------------------------------
SOURCE                                                           COUNT
----------------------------------------------------------------------
Squiz.PHP.DisallowMultipleAssignments.Found                      906
WordPress.PHP.YodaConditions.NotYoda                             692
WordPress.WP.I18n.MissingTranslatorsComment                      585
WordPress.NamingConventions.ValidVariableName.NotSnakeCase       423
WordPress.DB.PreparedSQL.NotPrepared                             334
Generic.PHP.NoSilencedErrors.Discouraged                         288
WordPress.NamingConventions.ValidVariableName.NotSnakeCaseMembe  268
WordPress.WhiteSpace.PrecisionAlignment.Found                    155
WordPress.NamingConventions.ValidHookName.UseUnderscores         69
WordPress.Files.FileName.InvalidClassFileName                    52
WordPress.NamingConventions.ValidFunctionName.MethodNameInvalid  43
WordPress.NamingConventions.ValidFunctionName.FunctionNameInval  18
WordPress.NamingConventions.ValidHookName.NotLowercase           14
WordPress.NamingConventions.ValidVariableName.MemberNotSnakeCas  11
WordPress.NamingConventions.ValidFunctionName.FunctionDoubleUnd  9
PEAR.NamingConventions.ValidClassName.StartWithCapital           8
PEAR.NamingConventions.ValidClassName.Invalid                    6
WordPress.DB.PreparedSQLPlaceholders.UnfinishedPrepare           5
WordPress.DB.PreparedSQLPlaceholders.UnquotedComplexPlaceholder  4
WordPress.PHP.DontExtract.extract_extract                        4
WordPress.DB.PreparedSQLPlaceholders.ReplacementsWrongNumber     3
WordPress.DB.PreparedSQLPlaceholders.QuotedDynamicPlaceholderGe  1
WordPress.DB.PreparedSQLPlaceholders.QuotedSimplePlaceholder     1
WordPress.NamingConventions.ValidVariableName.StringNotSnakeCas  1
----------------------------------------------------------------------
A TOTAL OF 3900 SNIFF VIOLATIONS WERE FOUND IN 24 SOURCES
----------------------------------------------------------------------

#9 @GaryJ
6 years ago

r44562 contains a // wpcs:ignore ... instead of // phpcs:ignore...

#10 @pento
6 years ago

In 44569:

Coding Standards: Extract extract() from the codebase.

Of the last four instances of extract() occurring, three of them are removed by this commit, and the fourth is appropriately documented.

See #45934.

#11 @pento
6 years ago

In 44570:

Coding Standards: Fix an incorrect phpcs:ignore comment.

It's phpcs:ignore, not wpcs:ignore.

Props GaryJ.

See #45934.

#12 @pento
6 years ago

In 44571:

Coding Standards: Exclude some external files.

class-wp-block-parser.php and wp-includes/blocks/* are imported from the Gutenberg plugin.
speed-trap-listener.php is a third party library.

See #45934.

#13 @pento
6 years ago

In 44572:

Coding Standards: Remove an unnecessary line whitelisting.

Props jrf.
See #45934.

#14 @jrf
6 years ago

@pento Thanks for opening this ticket and getting the ball rolling on this.

I've had a quick look through the ticket and left one remark inline on GH: https://github.com/WordPress/wordpress-develop/commit/8cc49330a4f812d0d374dc49b703ad2704139af0#r31916991 (partially the same as @GaryJ's remark which has already been fixed, sorry).

Re: https://core.trac.wordpress.org/changeset/44571 - Just wondering: why would the Gutenberg plugin not be expected to comply with the WP Coding Standards ? (first two excludes, the third for external file is of course perfectly fine)

Regarding Yoda conditions - we've talked about this before. IMO this rule should be dropped in favour of the "No assignment in condition" rule and the associated sniff WordPress/Generic.CodeAnalysis.AssignmentInCondition which is currently in Extra should be moved to Core.

At the time you asked me to investigate whether there is a sniff available out there which could revert existing Yoda conditions. The answer to that is "Yes".
The external Slevomat Coding Standard contains a sniff which can auto-fix Yoda to non-Yoda. Tests could (should) be run to see how well it works, but this sniff could be used in a one-time action to move away from Yoda.
Note: if the sniff would not be good enough/would turn out to be buggy, I'm happy to contribute fixes to the Slevomat standard just to make moving away from Yoda possible.

Having said that, as the tooling is available which would allow us to move away from Yoda and still prevent assignments in conditions, would strongly suggest now is a good time to take a decision on this.

Version 1, edited 6 years ago by jrf (previous) (next) (diff)

#15 @pento
6 years ago

why would the Gutenberg plugin not be expected to comply with the WP Coding Standards

It should be, but these files are managed in the Gutenberg plugin: if they have code formatting issues, they need to be fixed in the Gutenberg repo.

Regarding Yoda conditions

Yah, I agree that WordPress/Generic.CodeAnalysis.AssignmentInCondition should be moved into Core, violations should be fixed, and then Yoda conditions should be removed. There's some discussion on #42885 about Yoda conditions, too.

Notably, the JS team are planning on removing guidance on Yoda conditions, I'm inclined to disallow Yoda conditions, rather than simply allow either style, though.

@pento
6 years ago

Attachment 45934.diff added

#16 @pento
6 years ago

@jrf: Could I get you to review the phpcs.xml.dist changes in 45934.diff ? While it correctly ignores things it should ignore, this doesn't seem like a well-targeted method of whitelisting member variables that we can't fix.

#17 follow-up: ↓ 18 @jrf
6 years ago

@pento Reviewed. This is currently (aside from whitelist comments) the right way to do this.

I presume you are concerned that using the properties would accidentally whitelist too much ?

I'd been thinking about this before and have been playing with the thought of adding scoping to this property.
Elements in an property array can have keys, i.e. <element key="something" value="something"/>, though the sniff currently only expects and looks at values.

We could use the key to limit the whitelisting for a particular property to a specific file/folder or class.

I'm not sure (yet) what would be the best identifier to use. Input welcome. (and yes, one of us should probably open an issue about this in the WPCS repo).

#18 in reply to: ↑ 17 @pento
6 years ago

Replying to jrf:

I presume you are concerned that using the properties would accidentally whitelist too much ?

Yup, that's my concern.

I'd been thinking about this before and have been playing with the thought of adding scoping to this property.
Elements in an property array can have keys, i.e. <element key="something" value="something"/>, though the sniff currently only expects and looks at values.

We could use the key to limit the whitelisting for a particular property to a specific file/folder or class.

I'm not sure (yet) what would be the best identifier to use. Input welcome. (and yes, one of us should probably open an issue about this in the WPCS repo).

I like this idea. I'll open an issue in the WPCS repo. 🙂

#19 @pento
6 years ago

In 44573:

Coding Standards: Fix and whitelist variable names.

From the WordPress.NamingConventions.ValidVariableName sniff, this commit fixes/whitelists all NotSnakeCaseMemberVar, MemberNotSnakeCase, and StringNotSnakeCase violations. It also fixes a handful of the NotSnakeCase violations.

See #45934.

#20 @pento
6 years ago

PHP CODE SNIFFER VIOLATION SOURCE SUMMARY
----------------------------------------------------------------------
SOURCE                                                           COUNT
----------------------------------------------------------------------
Squiz.PHP.DisallowMultipleAssignments.Found                      882
WordPress.PHP.YodaConditions.NotYoda                             691
WordPress.WP.I18n.MissingTranslatorsComment                      585
WordPress.NamingConventions.ValidVariableName.NotSnakeCase       397
WordPress.DB.PreparedSQL.NotPrepared                             334
WordPress.PHP.NoSilencedErrors.Discouraged                       165
WordPress.WhiteSpace.PrecisionAlignment.Found                    149
WordPress.NamingConventions.ValidHookName.UseUnderscores         69
WordPress.Files.FileName.InvalidClassFileName                    50
WordPress.NamingConventions.ValidFunctionName.MethodNameInvalid  43
WordPress.NamingConventions.ValidFunctionName.FunctionNameInval  18
WordPress.NamingConventions.ValidHookName.NotLowercase           14
WordPress.NamingConventions.ValidFunctionName.FunctionDoubleUnd  9
PEAR.NamingConventions.ValidClassName.StartWithCapital           8
PEAR.NamingConventions.ValidClassName.Invalid                    6
WordPress.DB.PreparedSQLPlaceholders.UnfinishedPrepare           5
WordPress.DB.PreparedSQLPlaceholders.UnquotedComplexPlaceholder  4
WordPress.DB.PreparedSQLPlaceholders.ReplacementsWrongNumber     3
WordPress.DB.PreparedSQLPlaceholders.QuotedDynamicPlaceholderGe  1
WordPress.DB.PreparedSQLPlaceholders.QuotedSimplePlaceholder     1
----------------------------------------------------------------------
A TOTAL OF 3434 SNIFF VIOLATIONS WERE FOUND IN 20 SOURCES
----------------------------------------------------------------------

Along with upgrading WPCS in #45956, we're making some progress here. 🙂

#21 @pento
6 years ago

Version set to trunk

#22 @pento
6 years ago

Resolution set to fixed
Status changed from assigned to closed

Closing this one, not planning on doing any more manual fixes for 5.1. Can pick it up again in 5.2.

Note: See TracTickets for help on using tickets.

Trac UI Preferences

Download in other formats:

Make WordPress Core

Context Navigation

#45934 closed task (blessed) (fixed)

Coding Standards: Manual Fixes

Description

Attachments (1)

Change History (23)

#1 @pento 6 years ago

#2 @pento 6 years ago

#3 @pento 6 years ago

#4 @pento 6 years ago

#5 @pento 6 years ago

#6 @pento 6 years ago

#7 @pento 6 years ago

#8 @pento 6 years ago

#9 @GaryJ 6 years ago

#10 @pento 6 years ago

#11 @pento 6 years ago

#12 @pento 6 years ago

#13 @pento 6 years ago

#14 @jrf 6 years ago

#15 @pento 6 years ago

@pento 6 years ago

#16 @pento 6 years ago

#17 follow-up: ↓ 18 @jrf 6 years ago

#18 in reply to: ↑ 17 @pento 6 years ago

#19 @pento 6 years ago

#20 @pento 6 years ago

#21 @pento 6 years ago

#22 @pento 6 years ago

Download in other formats:

#1 @pento
6 years ago

#2 @pento
6 years ago

#3 @pento
6 years ago

#4 @pento
6 years ago

#5 @pento
6 years ago

#6 @pento
6 years ago

#7 @pento
6 years ago

#8 @pento
6 years ago

#9 @GaryJ
6 years ago

#10 @pento
6 years ago

#11 @pento
6 years ago

#12 @pento
6 years ago

#13 @pento
6 years ago

#14 @jrf
6 years ago

#15 @pento
6 years ago

@pento
6 years ago

#16 @pento
6 years ago

#17 follow-up: ↓ 18 @jrf
6 years ago

#18 in reply to: ↑ 17 @pento
6 years ago

#19 @pento
6 years ago

#20 @pento
6 years ago

#21 @pento
6 years ago

#22 @pento
6 years ago