Make WordPress Core

#56791 closed task (blessed) (fixed)

Coding Standards fixes for WP 6.2

Reported by: desrosj's profile desrosj Owned by:
Milestone: 6.2 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch
Focuses: coding-standards Cc:

Description

Previously:

Attachments (11)

str_contains.patch (572 bytes) - added by TobiasBg 17 months ago.
See ticket:56969#comment:35 .
56791-_filter_query_attachment_filenames-fix-incorrect-dep.patch (1.1 KB) - added by jrf 17 months ago.
Fix incorrect deprecation version number
56791-always-declare-visibility-for-methods.patch (49.6 KB) - added by jrf 17 months ago.
Follow up to previous efforts to make sure visibility is declared on all methods. Note: this will be enforced by WPCS 3.0.0.
56791-always-declare-visibility-for-properties.patch (1.8 KB) - added by jrf 17 months ago.
Follow up to previous efforts to make sure visibility is declared on all properties. Note: this will be enforced by WPCS 3.0.0.
56791-always-use-parentheses-when-instantiating-an-object.patch (76.7 KB) - added by jrf 17 months ago.
Follow up to previous efforts for the same. Note: this will be enforced by WPCS 3.0.0.
56791-always-use-strict-comparisons-when-using-in_array.patch (2.1 KB) - added by jrf 17 months ago.
This fixes three out of the four currently flagged issues for in_array() usage. These three all do comparisons with strings, so all the more reason why it is imperative that a strict comparison is used.
56791-fix-spacing-for-incrementors-and-decrementors.patch (8.4 KB) - added by jrf 17 months ago.
Note: this will be enforced by WPCS 3.0.0.
56791-fix-incorrect-comment-closer-indentation.patch (973 bytes) - added by jrf 17 months ago.
Very minor fix, still should be made ;-)
56791-fix-indentation-of-multi-line-chained-method-call.patch (971 bytes) - added by jrf 17 months ago.
Again, very small fix, should still be made ;-)
56791-remove-redundant-semicolon-empty-statement.patch (747 bytes) - added by jrf 17 months ago.
Fixes an apparent typo
56791-fix-a-non-snake_case-function-name.patch (1.4 KB) - added by jrf 17 months ago.
Note: if I'm honest, this whole function declaration should be changed as it is now declared as a function in the global namespace, while only intended for one test, but that's outside the scope of this patch.

Download all attachments as: .zip

Change History (52)

#1 @desrosj
19 months ago

  • Milestone changed from Awaiting Review to 6.2

This ticket was mentioned in PR #2435 on WordPress/wordpress-develop by @kebbet.


19 months ago
#2

  • Keywords has-patch added

Maybe this PR can be included in 6.2, after being overseen in 6.0 and 6.1.

https://core.trac.wordpress.org/ticket/56791

#3 @kebbet
19 months ago

There is no consensus on how to use behaviour and behavior in Core atm. Is this something that should be adressed/fixed?

Version 0, edited 19 months ago by kebbet (next)

#4 @SergeyBiryukov
18 months ago

In 54752:

Coding Standards: Correct alignment in various files.

This fixes Equals sign not aligned with surrounding statements WPCS warnings, so that the output of composer format is clean.

Follow-up to [54445], [54476], [54494], [54522], [54652], [54687].

See #56791.

#5 @SergeyBiryukov
18 months ago

In 54753:

Coding Standards: Revert [54752] from the 6.1 branch.

This was supposed to be committed to trunk.

See #56791.

#6 @SergeyBiryukov
18 months ago

In 54754:

Coding Standards: Correct alignment in various files.

This fixes Equals sign not aligned with surrounding statements WPCS warnings, so that the output of composer format is clean.

Follow-up to [54445], [54476], [54494], [54522], [54652], [54687].

See #56791.

#7 follow-up: @TobiasBg
17 months ago

In references to ticket:56969#comment:33 and ticket:56969#comment:35 (pinging @peterwilsoncc and @adamsilverstein), I've attached a patch to replace a strpos() check with modern (and polyfilled) str_contains().

The question that arises: There are about 60 false === strpos( ... ) and 150 false !== strpos( ... ) in Core that could be replaced by ! str_contains( ... ) and str_contains( ... ). Should these be batch-replaced or rather left alone until the respective area of code is touched anyways? Likewise, possible str_starts_with() and str_ends_with() usage could be introduced.

#8 in reply to: ↑ 7 @SergeyBiryukov
17 months ago

Replying to TobiasBg:

The question that arises: There are about 60 false === strpos( ... ) and 150 false !== strpos( ... ) in Core that could be replaced by ! str_contains( ... ) and str_contains( ... ). Should these be batch-replaced or rather left alone until the respective area of code is touched anyways? Likewise, possible str_starts_with() and str_ends_with() usage could be introduced.

