Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#44167 closed defect (bug) (invalid)

Add is_countable() check to wp-admin/edit.php

Reported by: thrijith's profile thrijith Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: General Keywords: close
Focuses: Cc:

Description

Update code with is_countable() wherever necessary in wp-admin/edit-form-advanced.php

See #44123.

Attachments (1)

44167.diff (777 bytes) - added by thrijith 6 years ago.

Download all attachments as: .zip

Change History (6)

@thrijith
6 years ago

#1 @thrijith
6 years ago

  • Keywords has-patch added

#2 @subrataemfluence
6 years ago

  • Keywords reporter-feedback added

Hi, thank you for the patch. However, although it is good to have is_countable() prior to actually count number of elements in an array, in this particular context I don't feel it is super necessary because before counting we are already checking

if ( is_array( $done ) ) { 
 ...
 ...
}

If $done is not an array, the count will never be executed. So, to me adding an extra check is_countable() inside is_array() will cause extra burden.

Here are some use cases (as I see it):

$cars = array("Volvo","BMW","Toyota");
echo is_countable( $cars) ? count($cars) : 0;

if we don't have the check is_array(), is_countable() is an obvious choice.

If we have this:

$cars = ''
echo is_countable( $cars) ? count($cars) : 0;

again, we must use is_countable().

But if we have this:

$cars = '';
if( is_array( $cars ) ) {
  echo count($cars);
}

the above snippet will be enough because since $cars is not an array and we are using count() inside is_array( $cars ){ ... } check already, the code count( $cars ) will never execute.

The bottom line is, to my understanding we do not need to have is_countable() function when using count() inside is_array() check.

#3 @johnbillion
6 years ago

  • Keywords close added
  • Version trunk deleted

Thanks for your patch @thrijith.

I agree with @subrataemfluence. I don't believe this is appropriate use of is_countable(). If a bug is introduced elsewhere which results in $done being set to a non-countable value (for example via the return value of bulk_edit_posts()), it's not a good idea to mask it via logic such as this. This is the reason that PHP now triggers warnings when attempting to count un-countable values since PHP 7.2.

See 44123#comment:11.

#4 @johnbillion
6 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from new to closed

Ah yes, the is_array() guard condition immediately preceding these lines means this is definitely not a necessary change.

#5 @subrataemfluence
6 years ago

  • Keywords has-patch reporter-feedback removed
Note: See TracTickets for help on using tickets.