Make WordPress Core

Opened 8 months ago

Closed 5 months ago

Last modified 3 months ago

#58831 closed task (blessed) (fixed)

Coding Standards fixes for WP 6.4

Reported by: audrasjb's profile audrasjb Owned by:
Milestone: 6.4 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch has-unit-tests
Focuses: coding-standards Cc:

Description

Previously:

Attachments (23)

58831-add-missing-translators-comment.patch (1.6 KB) - added by jrf 8 months ago.
58831-align-equal-signs-for-consecutive-statements.patch (4.7 KB) - added by jrf 8 months ago.
58831-all-methods-should-have-visibility-declared.patch (1.0 KB) - added by jrf 8 months ago.
58831-always-use-parentheses-for-class-instantiation.patch (923 bytes) - added by jrf 8 months ago.
58831-no-precision-alignment.patch (884 bytes) - added by jrf 8 months ago.
58831-no-trailing-whitespace.patch (776 bytes) - added by jrf 8 months ago.
58831-use-correct-case-for-class-name.patch (807 bytes) - added by jrf 8 months ago.
58831-Modernize-use-__DIR__-instead-of-dirname-__FILE__.patch (790 bytes) - added by jrf 8 months ago.
58831-add-missing-escape.patch (569 bytes) - added by viralsampat 6 months ago.
58831-add-missing-escape-wp-admin-plugins.patch (433 bytes) - added by viralsampat 6 months ago.
58831-add-missing-global-wpdb.patch (472 bytes) - added by viralsampat 6 months ago.
58831-add-missing-global-wpdb-and-missing-escaping.patch (2.9 KB) - added by viralsampat 6 months ago.
58831-add-missing-escape-wp-includes.patch (1.8 KB) - added by viralsampat 6 months ago.
58831-missing-escaping.patch (1.7 KB) - added by himanshuc 6 months ago.
58831-escaping-function-missing.patch (922 bytes) - added by nidhidhandhukiya 6 months ago.
58831.patch (2.3 KB) - added by viralsampat 6 months ago.
I have checked above mentioned issue and founds another file. Here, I have added its patch.
58831-escaping-function-missing-twentyeleven.patch (555 bytes) - added by himanshuc 6 months ago.
58831-escaping-function-missing.2.patch (907 bytes) - added by viralsampat 6 months ago.
I have added another patch
58831-escaping-function-missing-twentythirteen-theme.patch (843 bytes) - added by nidhidhandhukiya 5 months ago.
58831-escaping-function-missing-twentyten-theme.patch (549 bytes) - added by nidhidhandhukiya 4 months ago.
58831-escaping-function-missing-in-class-wp-customize-date-time-control.php.patch (593 bytes) - added by nidhidhandhukiya 4 months ago.
58831.2.patch (2.0 KB) - added by viralsampat 4 months ago.
I have checked above mentioned issue and founds new file. I have added another patch
58831-escaping-function-missing-in-widget.php.diff (4.9 KB) - added by nidhidhandhukiya 3 months ago.

Download all attachments as: .zip

Change History (96)

#1 follow-up: @jrf
8 months ago

I've just uploaded 8 pretty small patches.

About half are for things which are checked by WPCS, but flagged as a warning (which doesn't fail the build) and which have slipped into the codebase unnoticed.

The other half are things which are not (yet) checked by WPCS, but are rules/best practices which have been fixed up before, for which new issues have slipped into the codebase.

#2 @audrasjb
8 months ago

Thanks! These patches all look good to me.
Do you plan to commit them? If so, I think they're all good to ship, now that 6.3 has been branched.

#3 @jrf
8 months ago

Do you plan to commit them?

I'm neck-deep in WPCS at the moment. I've asked @SergeyBiryukov if he'd be so kind as to commit these.

#4 in reply to: ↑ 1 ; follow-up: @SergeyBiryukov
8 months ago

Replying to jrf:

I've just uploaded 8 pretty small patches.

Thank you!

As previously noted in comment:1:ticket:50085, the change to __DIR__ in wp-tests-config-sample.php was previously reverted in [47201] to avoid breaking unit tests created with the wp scaffold plugin WP-CLI command, see comment:15:ticket:48082 and #49377 for details. It might be worth checking if that concern is still relevant.

Other patches look good to me at a glance :)

#5 in reply to: ↑ 4 @jrf
8 months ago

Replying to SergeyBiryukov:

As previously noted in comment:1:ticket:50085, the change to __DIR__ in wp-tests-config-sample.php was previously reverted in [47201] to avoid breaking unit tests created with the wp scaffold plugin WP-CLI command, see comment:15:ticket:48082 and #49377 for details. It might be worth checking if that concern is still relevant.

Ah, that's good to know. We may need to put an exception in place for that file once WPCS 3.0.0 comes out (which will flag this).

Having read up on the discussion, I don't think this concern will ever really go away completely, as, even though the WP-CLI scaffold plugin script has been updated, the problem is not the script itself, but the fact that there are countless plugins using a copy of that script and getting all those copies updated to the latest version is a manual process (and in the mean time CI workflows would break).

#6 @SergeyBiryukov
8 months ago

In 56273:

Coding Standards: Correct equals sign alignment in various files.

This resolves a few WPCS warnings:

Equals sign not aligned with surrounding statements

so that the output of composer format is clean.

Follow-up to [55971], [56033], [56056], [56143], [56214].

Props jrf.
See #58831.

#7 @SergeyBiryukov
7 months ago

In 56276:

I18N: Add missing translator comment in WP_Upgrader::generic_strings().

This resolves a WPCS warning:

A gettext call containing placeholders was found, but was not accompanied by a "translators:" comment on the line above to clarify the meaning of the placeholders.

Includes moving wp-content out of the translatable string in a similar message in _wp_delete_all_temp_backups().

Follow-up to [55720], [56117].

Props jrf.
See #58831.

#8 @SergeyBiryukov
7 months ago

In 56281:

Coding Standards: Always use parentheses for class instantiation.

This addresses a new stdClass() instance in _get_non_cached_ids() tests.

Follow-up to [55543].

Props jrf.
See #58831.

#9 @SergeyBiryukov
7 months ago

In 56283:

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

This resolves a WPCS warning:

Found precision alignment of 1 space.

Follow-up to [55988].

Props jrf.
See #58831.

#10 @SergeyBiryukov
7 months ago

In 56285:

Coding Standards: Remove trailing tabs in wp-admin/about.php.

This resolves a WPCS warning:

Found precision alignment of 2 spaces.

Follow-up to [56263].

Props jrf.
See #58831, #58067.

#11 @SergeyBiryukov
7 months ago

In 56301:

Coding Standards: Always declare visibility for class methods.

This adds a missing public keyword for WP_HTML_Tag_Processor::get_attribute_names_with_prefix().

Follow-up to [55203].

Props jrf.
See #58831.

#12 follow-up: @dmsnell
7 months ago

@SergeyBiryukov thanks for the update - do you plan on creating the required backport to Gutenberg for your change to the Tag Processor?

#13 in reply to: ↑ 12 @SergeyBiryukov
7 months ago

Replying to dmsnell:

do you plan on creating the required backport to Gutenberg for your change to the Tag Processor?

Yup, created PR 52954. Thanks!

#16 @SergeyBiryukov
7 months ago

In 56319:

Coding Standards: Use correct case for class name in WP_Http tests.

Follow-up to [8516], [54968].

Props jrf.
See #58831.

#17 @SergeyBiryukov
7 months ago

In 56321:

Coding Standards: Use strict comparison in wp-includes/class-wp-roles.php.

Follow-up to [25695], [41625].

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

#18 @SergeyBiryukov
7 months ago

In 56324:

Coding Standards: Use strict comparison in wp-includes/feed-atom-comments.php.

Follow-up to [9818].

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

#19 @SergeyBiryukov
7 months ago

In 56325:

Coding Standards: Use strict comparison in wp-includes/formatting.php.

Follow-up to [1345], [4112], [6974], [24214], [25055], [28831], [32863].

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

#20 @SergeyBiryukov
7 months ago

In 56326:

Coding Standards: Use strict comparison in wp-includes/functions.php.

Follow-up to [5999], [6342], [7406], [8369], [10322], [11288], [11332], [11597], [12405], [13569], [14649], [15806], [19773], [26449], [26926], [39831], [40124].

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

#21 @SergeyBiryukov
7 months ago

In 56333:

Coding Standards: Use strict comparison in wp-includes/option.php.

Follow-up to [8784], [25109].

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

#22 @SergeyBiryukov
7 months ago

In 56359:

Coding Standards: Use strict comparison in wp-includes/revision.php.

Follow-up to [24414], [38118], [38433].

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

#23 @SergeyBiryukov
7 months ago

In 56360:

Coding Standards: Rewrite loose comparison in wp_list_categories().

A truthy check is more in line with similar checks elsewhere, including conditionals in the same exact code block.

Follow-up to [10275], [34696].

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