I am in favor of making a batch replacement to use str_contains(), str_starts_with(), and str_ends_with() where appropriate, for readability and consistency.

#9 @SergeyBiryukov
17 months ago

In 54870:

Coding Standards: Use HOUR_IN_SECONDS where appropriate.

This aims to clarify the time units for some time offset values.

Follow-up to [21996], [23823], [40108], [41626].

See #56791.

@jrf
17 months ago

Fix incorrect deprecation version number

#10 follow-up: @jrf
17 months ago

While updating the deprecated functions list in WordPressCS, I noticed a typo in the call to the _deprecated_function() function for the _filter_query_attachment_filenames() function.

I've attached a patch to fix it.

The issue was introduced in [54524] /cc @audrasjb

Even though it is a small fix, it is misleading, so I wonder if the patch should be backported ?

#11 in reply to: ↑ 10 ; follow-up: @SergeyBiryukov
17 months ago

Replying to jrf:

Even though it is a small fix, it is misleading, so I wonder if the patch should be backported ?

Thanks for the patch!

It looks like the issue is only in these two commits: [54524], [54534], i.e. in trunk and the 6.0 branch. Backports to other branches have the correct version already.

Backporting to the 6.0 branch makes sense to me.

#12 @SergeyBiryukov
17 months ago

In 54878:

Coding Standards: Correct the deprecation version for _filter_query_attachment_filenames().

Follow-up to [54524].

Props jrf.
See #56791.

#13 in reply to: ↑ 11 @SergeyBiryukov
17 months ago

Replying to SergeyBiryukov:

It looks like the issue is only in these two commits: [54524], [54534], i.e. in trunk and the 6.0 branch. Backports to other branches have the correct version already.

Backporting to the 6.0 branch makes sense to me.

Ah, it was before 6.1 was branched, so it should also be backported to the 6.1 branch now :)

#14 @jrf
17 months ago

Thanks @SergeyBiryukov And yes, I concur with your assessment - backports to 6.0 and 6.1 only (issue didn't exist before that).

#15 @SergeyBiryukov
17 months ago

In 54879:

Coding Standards: Correct the deprecation version for _filter_query_attachment_filenames().

Follow-up to [54524].

Props jrf.
Merges [54878] to the 6.1 branch.
See #56791.

#16 @SergeyBiryukov
17 months ago

In 54880:

Coding Standards: Correct the deprecation version for _filter_query_attachment_filenames().

Follow-up to [54524], [54534].

Props jrf.
Merges [54878] to the 6.0 branch.
See #56791.

#17 @audrasjb
17 months ago

Thanks for spotting and fixing this issue!

#18 @audrasjb
17 months ago

In 54881:

Coding Standards: Various brace indentation corrections.

Props mukesh27.
Fixes #57210.
See #56791.

#19 @audrasjb
17 months ago

In 54882:

Twenty Ten: Fixes brace indentation in loop-attachment template.

Props mukesh27.
See #57210, #56791.

@jrf
17 months ago

Follow up to previous efforts to make sure visibility is declared on all methods. Note: this will be enforced by WPCS 3.0.0.

@jrf
17 months ago

Follow up to previous efforts to make sure visibility is declared on all properties. Note: this will be enforced by WPCS 3.0.0.

@jrf
17 months ago

Follow up to previous efforts for the same. Note: this will be enforced by WPCS 3.0.0.

@jrf
17 months ago

This fixes three out of the four currently flagged issues for in_array() usage. These three all do comparisons with strings, so all the more reason why it is imperative that a strict comparison is used.

@jrf
17 months ago

Note: this will be enforced by WPCS 3.0.0.

@jrf
17 months ago

Very minor fix, still should be made ;-)

@jrf
17 months ago

Again, very small fix, should still be made ;-)

@jrf
17 months ago

Note: if I'm honest, this whole function declaration should be changed as it is now declared as a function in the global namespace, while only intended for one test, but that's outside the scope of this patch.

#20 @jrf
17 months ago

I've just uploaded 9 patches to fix a variety of coding standards issues. Most a very simple and easy merges. The only "big" ones are the "always use parentheses when instantiating an object" and "always declare visibility for methods" patches.

#21 @SergeyBiryukov
17 months ago

In 54889:

Coding Standards: Add visibility to methods in tests/phpunit/tests/.

Adds a public visibility to test fixtures, tests, data providers, and callbacks methods. This continues the previous efforts to make sure visibility is declared on all methods.

Note: This will be enforced by WPCS 3.0.0.

Follow-up to [51919], [52009], [52010].

Props jrf.
See #56791.

