Make WordPress Core

Opened 9 years ago

Closed 8 years ago

#33440 closed enhancement (fixed)

Deprecated PHP4 style constructor warnings make debugging hard

Reported by: mcguive7's profile McGuive7 Owned by: ocean90's profile ocean90
Milestone: 4.5 Priority: normal
Severity: normal Version: 4.3
Component: Widgets Keywords: 2nd-opinion dev-feedback has-patch
Focuses: Cc:

Description

At present, the PHP 4 style constructor deprecation warning messages read as follows for widgets:

The called constructor method for WP_Widget is deprecated

This indicates that a widget is causing the issue, however gives no indication as to which widget is using the PHP4 style constructor. What might be more helpful is indicate the specific widget that is throwing the notice, like so:

The called constructor method for {Widget class that is extending WP_Widget} is deprecated

This can be easily accomplished by subbing out the static text used in the _deprecated_constructor() call (currently "WP_Widget"), for the actual name of the widget class that is extending WP_Widget.

A quick search for _deprecated_constructor across all files reveals that it is only used in one other instance, in the WP_Widget_Factory class. This class is likely far less extended, however it can't hurt to make the same change here as well.

Attachments (3)

33440.diff (551 bytes) - added by McGuive7 9 years ago.
Switch out static text in _deprecated_constructor() calls to dynamically generated class name of actual widget generating warning
33440.2.diff (587 bytes) - added by wonderboymusic 9 years ago.
35470.patch (3.6 KB) - added by sebastian.pisula 8 years ago.

Download all attachments as: .zip

Change History (15)

@McGuive7
9 years ago

Switch out static text in _deprecated_constructor() calls to dynamically generated class name of actual widget generating warning

#1 @McGuive7
9 years ago

  • Keywords has-patch 2nd-opinion dev-feedback added

Patch is in for widgets. Question: are there other non-widget implementations of PHP4 deprecation that need looking at as well?

#2 @dd32
9 years ago

Arguably switching out WP_Widget for get_class( $this ) is incorrect, as the constructor for the current class isn't deprecated at all, it's that that class is calling parent::WP_Widget() instead.

That being said, if we can include the actual widget in question here somehow that'd be great for debugging.. but it's also worth noting that for all other cases of deprecated notices, we don't/can't detect what is causing the deprecated notice in a performant manner.

The Log Deprecated Notices plugin hasn't been updated, but one of it's features is to do a back-trace to locate what plugin/class/function/method/arguement the notice is actually refering to.

#3 @McGuive7
9 years ago

What about something like this:

public function WP_Widget( $id_base, $name, $widget_options = array(), $control_options = array() ) {
	$class = 'WP_Widget';
	if ( $class != get_class( $this ) ) {
		$class .= ' (' . get_class( $this ) . ')';
	}

	_deprecated_constructor( $class, '4.3.0' );
	WP_Widget::__construct( $id_base, $name, $widget_options, $control_options );
}

We could even prepend some text so the whole thing looks like "... WP_Widget (extended by {widget_class)", however I figure it might be better to keep it simple and avoid creating new translations (no idea if that's a big deal or not).

Thoughts?

#4 @jorbin
9 years ago

I've submitted a patch via slack to @nacin for Log Deprecated Notices to include deprecated constructor support.

Are we able to guarantee that WP_Widget will never be called staticly and thus without $this being set? If get_class() is called with anything other than an object, an E_WARNING level error is raised.

New translatable strings are fine for 4.4. I'm sure there will be plenty of them.

#6 @McGuive7
9 years ago

@jorbin: good question. No way to guarantee that WP_Widget() won't be called statically, but it seems like this same issue would arise for the vast majority of the WP_Widget class's methods, most of which also include calls to $this. So can you think of a reason why WP_Widget() might be particularly likely to be called statically?

A quick search across all core files for the following strings doesn't yield any results that would indicate this method is getting called statically in core:
::WP_Widget
WP_Widget

That said, I could easily imagine a scenario in which a plugin might call the method statically for any number of reasons, well-coded or not :)

Not to overcomplicate things, but it'd be easy enough to detect if the call was made statically, and only proceed when that's not the case with something like:
isset( $this ) && get_class($this) == __CLASS__
(http://stackoverflow.com/questions/1858538/how-do-i-check-in-php-that-im-in-a-static-context-or-not)

Seems like possible overkill for what may be an unlikely scenario, but does this seem worth doing to others? Any other considerations I'm missing?

#7 @jorbin
9 years ago

  • Keywords needs-patch added; has-patch removed

It's unlikely, but I'd rather be defensive, especially since this is for enhancing a deprecation warning that will have been around a release cycle.

I would support an updated version of attachment:33440.2.diff that checked if $this is set and if so, used get_class( $this ) instead of WP_Widget.

Any other enhancement ideas for the string?

Last edited 9 years ago by jorbin (previous) (diff)

#8 @McGuive7
9 years ago

Great, happy to implement this. The patch is ready to go, I'd just like to optimize the message to be as clear as possible. Anyone feel strongly about one of the following options, or another - assuming Example_Widget is the name of the child class?

  1. ... method for WP_Widget (Example_Widget)
  2. ... method for WP_Widget (called by Example_Widget)
  3. ... method for WP_Widget (called via Example_Widget)

I'm inclined to go with option 2.

#9 @ocean90
8 years ago

#35470 was marked as a duplicate.

#10 @sebastian.pisula
8 years ago

I added my patch for 4.5-alpha-36312 :)

Last edited 8 years ago by sebastian.pisula (previous) (diff)

#11 @johnbillion
8 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Awaiting Review to 4.5

#12 @ocean90
8 years ago

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

In 36541:

Introduce a $parent_class parameter for _deprecated_constructor().

Use the parameter for the deprecated constructor warning in WP_Widget to provide an indication to which widget is using the PHP4 style constructor.

Props sebastian.pisula.
Fixes #33440.

Note: See TracTickets for help on using tickets.