Opened 8 years ago
Last modified 3 months ago
#38133 reviewing defect (bug)
Core widget fields fail to render value of "0" when empty() checks are used
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | normal | Version: | 2.6 |
Component: | Widgets | Keywords: | has-patch early |
Focuses: | administration | Cc: |
Description (last modified by )
If you put a single zero 0 the instance property for in many default widgets, it won't output any thing if an empty()
check is used since empty( '0' ) === true
. You can try putting a 0 in any widget title or content. It works if you put 00 or anything else.
The reason being our use of empty( $variable )
and ! empty( $variable )
in the default widgets. empty
returns FALSE if var exists and has a non-empty, non-zero value.
So it will be better to use isset( $variable) && $variable != ''
instead. Wanted to submit a patch but wanted to know if above method will be the best one.
Attachments (3)
Change History (22)
#1
follow-up:
↓ 2
@
8 years ago
- Description modified (diff)
- Keywords needs-patch added
- Milestone changed from Awaiting Review to Future Release
- Summary changed from Widget output empty if input is zero to Default widget title used if instance title is "0"
- Version changed from trunk to 2.6
#2
in reply to:
↑ 1
@
8 years ago
Replying to westonruter:
@hardeepasrani good catch. Yes, the
isset( $variable) && $variable !== ''
check would be better over using! empty( $variable )
. (Note!==
instead of!=
.)
It's not just title. It could also be content in Text widget. Anyway, I'll send a patch after doing some tests. Thanks!
#3
@
8 years ago
- Description modified (diff)
- Summary changed from Default widget title used if instance title is "0" to Core widget fields fail to render value of "0" when empty() checks are used
#4
@
8 years ago
@westonruter Tried to be consistent and tested various inputs & scenarios to make sure nothing breaks. Hope it will do it. :)
#5
follow-up:
↓ 6
@
8 years ago
widgets.diff is a great start.
- While we're at it, it would be great to replace cryptic lines like this:
/** This filter is documented in wp-includes/widgets/class-wp-widget-pages.php */ $title = apply_filters( 'widget_title', isset( $instance['title'] ) && $instance['title'] !== '' ? $instance['title'] : '', $instance, $this->id_base );
with this:if ( isset( $instance['title'] ) && '' !== $instance['title'] ) { $title = $instance['title']; } else { $title = ''; } /** This filter is documented in wp-includes/widgets/class-wp-widget-pages.php */ $title = apply_filters( 'widget_title', $title, $instance, $this->id_base );
for clarity. See #23089. isset($instance['title'])
should beisset( $instance['title'] )
, see Brace Style.$instance['title'] !== ''
should be'' !== $instance['title']
, see Yoda Conditions.- It probably makes sense to check if
$instance['title']
is set (just in case, although it should always be set anyway), butisset( $title )
would be redundant, as it's always set just a few lines above.
#6
in reply to:
↑ 5
@
8 years ago
Replying to SergeyBiryukov:
I'll submit an update. And brace style is inconsistent throughout the widget files.
#8
@
8 years ago
In widgets2.diff, improved as suggested by @SergeyBiryukov, even some bracket styles not used by the diff. Left isset point because there's no harm in checking again. :)
#10
@
8 years ago
- Keywords has-patch added; needs-patch removed
- Milestone changed from Future Release to 4.7
- Owner set to SergeyBiryukov
- Status changed from new to reviewing
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
8 years ago
This ticket was mentioned in Slack in #core by stevenkword. View the logs.
8 years ago
#14
@
8 years ago
If it's helpful ... I can confirm the original issue and that the patch fixes it.
I also agree with @SergeyBiryukov that if we're changing this, it should match the code standards. I created a new patch using WP coding standards, removing unnecessary isset() checks, and setting the $title var consistently. I also changed the $instancefilter? check in class-wp-widget-text.php back to empty() since that's just a yes/no. Some of the spacing changes unrelated to the edits here made this a little tougher to review, FYI.
One thing though ... in the RSS widget, I noticed that the behavior there is a little buggy. If I set a title, it's overridden on output with the feed name, which seems to me like the wrong thing to do. It's pretty clear what's going wrong (L:71-81 in the attached patched version) but feels like it should be in a different ticket. Thoughts?
#15
@
8 years ago
- Owner changed from SergeyBiryukov to stevenkword
Reassign for review - @stevenkword, if you could look at this in the next couple of days, that would be great.
This ticket was mentioned in Slack in #core by helen. View the logs.
8 years ago
#17
@
8 years ago
- Keywords 4.8-early added
- Milestone changed from 4.7 to Future Release
I am actually going to punt this for now - if somebody wants to bring it back for 4.7, you still can, though it's a bit late for somebody to own this and it's quite an old bug at this point. Will mark for 4.8-early, at least.
#18
@
6 years ago
- Keywords early needs-refresh added; 4.8-early removed
This patch needs to be refreshed against trunk
.
This ticket was mentioned in PR #7785 on WordPress/wordpress-develop by @sukhendu2002.
3 months ago
#19
- Keywords needs-refresh removed
Trac ticket: https://core.trac.wordpress.org/ticket/38133
## Description
This PR fixes an issue where core widget fields fail to render when the value is "0". The problem occurs because empty()
checks in widget fields treat "0" as an empty value, causing widgets to not display content when "0" is the only value.
## Problem
Currently, if you set a single "0" as the value in many default widgets (e.g., widget title or content), it won't output anything due to the use of empty()
checks. The issue arises because empty()
returns TRUE
for "0" values, treating them as empty.
For example:
- Setting a widget title to "0" results in no title being displayed
- Multiple zeros ("00") or other values work correctly
- This affects multiple default widgets where
empty()
is used for validation
## Solution
Replace empty()
checks with explicit isset()
and string comparison:
`php
Before
if ( ! empty( $instancetitle? ) ) {
$title = $instancetitle?;
}
After
if ( isset( $instancetitle? ) && !== $instancetitle? ) {
$title = $instancetitle?;
}
@hardeepasrani good catch. Yes, the
isset( $variable) && $variable !== ''
check would be better over using! empty( $variable )
. (Note!==
instead of!=
.)