Make WordPress Core

Opened 2 years ago

Closed 22 months ago

Last modified 3 months ago

#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 2 years ago.
See ticket:56969#comment:35 .
56791-_filter_query_attachment_filenames-fix-incorrect-dep.patch (1.1 KB) - added by jrf 2 years ago.
Fix incorrect deprecation version number
56791-always-declare-visibility-for-methods.patch (49.6 KB) - added by jrf 2 years 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 2 years 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 2 years 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 2 years 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 2 years ago.
Note: this will be enforced by WPCS 3.0.0.
56791-fix-incorrect-comment-closer-indentation.patch (973 bytes) - added by jrf 2 years ago.
Very minor fix, still should be made ;-)
56791-fix-indentation-of-multi-line-chained-method-call.patch (971 bytes) - added by jrf 2 years ago.
Again, very small fix, should still be made ;-)
56791-remove-redundant-semicolon-empty-statement.patch (747 bytes) - added by jrf 2 years ago.
Fixes an apparent typo
56791-fix-a-non-snake_case-function-name.patch (1.4 KB) - added by jrf 2 years 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 (54)

#1 @desrosj
2 years ago

  • Milestone changed from Awaiting Review to 6.2

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


2 years 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
2 years ago

comment on incorrect ticket

Last edited 2 years ago by kebbet (previous) (diff)

#4 @SergeyBiryukov
2 years 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
2 years ago

In 54753:

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

This was supposed to be committed to trunk.

See #56791.

#6 @SergeyBiryukov
2 years 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
2 years 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
2 years 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
2 years 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
2 years ago

Fix incorrect deprecation version number

#10 follow-up: @jrf
2 years 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
2 years 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
2 years 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
2 years 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
2 years 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
2 years 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
2 years 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
2 years ago

Thanks for spotting and fixing this issue!

#18 @audrasjb
2 years ago

In 54881:

Coding Standards: Various brace indentation corrections.

Props mukesh27.
Fixes #57210.
See #56791.

#19 @audrasjb
2 years ago

In 54882:

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

Props mukesh27.
See #57210, #56791.

@jrf
2 years 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
2 years 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
2 years ago

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

@jrf
2 years 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
2 years ago

Note: this will be enforced by WPCS 3.0.0.

@jrf
2 years ago

Very minor fix, still should be made ;-)

@jrf
2 years ago

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

@jrf
2 years ago

Fixes an apparent typo

@jrf
2 years 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
2 years 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
2 years 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
2 years 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
2 years 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
2 years 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
2 years 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
2 years 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
2 years ago

In 54902:

Twenty Seventeen: Fix comment indentation in twentyseventeen_setup().

Follow-up to [38833], [42343]

Props jrf.
See #56791.

#28 @SergeyBiryukov
2 years 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
2 years ago

In 54916:

Coding Standards: Remove redundant semicolon after get_template_hierarchy().

Follow-up to [54269].

Props jrf.
See #56791.

#30 @SergeyBiryukov
2 years 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
2 years 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
2 years 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
2 years 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
23 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
23 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
22 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
22 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
22 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
22 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
22 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 :)

@SergeyBiryukov commented on PR #2435:


4 months ago
#42

Thanks for the PR! Merged in r58888.

@SergeyBiryukov commented on PR #3873:


3 months ago
#43

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!

Note: See TracTickets for help on using tickets.