WordPress.org

Make WordPress Core

Opened 7 weeks ago

Closed 7 weeks ago

#48142 closed defect (bug) (fixed)

Let's do some `phpcbf` cleanup...

Reported by: whyisjake Owned by: whyisjake
Milestone: 5.3 Priority: normal
Severity: normal Version: 5.3
Component: General Keywords: has-patch needs-refresh
Focuses: Cc:
PR Number:

Description

There are a couple of regressions to cleanup that have been flagged by the linter tools.

Attachments (4)

48142.diff (2.5 KB) - added by whyisjake 7 weeks ago.
48142.2.diff (888 bytes) - added by whyisjake 7 weeks ago.
CS-network.php.diff (752 bytes) - added by garrett-eclipse 7 weeks ago.
CS improvement for network.php
CS-network.php.2.diff (6.7 KB) - added by garrett-eclipse 7 weeks ago.
Updated network.php patch to address comment convention (Capital first letter, punctuation at end) Thanks @jrf

Download all attachments as: .zip

Change History (18)

@whyisjake
7 weeks ago

#1 @whyisjake
7 weeks ago

  • Keywords has-patch added
  • Version set to trunk

#2 @whyisjake
7 weeks ago

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

In 46312:

General: Linter cleanup

phpcbf was able to clean up a few files. Tests were breaking as a result of code formatting.

Fixes #48142
Props whyisjake

#3 @whyisjake
7 weeks ago

  • Keywords commit added; has-patch removed
  • Milestone changed from Awaiting Review to 5.3

@whyisjake
7 weeks ago

#4 @whyisjake
7 weeks ago

In 46313:

General: Further phpcs cleanup.

In [46312] we attempted to do some phpcs cleanup. This commit cleans up three issues that were introduced in [46309].

Fixes #48142.
Props whyisjake, garrett-eclipse.

#5 @whyisjake
7 weeks ago

In 46314:

General: Further phpcs cleanup.

In [46312] and [46313] we attempted to do some phpcs cleanup. This commit cleans up three issues that were introduced in [46309].

Fixes #48142.

Props: whyisjake, garrett-eclipse, desrosj

#6 @SergeyBiryukov
7 weeks ago

In 46316:

General: Correct strict comparison in WP_Links_List_Table::column_categories() introduced in [46313].

$cat_id is a string, $category is an integer.

See #48142.

#7 @SergeyBiryukov
7 weeks ago

In 46317:

Coding Standards: Remove inline assignment in test_wp_delete_term_should_invalidate_cache().

See #48142.

@garrett-eclipse
7 weeks ago

CS improvement for network.php

#8 @garrett-eclipse
7 weeks ago

  • Keywords has-patch added; commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Thanks for correcting the CS in getTerms.php @SergeyBiryukov
I attached CS-network.php.diff to correct the CS on /multisite/network.php
*No need for 27 spaces ;)

This ticket was mentioned in Slack in #core-coding-standards by garrett-eclipse. View the logs.


7 weeks ago

#10 follow-ups: @jrf
7 weeks ago

@garrett-eclipse pinged me to have a look at the patches here.

Re: [46316] - this may lead to difficult to debug issues unless you are 100% sure both sides of the comparison will always be numeric. Effectively, the change makes no difference to the code and it is still a loose type comparison, as the (int) cast is what PHP would do internally when using !=.

So if a true strict type comparison is intended here, a string-based strict comparison should be used:

<?php
if ( $cat_id !== (string) $category ) {

Also see: https://phpcheatsheets.com/compare/not_equal/

Re: CS-network.php.diff: comments should use proper punctuation, i.e. start with a capital letter and end with a punctuation character, so this inline comment would not pass once the docs checks get activated.

<?php
activate_plugin( $path, '', true );  // $network_wide = true
Last edited 7 weeks ago by jrf (previous) (diff)

@garrett-eclipse
7 weeks ago

Updated network.php patch to address comment convention (Capital first letter, punctuation at end) Thanks @jrf

#11 in reply to: ↑ 10 @garrett-eclipse
7 weeks ago

  • Keywords needs-refresh added

Thanks for the feedback @jrf

Replying to jrf:

@garrett-eclipse pinged me to have a look at the patches here.

Re: [46316] - this may lead to difficult to debug issues unless you are 100% sure both sides of the comparison will always be numeric. Effectively, the change makes no difference to the code and it is still a loose type comparison, as the (int) cast is what PHP would do internally when using !=.

So if a true strict type comparison is intended here, a string-based strict comparison should be used:

<?php
if ( $cat_id !== (string) $category ) {

Also see: https://phpcheatsheets.com/compare/not_equal/

I'm going to leave this comparison check to @SergeyBiryukov for review.

Re: CS-network.php.diff: comments should use proper punctuation, i.e. start with a capital letter and end with a punctuation character, so this inline comment would not pass once the docs checks get activated.

<?php
activate_plugin( $path, '', true );  // $network_wide = true

I appreciate the feedback, as I copied that comment from previous in the file I've gone through and addressed all comments in the file in CS-network.php.2.diff ensuring they started with a capital and ended with punctuation.

#12 @SergeyBiryukov
7 weeks ago

In 46342:

Docs: Improve comments in tests/multisite/network.php per the documentation standards.

Props garrett-eclipse, jrf.
See #48142.

#13 @SergeyBiryukov
7 weeks ago

In 46343:

Coding Standards: Remove inline assignments and extra whitespace in tests/multisite/network.php.

Props garrett-eclipse, jrf.
See #48142.

#14 in reply to: ↑ 10 @SergeyBiryukov
7 weeks ago

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

Replying to jrf:

Re: [46316] - this may lead to difficult to debug issues unless you are 100% sure both sides of the comparison will always be numeric. Effectively, the change makes no difference to the code and it is still a loose type comparison, as the (int) cast is what PHP would do internally when using !=.

So if a true strict type comparison is intended here, a string-based strict comparison should be used:

<?php
if ( $cat_id !== (string) $category ) {

Thanks for the feedback! After some consideration, I'd like to leave that part as is, for a few reasons:

  • [46316] was not meant to introduce the strict comparison, only to fix the issue introduced in [46313]. I've decided to double-check the change in [46313] and confirmed my suspicions, as the types were indeed different, hence the fix in [46316]. In my testing, this worked as expected, and did make a difference to the code.
  • Link Manager was deprecated in core 7 years ago (#21307), so there should not be any changes to data structures there in the forseeable future, and it's safe to assume both sides of the comparison will always be numeric.
  • I don't mind converting this check to (string), however (int) seems more consistent with other similar checks in core:
    • 29 matches for \(int\) [$a-z_]+? (=|!)== or (=|!)== \(int\)
    • 2 matches for \(string\) [$a-z_]+? (=|!)== or (=|!)== \(string\)
Note: See TracTickets for help on using tickets.