Make WordPress Core

Opened 11 years ago

Closed 11 years ago

Last modified 9 years ago

#23089 closed enhancement (fixed)

Rewrite a confusing condition in default-widgets.php

Reported by: alexvorn2's profile alexvorn2 Owned by: sergeybiryukov's profile 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 11 years ago.
23089.2.patch (2.8 KB) - added by SergeyBiryukov 11 years ago.
23089.3.patch (3.1 KB) - added by SergeyBiryukov 11 years ago.

Download all attachments as: .zip

Change History (21)

#1 @helen
11 years 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.

#2 @scribu
11 years ago

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

#3 follow-up: @scribu
11 years ago

A more readable version would be:

$number = false;

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

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

#4 in reply to: ↑ 3 @alexvorn2
11 years 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

#5 @scribu
11 years ago

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

#6 follow-up: @SergeyBiryukov
11 years 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

#7 @SergeyBiryukov
11 years ago

  • Type changed from defect (bug) to enhancement

#8 in reply to: ↑ 6 ; follow-up: @azaozz
11 years 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).

#9 @MikeHansenMe
11 years ago

+1 to readable code

#10 in reply to: ↑ 8 @SergeyBiryukov
11 years 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().

#11 follow-up: @kovshenin
11 years 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;

#12 in reply to: ↑ 11 @SergeyBiryukov
11 years 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.

#13 @SergeyBiryukov
11 years ago

  • Keywords commit added

#14 follow-up: @nacin
11 years 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.

#15 @nacin
11 years ago

  • Keywords commit removed

#16 in reply to: ↑ 14 @SergeyBiryukov
11 years 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].

#17 @SergeyBiryukov
11 years 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.

This ticket was mentioned in IRC in #wordpress-dev by SergeyBiryukov. View the logs.


9 years ago

Note: See TracTickets for help on using tickets.