Context Navigation

← Previous Ticket
Next Ticket →

#44167 closed defect (bug) (invalid)

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

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

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 8 years ago.

Download all attachments as: .zip

Change History (6)

@thrijith
8 years ago

Attachment 44167.diff added

#1 @thrijith
8 years ago

Keywords has-patch added

#2 @subrataemfluence
8 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
8 years ago

Keywords close added
Version trunk

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

Milestone Awaiting Review
Resolution → invalid
Status new → closed

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

#5 @subrataemfluence
8 years ago

Keywords has-patch reporter-feedback removed

Note: See TracTickets for help on using tickets.

Download in other formats:

Make WordPress Core

Context Navigation

#44167 closed defect (bug) (invalid)

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

Description

Attachments (1)

Change History (6)

@thrijith 8 years ago

#1 @thrijith 8 years ago

#2 @subrataemfluence 8 years ago

#3 @johnbillion 8 years ago

#4 @johnbillion 8 years ago

#5 @subrataemfluence 8 years ago