#24 @SergeyBiryukov
7 months ago

In 56361:

Coding Standards: Use strict comparison in wp-includes/class-wp-image-editor.php.

Follow-up to [22094].

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

#25 @SergeyBiryukov
7 months ago

In 56362:

Coding Standards: Use strict comparison in wp-includes/class-wp.php.

Includes minor code layout fixes for better readability.

Follow-up to [1043], [2534], [2584], [2627], [2958], [3252], [3564], [21818], [37356].

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

#26 @audrasjb
7 months ago

#58988 was marked as a duplicate.

#27 @SergeyBiryukov
7 months ago

In 56377:

Coding Standards: Use strict comparison in wp-includes/kses.php.

Follow-up to [649], [2896], [3418], [8386], [20540], [47219], [54933].

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

#28 @SergeyBiryukov
7 months ago

In 56394:

Coding Standards: Use strict comparison in wp-includes/cron.php.

Includes minor code layout fixes for better readability.

Follow-up to [3634], [4189].

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

#29 @SergeyBiryukov
7 months ago

In 56395:

Coding Standards: Bring more consistency to Last-Modified and ETag checks.

This updates two fragments for sending a 304 Not Modified header to better align with each other by using consistent variable names and formatting.

Follow-up to [1036], [1037], [1043], [2534], [2584], [2627], [12603], [12936], [56362].

See #58831.

#30 @SergeyBiryukov
7 months ago

In 56396:

Coding Standards: Use strict comparison in wp-admin/includes/class-wp-importer.php.

Follow-up to [14760], [50658].

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

#31 @SergeyBiryukov
7 months ago

In 56400:

Coding Standards: Use strict comparison in wp-admin/includes/image-edit.php.

Follow-up to [11911], [11965], [11984], [12155], [12163], [22094].

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

#32 @SergeyBiryukov
7 months ago

In 56411:

Coding Standards: Improve variable names in wp_save_image().

This resolves a few WPCS warnings:

Variable "$sX" is not in valid snake_case format, try "$s_x"
Variable "$sY" is not in valid snake_case format, try "$s_y"

The $sX and $sY variables are renamed to $original_width and $original_height, respectively.

Additionally, the $fwidth and $fheight variables are renamed to $full_width and $full_height, for clarity.

Follow-up to [11965], [22094], [56400].

See #58831.

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


6 months ago
#33

### PHPCS: improve organisation of the PHPCompatibility ruleset

No functional changes.

This commit:

  • Adds section headers to the ruleset file.
  • Organizes all directives in their respective sections.

### PHPCS: remove unnecessary directives in PHPCompatibility ruleset

This commit:

  • Removes the unnecessary exclusion patterns for the node_modules and the vendor directory.

As this ruleset only scans the src directory, those directories would never be scanned anyway.

  • Removes the selective excludes related to the Random Compat package.

This package was removed in WP 6.3, so these excludes are no longer necessary.

---

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

#34 @SergeyBiryukov
6 months ago

In 56420:

Coding Standards: Use strict comparison in wp-admin/includes/meta-boxes.php.

Follow-up to [38], [647], [2890], [3570], [11815], [11816], [31550], [52620].

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

#35 @SergeyBiryukov
6 months ago

In 56430:

Coding Standards: Use strict comparison in wp-includes/ms-files.php.

Follow-up to [12603], [12936], [56395].

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

#36 @SergeyBiryukov
6 months ago

In 56438:

Coding Standards: Use strict comparison in wp-includes/ms-site.php.

Follow-up to [12603], [43548], [43654], [44472].

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

#37 @SergeyBiryukov
6 months ago

In 56466:

Coding Standards: Use strict comparison in wp-includes/class-wp-widget.php.

Follow-up to [10764], [10912], [11427].

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

#38 @SergeyBiryukov
6 months ago

In 56511:

Coding Standards: Use strict comparison in wp-includes/class-wp-hook.php.

Follow-up to [4955], [38571].

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

#39 @SergeyBiryukov
6 months ago

In 56521:

Coding Standards: Correct equals sign alignment in various files.

This resolves a few WPCS warnings:

Equals sign not aligned with surrounding statements

so that the output of composer format is clean.

Follow-up to [56276], [56302].

Props jrf.
See #59161, #58831.

#40 @SergeyBiryukov
6 months ago

In 56536:

Coding Standards: Remove superfluous blank lines at the end of various files.

Note: This is enforced by WPCS 3.0.0.

Props jrf.
See #59161, #58831.

@viralsampat
6 months ago

