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

[hardeepasrani]

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.

[westonruter]

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

[hardeepasrani]

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!

[hardeepasrani]

Initial diff.

[hardeepasrani]

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

[SergeyBiryukov]

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.

[hardeepasrani]

Replying to SergeyBiryukov:

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

[SergeyBiryukov]

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

[hardeepasrani]

Improved diff.

[hardeepasrani]

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. :)

[hardeepasrani]

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

[stevenkword]

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

[joshcanhelp]

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?

[helen]

Reassign for review - @stevenkword, if you could look at this in the next couple of days, that would be great.

[helen]

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.

[desrosj]

This patch needs to be refreshed against trunk.