WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 9 months ago

#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 3 years ago.
23089.2.patch (2.8 KB) - added by SergeyBiryukov 2 years ago.
23089.3.patch (3.1 KB) - added by SergeyBiryukov 2 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 @helen3 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.

comment:2 @scribu3 years ago

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

comment:3 follow-up: @scribu3 years ago

A more readable version would be:

$number = false;

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

if ( !$number )
  $number = 5;
Version 0, edited 3 years ago by scribu (next)

comment:4 in reply to: ↑ 3 @alexvorn23 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

comment:5 @scribu3 years ago

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

@SergeyBiryukov3 years ago

comment:6 follow-up: @SergeyBiryukov3 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

comment:7 @SergeyBiryukov3 years ago

  • Type changed from defect (bug) to enhancement

comment:8 in reply to: ↑ 6 ; follow-up: @azaozz3 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).

comment:9 @MikeHansenMe2 years ago

+1 to readable code

@SergeyBiryukov2 years ago

comment:10 in reply to: ↑ 8 @SergeyBiryukov2 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().

comment:11 follow-up: @kovshenin2 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;

comment:12 in reply to: ↑ 11 @SergeyBiryukov2 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.

comment:13 @SergeyBiryukov2 years ago

  • Keywords commit added

comment:14 follow-up: @nacin2 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.

comment:15 @nacin2 years ago

  • Keywords commit removed

@SergeyBiryukov2 years ago

comment:16 in reply to: ↑ 14 @SergeyBiryukov2 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].

comment:17 @SergeyBiryukov2 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.

comment:18 @ircbot9 months ago

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

Note: See TracTickets for help on using tickets.