I have checked above mentioned issue and founds another file. Here, I have added its patch.

#41 @SergeyBiryukov
6 months ago

In 56547:

Coding Standards: Remove superfluous blank lines at the end of various classes.

Note: This is enforced by WPCS 3.0.0.

Follow-up to [56536].

Props jrf.
See #59161, #58831.

#42 @SergeyBiryukov
6 months ago

In 56548:

Coding Standards: Remove superfluous blank lines at the end of various functions.

Note: This is enforced by WPCS 3.0.0.

Follow-up to [56536], [56547].

Props jrf.
See #59161, #58831.

#43 @SergeyBiryukov
6 months ago

In 56549:

Coding Standards: Use pre-increment/decrement for stand-alone statements.

Note: This is enforced by WPCS 3.0.0:

  1. There should be no space between an increment/decrement operator and the variable it applies to.
  2. Pre-increment/decrement should be favoured over post-increment/decrement for stand-alone statements. “Pre” will in/decrement and then return, “post” will return and then in/decrement. Using the “pre” version is slightly more performant and can prevent future bugs when code gets moved around.

References:

Props jrf.
See #59161, #58831.

#44 @SergeyBiryukov
6 months ago

In 56551:

Coding Standards: Correct spacing for spread operators.

No space allowed between the operator and the variable it applies to.

Note: This is enforced by WPCS 3.0.0.

Follow-up to [46133].

Props jrf.
See #59161, #58831.

#45 @SergeyBiryukov
6 months ago

In 56552:

Code Modernization: Use dirname() with the $levels parameter.

PHP 7.0 introduced the $levels parameter to the dirname() function, which means nested calls to dirname() are no longer needed.

Note: This is enforced by WPCS 3.0.0.

Reference: PHP Manual: dirname().

Follow-up to [56141].

Props jrf.
See #59161, #58831.

#46 @SergeyBiryukov
6 months ago

In 56559:

Coding Standards: Include one space after function keyword for closures.

Note: This is enforced by WPCS 3.0.0.

Reference: WPCS: PR #2328 Core: properly check formatting of function declaration statements.

Props jrf.
See #59161, #58831.

#47 @SergeyBiryukov
6 months ago

In 56586:

Coding Standards: Restore more descriptive variable names in a few class methods.

When various methods parameters in child classes were renamed to $item to match the parent class for PHP 8 named parameter support, most of the methods restored the more descriptive, specific name at the beginning for better readability, with several exceptions for methods consisting only of a few lines.

To avoid confusion about why some methods do that and some don't, this commit aims to bring more consistency to the code, specifically in list tables' ::column_default() methods.

Follow-up to [51728], [51737], [51786].

See #58831.

#48 follow-up: @jrf
6 months ago

@SergeyBiryukov Re commit [56586] - FYI: the proposed change in ticket #59232 should also help in making it clearer for the future that the signatures of those methods need to stay in sync with the signature of the parent method.

@viralsampat
6 months ago

I have added another patch

#49 in reply to: ↑ 48 ; follow-up: @SergeyBiryukov
6 months ago

Replying to jrf:

Re commit [56586] - FYI: the proposed change in ticket #59232 should also help in making it clearer for the future that the signatures of those methods need to stay in sync with the signature of the parent method.

Thanks! I have a draft PR there as well as part of coding sessions with @poena, @andrea, and @aristath :)

#50 in reply to: ↑ 49 @jrf
6 months ago

Replying to SergeyBiryukov:

Thanks! I have a draft PR there as well as part of coding sessions with @poena, @andrea, and @aristath :)

Awesome!

#51 @SergeyBiryukov
6 months ago

In 56596:

Coding Standards: Remove extra parentheses in a few str_contains() conditionals.

Follow-up to [55988].

Props Cybr.
See #58831.

#52 @SergeyBiryukov
6 months ago

In 56598:

Coding Standards: Fix a few newly introduced WPCS issues.

Follow-up to [56515], [56557], [56560].

Props jrf.
See #59161, #58831.

#53 @audrasjb
5 months ago

In 56632:

Coding Standards: Add missing escaping functions in wp-admin/export.php

Props viralsampat.
See #58831.

#54 @SergeyBiryukov
5 months ago

In 56633:

Coding Standards: Escape the whole attribute in wp-admin/export.php.

It is best to always escape the complete value of an attribute, not a partial value, as otherwise the escaping could be (partially) undone when the values are joined together.

While the hardcoded hyphen in this case don't necessarily create that risk, it may change to a value which could be problematic, so making it a habit to escape the value in one go is best practice.

