![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png){kind=logo}
#23089 closed enhancement (fixed)
Rewrite a confusing condition in default-widgets.php
| Reported by: | alexvorn2 | Owned by: | SergeyBiryukov |
|---|---|---|---|
| Priority: | lowest | Milestone: | 3.6 |
| Component: | Widgets | Version: | |
| Severity: | minor | Keywords: | has-patch |
| Cc: | Focuses: |
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)
Change History (21)
#3
follow-up:
↓ 4
@
14 years ago
A more readable version would be:
$number = false; if ( !empty( $instance['number'] ) ) $number = absint( $instance['number'] ); if ( !$number ) $number = 5;
#4
in reply to: ↑ 3
@
14 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
@
14 years ago
- Milestone → Awaiting Review
- Priority normal → lowest
- Resolution invalid
- Status closed → reopened
#6
follow-up:
↓ 8
@
14 years ago
- Keywords has-patch added
- Milestone Awaiting Review → 3.6
- Severity normal → minor
- Summary WP_Widget_Recent_Comments bug → Rewrite a confusing condition in default-widgets.php
23089.patch uses the pattern we already have in a couple of other places:
http://core.trac.wordpress.org/browser/tags/3.5/wp-includes/default-widgets.php#L612
#8
in reply to: ↑ 6
;
follow-up:
↓ 10
@
14 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).
#10
in reply to: ↑ 8
@
13 years ago
Replying to azaozz:
Perhaps the
isset()has to be!empty()so it doesn't allow$numberto be 0 (zero).
Good point, 23089.2.patch uses !empty().
#11
follow-up:
↓ 12
@
13 years ago
- Cc 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
@
13 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.
#14
follow-up:
↓ 16
@
13 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.
#16
in reply to: ↑ 14
@
13 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].
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.