Make WordPress Core

Opened 7 years ago

Closed 6 years ago

#44168 closed defect (bug) (wontfix)

Add is_countable() check to wp-admin/includes/ajax-actions.php

Reported by: thrijith's profile thrijith Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch 2nd-opinion
Focuses: administration Cc:

Description

Update code with is_countable() wherever necessary in wp-admin/includes/ajax-actions.php

See #44123.

Attachments (1)

44168.diff (1.5 KB) - added by thrijith 7 years ago.

Download all attachments as: .zip

Change History (6)

@thrijith
7 years ago

#1 @thrijith
7 years ago

  • Keywords has-patch added

#2 @subrataemfluence
7 years ago

  • Keywords reporter-feedback added

Thanks for the ticket and patch you have proposed.

Code snippet 1:

<?php
if ( is_taxonomy_hierarchical( $taxonomy ) ) {
   $level = count( get_ancestors( $tag->term_id, $taxonomy, 'taxonomy' ) );
   $ancestors = get_ancestors( $tag->term_id, $taxonomy, 'taxonomy' );
   $level     = is_countable( $ancestors ) ? count( $ancestors ) : 0;
   ...
}

I don't think is_countable() is necessary here since the return value of is_taxonomy_hierarchical() is either true or false. If it is true, and since get_ancestors() always returns an array(), we can safely use count() on get_ancestors(). Otherwise, this block will not execute.

Code snippet 2:

<?php
$status['count'] = count( $wp_list_table->items );
$status['count'] = is_countable( $wp_list_table->items ) ? count( $wp_list_table->items ) : 0;

Hence, $wp_list_table->items always returns an array, I think use of is_countable() will create over burden.

Here is the definition of $items in class-wp-list-table.php

<?php
/**
* The current list of items.
*
* @since 3.1.0
* @var array
*/
public $items;

Code snippet 3:

<?php
if ( is_countable( $exporters ) && 0 < count( $exporters ) ) { ... }

This is how we get $exporters. The value is already set as array().

<?php
$exporters = apply_filters( 'wp_privacy_personal_data_exporters', array() );

so, I believe we can safely use count() directly on $exporters.

Code snippet 4:

<?php
$erasers = apply_filters( 'wp_privacy_personal_data_erasers', array() );

Same as above.

Please let me know your views on this.

#3 @desrosj
7 years ago

  • Focuses administration added
  • Keywords close added; reporter-feedback removed
  • Version trunk deleted

Thanks for your work on this, @thrijith!

I do not think that is_countable() is needed for any of these instances of count(), though. is_countable() 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).

  • While it is possible for the get_ancestors() function to return an uncountable value if a plugin or theme incorrectly uses the get_ancestors filter, this would be a bug.
  • $wp_list_table->items could also not be countable. But this also indicates a bug in the list table class being used.
  • The third example is already preceded by a ! is_array() check and would never be reached if the $exporters variable is not countable.
  • If a plugin or theme is incorrectly using the wp_privacy_personal_data_erasers filter, this is a bug and the warning should not be suppressed.
Last edited 7 years ago by desrosj (previous) (diff)

#4 @desrosj
6 years ago

  • Keywords 2nd-opinion added

Marking for a second opinion.

#5 @SergeyBiryukov
6 years ago

  • Component changed from General to Administration
  • Keywords close removed
  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

I agree with comment:3. Thanks for the patch though!

Note: See TracTickets for help on using tickets.