Escaping the complete value also means that a single esc_attr() call can be used instead of two.

Follow-up to [14444], [16652], [55616], [56632].

See #58831.

@jrf commented on PR #5042:


5 months ago
#55

Rebased to get this passed the merge conflict.

#56 @jrf
5 months ago

Anything I can do to move GH PR #5042 forward ?

#57 @SergeyBiryukov
5 months ago

In 56666:

Coding Standards: Improve organization of the PHPCompatibility ruleset.

This commit:

  • Adds section headers to the ruleset file.
  • Organizes all directives in their respective sections.

No functional changes.

Props jrf.
See #58831.

#58 @SergeyBiryukov
5 months ago

In 56667:

Coding Standards: Remove unnecessary directives in the PHPCompatibility ruleset.

This commit:

  • Removes the unnecessary exclusion patterns for the node_modules and vendor directories. As this ruleset only scans the src directory, those directories would never be scanned anyway.
  • Removes the selective excludes related to the random_compat package. This package was removed in WP 6.3, so these excludes are no longer necessary.

Follow-up to [46290], [56141].

Props jrf.
See #58831.

@SergeyBiryukov commented on PR #5042:


5 months ago
#59

Thanks for the PR! Merged in r56666 and r56667.

@jrf commented on PR #5042:


5 months ago
#60

Thank you @SergeyBiryukov !

#61 @SergeyBiryukov
5 months ago

In 56669:

Build/Test Tools: Remove random_compat from PHPCS and PHPUnit configuration files.

This package was removed in WP 6.3, so these exclusion entries are no longer necessary.

Follow-up to [42346], [42665], [49797], [56141], [56667].

See #58831, #58955.

#62 @SergeyBiryukov
5 months ago

In 56680:

Coding Standards: Fix a few newly introduced WPCS issues.

Follow-up to [56570], [56573], [56589], [56604], [56612], [56620], [56629], [56631], [56638], [56642], [56644], [56649].

Props jrf.
See #59161, #58831.

#63 @SergeyBiryukov
5 months ago

In 56692:

Coding Standards: Fix a few newly introduced WPCS issues.

Follow-up to [56683], [56689].

See #59161, #58831.

#64 @SergeyBiryukov
5 months ago

In 56694:

Code Modernization: Rename parameters that use reserved keywords in phpunit/tests/media.php.

While using reserved PHP keywords as parameter name labels is allowed, in the context of function calls using named parameters in PHP 8.0+, this will easily lead to confusion. To avoid that, it is recommended not to use reserved keywords as function parameter names.

This commit renames the $match parameter to $matches in shortcode image tests.

Note: This is enforced by WPCS 3.0.0.

Follow-up to [56693].

See #58831.

#65 @SergeyBiryukov
5 months ago

In 56732:

Bootstrap/Load: Remove a redundant continue statement in add_magic_quotes().

Follow-up to [48205], [48440].

Props Cybr.
See #58831.

#66 @SergeyBiryukov
5 months ago

In 56798:

Coding Standards: Correct alignment in wp-admin/user-edit.php.

This resolves a WPCS warning:

Array double arrow not aligned correctly;
expected 1 space(s) between "'type'" and double arrow, but found 15.

Follow-up to [56570], [56680].

Props jrf.
See #59161, #58831.

#67 follow-up: @spacedmonkey
5 months ago

There is a false !== strpos in wp_render_layout_support_flag. This could use str_contains

#68 in reply to: ↑ 67 @dmsnell
5 months ago

Replying to spacedmonkey:

There is a false !== strpos in wp_render_layout_support_flag. This could use str_contains

#59622 covers this line in its patch, so there should be no need to separately address it here.

#69 @hellofromTonya
5 months ago

#59650 is now open for 6.5.

#70 @hellofromTonya
5 months ago

The RC1 release party is about to start. Closing this as fixed. Thank you everyone for your contributions :)

#71 @hellofromTonya
5 months ago

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

@viralsampat
4 months ago

I have checked above mentioned issue and founds new file. I have added another patch

#72 @kebbet
4 months ago

This ticket is closed, any new patches and PR should be directed to the current issue for 6.5: #59650. Thanks!

#73 @SergeyBiryukov
3 months ago

Hi @nidhidhandhukiya, thanks for the patch!

Please note that $esc_number, as the name suggests, is already escaped on line 1711 directly above.

This ticket is closed, any new patches should be added to the current issue for 6.5: #59650. Thanks!

Note: See TracTickets for help on using tickets.