Coding Standards fixes for WP 6.2

[desrosj]

Previously:

• #55647 (6.1)
• #54728 (6.0)
• #53359 (5.9)
• #52627 (5.8)
• #51799 (5.7)
• #50767 (5.6)
• #49542 (5.5)
• #49222 (5.4)
• #47632 (5.3)
• #45934 (5.1)

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

[kebbet]

comment on incorrect ticket

[SergeyBiryukov]

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.

[SergeyBiryukov]

In 54753:

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

This was supposed to be committed to trunk.

See #56791.

[SergeyBiryukov]

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.

[TobiasBg]

See ticket:56969#comment:35 .

[TobiasBg]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

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]

Fix incorrect deprecation version number

[jrf]

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 ?

[SergeyBiryukov]

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.

[SergeyBiryukov]

In 54878:

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

Follow-up to [54524].

Props jrf.
See #56791.

[SergeyBiryukov]

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

[jrf]

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

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

[audrasjb]

Thanks for spotting and fixing this issue!

[audrasjb]

In 54881:

Coding Standards: Various brace indentation corrections.

Props mukesh27.
Fixes #57210.
See #56791.

[audrasjb]

In 54882:

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

Props mukesh27.
See #57210, #56791.

[jrf]

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]

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]

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

[jrf]

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]

Note: this will be enforced by WPCS 3.0.0.

[jrf]

Very minor fix, still should be made ;-)

[jrf]

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

[jrf]

Fixes an apparent typo

[jrf]

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.

[jrf]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

[audrasjb]

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.

[SergeyBiryukov]

In 54902:

Twenty Seventeen: Fix comment indentation in twentyseventeen_setup().

Follow-up to [38833], [42343]

Props jrf.
See #56791.

[SergeyBiryukov]

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.

[SergeyBiryukov]

In 54916:

Coding Standards: Remove redundant semicolon after get_template_hierarchy().

Follow-up to [54269].

Props jrf.
See #56791.

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

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

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

[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 :)

[jrf]

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.

[SergeyBiryukov]

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.

[hellofromTonya]

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

Thanks for the PR! Merged in r58888.

This will need tests before it can be merged:

Indeed, this is basically still a draft where I cherry-pick certain changes after a closer review, making sure they are covered by new or existing tests. I will investigate anything related to wp_parse_args() and WP_Query, thanks!