Make WordPress Core

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: hardeepasrani's profile hardeepasrani Owned by: stevenkword's profile stevenkword
Milestone: Future Release Priority: normal
Severity: normal Version: 2.6
Component: Widgets Keywords: has-patch early
Focuses: administration Cc:

Description (last modified by westonruter)

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)

widgets.diff (13.3 KB) - added by hardeepasrani 8 years ago.
Initial diff.
widgets2.diff (15.2 KB) - added by hardeepasrani 8 years ago.
Improved diff.
38133.3.diff (15.3 KB) - added by joshcanhelp 8 years ago.

Download all attachments as: .zip

Change History (22)

#1 follow-up: @westonruter
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

@hardeepasrani good catch. Yes, the isset( $variable) && $variable !== '' check would be better over using ! empty( $variable ). (Note !== instead of !=.)

#2 in reply to: ↑ 1 @hardeepasrani
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 @westonruter
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

@hardeepasrani
8 years ago

Initial diff.

#4 @hardeepasrani
8 years ago

@westonruter Tried to be consistent and tested various inputs & scenarios to make sure nothing breaks. Hope it will do it. :)

Last edited 8 years ago by hardeepasrani (previous) (diff)

#5 follow-up: @SergeyBiryukov
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 be isset( $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), but isset( $title ) would be redundant, as it's always set just a few lines above.

#6 in reply to: ↑ 5 @hardeepasrani
8 years ago

Replying to SergeyBiryukov:

I'll submit an update. And brace style is inconsistent throughout the widget files.

#7 @SergeyBiryukov
8 years ago

True, but let's make it more consistent as we move forward :)

@hardeepasrani
8 years ago

Improved diff.

#8 @hardeepasrani
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. :)

#9 @hardeepasrani
8 years ago

It's almost 5 am so I'll try to sleep now. :p

#10 @SergeyBiryukov
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

#13 @stevenkword
8 years ago

I'm going to take a look at reviewing this one per the Bug Scrub.

#14 @joshcanhelp
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?

@joshcanhelp
8 years ago

#15 @helen
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 @helen
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 @desrosj
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?;

}

Note: See TracTickets for help on using tickets.