#22 @SergeyBiryukov
17 months ago

In 54890:

Coding Standards: Add visibility to properties in tests/phpunit/tests/.

Adds a private visibility to some test class properties where it was not explicitly specified. This continues the previous efforts to make sure visibility is declared on all properties.

Note: This will be enforced by WPCS 3.0.0.

Follow-up to [49184], [51919], [52009], [52010], [54889].

Props jrf.
See #56791.

#23 @SergeyBiryukov
17 months ago

In 54891:

Coding Standards: Always use parentheses when instantiating an object.

Note: This will be enforced by WPCS 3.0.0.

Props jrf.
See #56791.

#24 @SergeyBiryukov
17 months ago

In 54895:

Coding Standards: Always use strict type check for in_array().

This fixes the currently flagged WordPress.PHP.StrictInArray.MissingTrueStrict issues:

  • Not using strict comparison for in_array; supply true for third argument.

These all do comparisons with strings, so all the more reason why it is imperative that a strict comparison is used.

Follow-up to [47550], [47557], [54155], [53480].

Props jrf.
See #56791.

#25 @SergeyBiryukov
17 months ago

In 54896:

Coding Standards: Fix spacing for incrementors and decrementors in various files.

Note: This will be enforced by WPCS 3.0.0.

Props jrf.
See #56791.

#26 @audrasjb
17 months ago

In 54897:

Coding Standards: Use consistent markup for line break tags on update-core.php.

This changeset replaces <br/> with <br /> on various places, as per WordPress Coding Standards.
See https://developer.wordpress.org/coding-standards/wordpress-coding-standards/html/#self-closing-elements

Follow-up to [54062].

Props rajanpanchal2028, alberuni-azad, sabernhardt.
Fixes #57226.
See #56791.

#27 @SergeyBiryukov
17 months ago

In 54902:

Twenty Seventeen: Fix comment indentation in twentyseventeen_setup().

Follow-up to [38833], [42343]

Props jrf.
See #56791.

#28 @SergeyBiryukov
17 months ago

In 54915:

Coding Standards: Fix indentation of multi-line chained method call in test_json_error_with_status().

Follow-up to [34928], [42228].

Props jrf.
See #56791.

#29 @SergeyBiryukov
17 months ago

In 54916:

Coding Standards: Remove redundant semicolon after get_template_hierarchy().

Follow-up to [54269].

Props jrf.
See #56791.

#30 @SergeyBiryukov
17 months ago

In 54917:

Coding Standards: Fix a non-snake_case function name in WP_Block tests.

As the filter is only intended for a single test, it can be converted to a closure instead of being declared as a function in the global namespace. The remove_filter() part is redundant, as WP_UnitTestCase_Base saves the state of filter-related globals at set_up() and restores them on tear_down().

Follow-up to [54175].

Props jrf, SergeyBiryukov.
See #56791.

#31 @SergeyBiryukov
17 months ago

In 54920:

Coding Standards: Remove a one-time $loading variable in get_avatar().

This aims to bring consistency between two similar code fragments.

Follow-up to [47554], [53480], [54895].

See #56791.

#32 @SergeyBiryukov
16 months ago

In 55022:

Coding Standards: Add visibility to Tests_Dependencies::test_enqueue_before_register().

Adds a public visibility to a test where it was not explicitly specified. This continues the previous efforts to make sure visibility is declared on all methods.

Note: This will be enforced by WPCS 3.0.0.

Follow-up to [51919], [52009], [52010], [52338], [54889].

Props jrf.
See #56791.

#33 @SergeyBiryukov
16 months ago

In 55033:

Coding Standards: Correct alignment in wp-includes/option.php.

This fixes an Equals sign not aligned with surrounding statements WPCS warning, so that the output of composer format is clean.

Follow-up to [54948].

See #56791.

#35 @SergeyBiryukov
15 months ago

In 55136:

Coding Standards: Allow some parameters with reserved keywords in wp-includes/compat.php.

Parameter names for PHP polyfills in WordPress core need to 100% match the native PHP parameter names. Otherwise using named parameters with those functions could cause fatal errors for installations where the polyfills kick in.

This commit adds inline comments instructing PHPCS to ignore parameters with reserved keywords in the affected functions that should not be renamed:

  • $string parameter in mb_substr() and mb_strlen()
  • $array parameter in array_key_first() and array_key_last()

This resolves a few WPCS warnings along the lines of:

It is recommended not to use reserved keyword "string" as function parameter name. Found: $string

Follow-up to [7140], [10707], [17603], [17621], [32114], [52038], [53365].

Props jrf.
See #56788, #56791.

#36 @SergeyBiryukov
15 months ago

In 55145:

