WordPress.org

Make WordPress Core

#23089 closed enhancement (fixed)

Rewrite a confusing condition in default-widgets.php

Reported by: alexvorn2 Owned by: SergeyBiryukov
Milestone: 3.6 Priority: lowest
Severity: minor Version:
Component: Widgets Keywords: has-patch
Focuses: Cc:

Description

default-widgets.php file:

line 679:

 if ( empty( $instance['number'] ) || ! $number = absint( $instance['number'] ) )
 $number = 5;

shouldn't be ?:

 if ( empty( $instance['number'] ) || ! $number == absint( $instance['number'] ) )
 $number = 5;


Attachments (3)

23089.patch (1.7 KB) - added by SergeyBiryukov 16 months ago.
23089.2.patch (2.8 KB) - added by SergeyBiryukov 15 months ago.
23089.3.patch (3.1 KB) - added by SergeyBiryukov 14 months ago.

Download all attachments as: .zip

Change History (20)

comment:1 helen16 months ago

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

No. It's checking the result of absint() while assigning it to the variable for use later if not 0. Assignment in an if can be purposeful.

comment:2 scribu16 months ago

It can also be confusing, especially when negating the result.

comment:3 follow-up: scribu16 months ago

A more readable version would be:

$number = false;

if ( !empty( $instance['number'] ) )
  $number = absint( $instance['number'] );

if ( !$number )
  $number = 5;
Last edited 16 months ago by scribu (previous) (diff)

comment:4 in reply to: ↑ 3 alexvorn216 months ago

Replying to scribu:

A more readable version would be:

$number = false;

if ( !empty( $instance['number'] ) )
  $number = absint( $instance['number'] );

if ( !$number )
  $number = 5;

agree! +1

comment:5 scribu16 months ago

  • Milestone set to Awaiting Review
  • Priority changed from normal to lowest
  • Resolution invalid deleted
  • Status changed from closed to reopened

SergeyBiryukov16 months ago

comment:6 follow-up: SergeyBiryukov16 months ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 3.6
  • Severity changed from normal to minor
  • Summary changed from WP_Widget_Recent_Comments bug to Rewrite a confusing condition in default-widgets.php

comment:7 SergeyBiryukov16 months ago

  • Type changed from defect (bug) to enhancement

comment:8 in reply to: ↑ 6 ; follow-up: azaozz16 months ago

Replying to scribu:

A more readable version...

+1. I think we had something about not doing assignments inside of if ( ... ) in the coding standards, not seeing it now.

Replying to SergeyBiryukov:

Yeah, that is far better:

$number = isset( $instance['number'] ) ? absint( $instance['number'] ) : 5; 

Perhaps the isset() has to be !empty() so it doesn't allow $number to be 0 (zero).

comment:9 MikeHansenMe16 months ago

+1 to readable code

SergeyBiryukov15 months ago

comment:10 in reply to: ↑ 8 SergeyBiryukov15 months ago

Replying to azaozz:

Perhaps the isset() has to be !empty() so it doesn't allow $number to be 0 (zero).

Good point, 23089.2.patch uses !empty().

comment:11 follow-up: kovshenin15 months ago

  • Cc kovshenin added

I rarely use the ternary operator, but when I do, I always put brackets around the condition, so that it's more obvious, easier to read and modify, just like an if statement.

$number = ( ! empty( $instance['number'] ) ) ? absint( $instance['number'] ) : 10;

comment:12 in reply to: ↑ 11 SergeyBiryukov15 months ago

Replying to kovshenin:

I rarely use the ternary operator, but when I do, I always put brackets around the condition

Yes, I prefer that form too. However, the form without the brackets appears to be prevailing throughout the core.

comment:13 SergeyBiryukov14 months ago

  • Keywords commit added

comment:14 follow-up: nacin14 months ago

The problem with something like ! empty( $instance['number'] ) ? absint( $instance['number'] ) : 10 is if 'number' is malformed, it gets cast to 0, and that 0 never becomes a 5.

Something like this seems like it would be the most straightforward:

$title = ! empty( $instance['title'] ) ? $instance['title'] : __( 'Recent Posts' );
$title = apply_filters( 'widget_title', $title, $instance, $this->id_base );
$number = ! empty( $instance['number'] ) ? absint( $instance['number'] ) : 10;
if ( ! $number )
    $number = 10;

I also don't think this is too hard to read:

if ( empty( $instance['number'] ) || ! $number = absint( $instance['number'] ) )
   $number = 10;

Which happens to be exactly what we have now.

comment:15 nacin14 months ago

  • Keywords commit removed

SergeyBiryukov14 months ago

comment:16 in reply to: ↑ 14 SergeyBiryukov14 months ago

23089.3.patch simplifies the conditions without changing the functionality.

Also contains a bit of cleanup for WP_Widget_Recent_Comments::form() for consistency with WP_Widget_Recent_Posts::form() cleanup done in [21935].

comment:17 SergeyBiryukov14 months ago

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

In 23597:

Simplify logic in WP_Widget_Recent_Posts and WP_Widget_Recent_Comments. fixes #23089.

Note: See TracTickets for help on using tickets.