WordPress.org

Make WordPress Core

Opened 4 weeks ago

Closed 2 weeks ago

#41743 closed defect (bug) (fixed)

Using the_widget on a widget that has not been registered results in an undefined index notice.

Reported by: SeBsZ Owned by: obenland
Milestone: 4.9 Priority: normal
Severity: normal Version: 2.8
Component: Widgets Keywords: good-first-bug has-patch
Focuses: Cc:

Description

First of all, this may be by design to notify the developer that they've probably made a mistake.

However, I'd like to suggest a small check that prevents this notice.

In Wordpress 4.8.1 wp-includes\widgets.php on line 1039 (or the second line inside the the_widget() function) there is:

$widget_obj = $wp_widget_factory->widgets[$widget];
if ( ! ( $widget_obj instanceof WP_Widget ) ) {
  return;
}

This means that calling the_widget with a $widget name that has not been registered, instantly results in an undefined index notice.

A simple check would solve this:

if ( ! ( isset($wp_widget_factory->widgets[$widget]) ) ) {
  return;
}

$widget_obj = $wp_widget_factory->widgets[$widget];
if ( ! ( $widget_obj instanceof WP_Widget ) ) {
  return;
}

Apologies if this is not a bug but by design.

Attachments (4)

41743.diff (938 bytes) - added by SeBsZ 3 weeks ago.
41743.2.diff (506 bytes) - added by mrasharirfan 3 weeks ago.
Cleaned up the patch according to WP Coding Standards.
41743.3.diff (1.0 KB) - added by mrasharirfan 3 weeks ago.
41743.patch (650 bytes) - added by ramiy 2 weeks ago.
Use informative message in _doing_it_wrong() function

Download all attachments as: .zip

Change History (20)

#1 @obenland
4 weeks ago

  • Keywords needs-patch good-first-bug added
  • Version changed from 4.8.1 to 2.8

Introduced in [11317].

Would you want to write a patch for that?

#2 @SeBsZ
3 weeks ago

Sure. This is my first patch here, let's hope I did it correctly.

@SeBsZ
3 weeks ago

#3 @obenland
3 weeks ago

  • Milestone changed from Awaiting Review to 4.9

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


3 weeks ago

@mrasharirfan
3 weeks ago

Cleaned up the patch according to WP Coding Standards.

#5 @mrasharirfan
3 weeks ago

  • Keywords has-patch added; needs-patch removed

Hi @obenland and @SeBsZ

I have tested the patch. And it is working fine.

Also, I have cleaned up the code according to WP Coding Standards given here: https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/

Cheers!

#6 @SeBsZ
3 weeks ago

Great, glad to have helped.

BTW, you cleaned up my code (added some spaces), however the line of code below mine does not use spaces inside the square brackets yet. Might need to clean that up as well? I actually copied that part for my patch.

@mrasharirfan
3 weeks ago

#7 @mrasharirfan
3 weeks ago

Here you go. Cleaned up the line below as well.

#8 @westonruter
3 weeks ago

To me it seems having a notice/warning is better than failing silently. If you have a theme/plugin that is calling the_widget() then you should know when it fails.

The other option would be to return a WP_Error on failure, and true on success.

#9 @obenland
3 weeks ago

Agreed, I was about to suggest adding a _doing_it_wrong() call here to tell developers that widgets need to be registered first

#10 @SeBsZ
3 weeks ago

I also agree, hence the reason my very first line in this ticket was:

First of all, this may be by design to notify the developer that they've probably made a mistake.

The reason I came across these notifications is my plugin was correctly registering and using, but another plugin (Yoast SEO) uses this call in certain situations:

<?php
remove_all_actions( 'widgets_init' );

Which led to my widgets not being registered when I tried to use them and that's how I first encountered the undefined index notice. I'm only saying this to explain that sometimes this isn't a mistake by a developer per se, but could be because of another plugin's incorrect behavior.

I do agree a _doing_it_wrong() is better than failing silently, so at least a developer will know something is wrong.

#11 @obenland
3 weeks ago

  • Owner set to obenland
  • Resolution set to fixed
  • Status changed from new to closed

In 41327:

Widgets: Add nudge for registered widgets

Informs developers that widgets need to be registered before they can be
displayed through the_widget(). Previously it would fail with an ambiguous
undefined index notice.

Props SeBsZ, mrasharirfan.
Fixes #41743.

#12 @ramiy
3 weeks ago

@obenland,

The text can be more informative. We can explain how to register widgets. It is done in other messages for developers.

Just replace:

_doing_it_wrong( __FUNCTION__, __( 'Widgets need to be registered before they can be displayed.' ), '4.9.0' );

With:

/* translators: %s: register_widget() */
_doing_it_wrong( __FUNCTION__, sprintf( __( 'Widgets need to be registered using %s, before they can be displayed.' ), '<code>register_widget()</code>' ), '4.9.0' );

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


2 weeks ago

#14 @SergeyBiryukov
2 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening to address comment:12.

@ramiy
2 weeks ago

Use informative message in _doing_it_wrong() function

#15 @ramiy
2 weeks ago

@obenland, @SergeyBiryukov

You can use the attached patch and close this ticket.

#16 @obenland
2 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 41340:

Widgets: Improve _doing_it_wrong message.

Adds more context to help sending developers on the path to success.

Props ramiy.
Fixes 41743.

Note: See TracTickets for help on using tickets.