Coding Standards: Bring some consistency to the order of attributes in password fields.

Follow-up to [11359], [13592], [13696], [33023], [33246], [33353], [41556], [46256], [49248], [53111], [55094].

See #56791.

#37 follow-up: @SergeyBiryukov
14 months ago

In 55308:

Coding Standards: Rename $comment_ID variable to $comment_id in various files.

This resolves 80+ WPCS warnings in core:

Variable "$comment_ID" is not in valid snake_case format

While matching the database field of the same name, the $comment_ID variable did not follow the WordPress coding standards, and is now renamed to address that.

This affects:

  • Function parameters in:
    • get_comment_author()
    • comment_author()
    • get_comment_author_email()
    • comment_author_email()
    • get_comment_author_link()
    • comment_author_link()
    • get_comment_author_IP()
    • comment_author_IP()
    • get_comment_author_rl()
    • comment_author_url()
    • get_comment_date()
    • comment_date()
    • get_comment_excerpt()
    • comment_excerpt()
    • get_comment_text()
    • comment_text()
    • get_comment_time()
    • comment_time()
    • get_comment_type()
    • get_page_of_comment()
    • wp_new_comment_notify_moderator()
    • wp_new_comment_notify_postauthor()
    • get_commentdata()
  • Internal variables in:
    • get_comment_ID()
    • wp_new_comment()
    • wp_xmlrpc_server::wp_deleteComment()
    • wp_xmlrpc_server::wp_editComment()
    • wp_xmlrpc_server::wp_newComment()
    • wp_xmlrpc_server::pingback_ping()
  • Hook parameters in:
    • get_comment_author
    • comment_author
    • get_comment_author_email
    • author_email
    • get_comment_author_link
    • get_comment_author_IP
    • get_comment_author_url
    • comment_url
    • get_comment_excerpt
    • comment_excerpt
    • get_comment_ID
    • get_comment_type
    • get_page_of_comment
    • comment_{$new_status}_{$comment->comment_type}
    • comment_post
    • notify_moderator
    • notify_post_author
    • commentrss2_item
    • xmlrpc_call_success_wp_deleteComment
    • xmlrpc_call_success_wp_editComment
    • xmlrpc_call_success_wp_newComment
    • pingback_post

Note: The name change only affects variable names and DocBlocks.

The change does not affect:

  • comment_ID as the $orderby value in WP_Comment_Query::__construct()
  • comment_ID as the $orderby value in WP_Comment::get_children()
  • comment_ID as part of $commentarr parameter in wp_update_comment()

The associated array keys still match the database field.

Follow-up to [53723].

Props krunal265, costdev, SergeyBiryukov.
Fixes #57671. See #56791.

#38 in reply to: ↑ 37 ; follow-up: @azaozz
14 months ago

Replying to SergeyBiryukov:

While matching the database field of the same name, the $comment_ID variable did not follow the WordPress coding standards...

I like this change, seems to make sense imho. However there is nothing about this in the WP coding standards (as far as I see). Seems the implementation of the standards in WPCS is wrong :)

#39 in reply to: ↑ 38 @jrf
14 months ago

Replying to azaozz:

Replying to SergeyBiryukov:

While matching the database field of the same name, the $comment_ID variable did not follow the WordPress coding standards...

I like this change, seems to make sense imho. However there is nothing about this in the WP coding standards (as far as I see). Seems the implementation of the standards in WPCS is wrong :)

From the handbook:

Use lowercase letters in variable, action/filter, and function names (never camelCase). Separate words via underscores. Don’t abbreviate variable names unnecessarily; let the code be unambiguous and self-documenting.

Ref: https://developer.wordpress.org/coding-standards/wordpress-coding-standards/php/#naming-conventions

Note: this phrase has been in the handbook since as long as I remember. Also see the original import to the Git repo from five years ago in which the exact same phrase is included.

Conclusion: WPCS follows the handbook to the letter and is doing nothing wrong here.

#40 @SergeyBiryukov
14 months ago

In 55420:

Coding Standards: Use strict comparison in bundled themes' PHP files.

This addresses all the remaining WPCS warnings in bundled themes.

Includes using the correct type when checking the number of comments, as get_comments_number() returns a numeric string, not an integer.

Follow-up to [41285], [44562], [47941].

Props aristath, poena, afercia, SergeyBiryukov.
See #56791.

#41 @hellofromTonya
14 months ago

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

The last schedule 6.3 beta happened today and next week is RC1. I'm closing this ticket for the cycle. However, please feel to reopen if coding standards fixes are needed for 6.2.

#57839 is now open for the continuation of work in the 6.3 cycle.

Thank you everyone for your contributions :)

Note: See TracTickets for help on using tickets.