WordPress.org

Make WordPress Core

Opened 15 months ago

Closed 7 months ago

#44162 closed defect (bug) (wontfix)

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

Reported by: thrijith Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch
Focuses: administration Cc:

Description (last modified by SergeyBiryukov)

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

See #43583.

Attachments (1)

44162.diff (1.3 KB) - added by thrijith 15 months ago.

Download all attachments as: .zip

Change History (11)

@thrijith
15 months ago

#1 @desrosj
15 months ago

  • Description modified (diff)
  • Milestone changed from Awaiting Review to 4.9.7

#2 follow-up: @thrijith
15 months ago

I am having a doubt regarding this, how to replace where count's value is used directly?
for eg

if ( is_array( $done ) ) {
 $done['updated'] = count( $done['updated'] );
 $done['skipped'] = count( $done['skipped'] );
 $done['locked']  = count( $done['locked'] );
 $sendback        = add_query_arg( $done, $sendback );
}

#3 in reply to: ↑ 2 @birgire
15 months ago

Replying to thrijith:

I am having a doubt regarding this, how to replace where count's value is used directly?
for eg

if ( is_array( $done ) ) {
 $done['updated'] = count( $done['updated'] );
 $done['skipped'] = count( $done['skipped'] );
 $done['locked']  = count( $done['locked'] );
 $sendback        = add_query_arg( $done, $sendback );
}

Instead of this verbose way:

if ( is_array( $done ) ) {
    $done['updated'] = is_countable( $done['updated'] ) ? count( $done['updated'] ) : 0;
    $done['skipped'] = is_countable( $done['skipped'] ) ? count( $done['skipped'] ) : 0;
    $done['locked']  = is_countable( $done['locked'] ) ? count( $done['locked'] ) : 0;
    $sendback        = add_query_arg( $done, $sendback );
}

this comes to mind:

if ( is_array( $done ) ) {
    foreach( array( 'update', 'skipped', 'locked' ) as $key ) {
        $done[ $key ] = is_countable( $done[ $key ] ) ? count( $done[ $key ] ) : 0;
    }
    $sendback = add_query_arg( $done, $sendback );
}

but I've not tested this.

Personally I might even check if $done[$key] is set and use e.g. the approach of adjusting an initial counting array array( 'update' => 0, 'skipped' => 0, 'locked' => 0 ), but I don't think we should make such changes in the logic here, just keep it as simple as possible.

#4 @thrijith
15 months ago

  • Keywords has-patch added

#5 @thrijith
15 months ago

but I don't think we should make such changes in the logic here, just keep it as simple as possible.

Agreed.

Also, I am thinking of updating this from

if ( post_type_supports( $post_type, 'page-attributes' ) || 
( is_countable( get_page_templates( $post ) ) && count( get_page_templates( $post ) ) > 0 ) )

to

$page_templates = get_page_templates( $post );
if ( post_type_supports( $post_type, 'page-attributes' ) || 
( is_countable( $page_templates ) && count( $page_templates ) > 0 ) )

Will that be ok?

#6 @SergeyBiryukov
15 months ago

  • Description modified (diff)

#7 @SergeyBiryukov
15 months ago

  • Component changed from General to Editor
  • Focuses administration added

#8 @desrosj
15 months ago

  • Milestone changed from 4.9.7 to 4.9.8

Moving all tickets in 4.9.7 to 4.9.8.

#9 @desrosj
15 months ago

  • Component changed from Editor to Administration
  • Keywords close added
  • Milestone changed from 4.9.8 to Awaiting Review

Prefacing my feedback by reiterating the is_countable() function should only be used before counting a value where it's valid for either a countable or non-countable value to be passed. See 44123#comment:11 for a longer explanation.

I don't think that these occurrences of count() should be preceded by an is_countable() check. All three functions (get_users(), wp_get_post_revisions(), and get_page_templates()) should always return arrays. If arrays are not returned, this is a bug and the PHP warning should not be suppressed.

Also, moving this to Administration because it relates to the edit screen and not directly with the Editor.

Last edited 15 months ago by desrosj (previous) (diff)

#10 @pento
7 months ago

  • Keywords close removed
  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed
  • Version trunk deleted

The functions that populate these variables will always return a countable value, so they don't need to be checked.

Note: See TracTickets for help on using tickets.