WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 8 months ago

#38133 reviewing defect (bug)

Core widget fields fail to render value of "0" when empty() checks are used

Reported by: hardeepasrani Owned by: stevenkword
Milestone: Future Release Priority: normal
Severity: normal Version: 2.6
Component: Widgets Keywords: has-patch early needs-refresh
Focuses: administration Cc:
PR Number:

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 3 years ago.
Initial diff.
widgets2.diff (15.2 KB) - added by hardeepasrani 3 years ago.
Improved diff.
38133.3.diff (15.3 KB) - added by joshcanhelp 3 years ago.

Download all attachments as: .zip

Change History (21)

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

Initial diff.

#4 @hardeepasrani
3 years ago

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

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

#5 follow-up: @SergeyBiryukov
3 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
3 years ago

Replying to SergeyBiryukov:

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

#7 @SergeyBiryukov
3 years ago

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

@hardeepasrani
3 years ago

Improved diff.

#8 @hardeepasrani
3 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
3 years ago

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

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


3 years ago

This ticket was mentioned in Slack in #core by stevenkword. View the logs.


3 years ago

#13 @stevenkword
3 years ago

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

#14 @joshcanhelp
3 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
3 years ago

#15 @helen
3 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.


3 years ago

#17 @helen
3 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
8 months ago

  • Keywords early needs-refresh added; 4.8-early removed

This patch needs to be refreshed against trunk.

Note: See TracTickets for help on using tickets.