Make WordPress Core

Opened 15 years ago

Closed 14 years ago

#10264 closed enhancement (wontfix)

use instanceof instead of is_a()

Reported by: mittineague's profile Mittineague Owned by:
Milestone: Priority: low
Severity: normal Version: 2.8
Component: Performance Keywords: needs-patch 2nd-opinion
Focuses: Cc:

Description

My Error Reporting plugin broke with 2.8 in part because of an extreme number of errors (mostly simplepie "non-static called staticly" errors).

But there is also a problem with errors in Core files i.e. is_a()

True they are only E_STRICT deprecated errors and is_a() is un-deprecated in PHP 5.3.0 and it may be more trouble than it's worth to handle the differences for PHP < 5.0, PHP 5.0, PHP 5.1+

I have PHP 5.2.6 so I didn't need the "fatal error" work-around, but I made changes similar to

//	if ( !is_a($wp_scripts, 'WP_Scripts') )
	$wp_scripts_string = 'WP_Scripts';
	if ( !($wp_scripts instanceof $wp_scripts_string) )

in the following wp-includes files

wp-includes/class-feed.php [1 fix]
wp-includes/class-IXR.php [5 fixes]
wp-includes/classes.php [1 fix]
wp-includes/functions.wp-scripts.php [6 fixes]
wp-includes/functions.wp-styles.php [5 fixes]
wp-includes/general-template.php [1 fix]
wp-includes/script-loader.php [4 fixes]

Of particular note is the classes.php file which literally throws hundreds of errors per page on my blog.

function is_wp_error($thing) {
//	if ( is_object($thing) && is_a($thing, 'WP_Error') )
	$wp_error_string = 'WP_Error';
	if ( is_object($thing) && ($thing instanceof $wp_error_string) )

And I may be off, but the functions.wp-styles.php file has a function that looks suspicious

function wp_style_is( $handle, $list = 'queue' ) {
.....
//	if ( !is_a($wp_styles, 'WP_Scripts') )
	$wp_scripts_string = 'WP_Scripts';
	if ( !($wp_styles instanceof $wp_scripts_string) )

i.e. if $wp_styles is not a WP_Scripts it becomes a WP_Styles

Change History (13)

#1 @Denis-de-Bernardy
15 years ago

  • Component changed from Performance to Warnings/Notices
  • Keywords reporter-feedback added
  • Milestone changed from Unassigned to 2.9
  • Owner Denis-de-Bernardy deleted

is_a() has been re-introduced in php. so not fixing that part.

are you messing around with the error reporting? WP should normally silence E_STRICT.

#2 @Mittineague
15 years ago

Thanks for the quick response.

Yes I am "messing" with error reporting. In particular, my Error Reporting plugin which allows for highly customizable (by type/folder) treatment of errors (logging and/or email notification).

I understand the uniqueness of is_a() in that it is needed < PHP 5.0, works in subsequent versions (deprecated, but still works), and is back again in 5.3

My main concerns are in particular it's use in the classes.php is_wp_error function, which is called hundreds of times per page load. My testing shows that is_a is 3 times slower than instanceof (amounts to maybe 1/10 sec. per page load) so it's more a performance thing than an error thing, although in can bloat logs if "ignore_repeated" is false.

I am also wondering about the functions.wp-style.php file's wp_style_is function. i.e.

function wp_style_is( $handle, $list = 'queue' ) {
	global $wp_styles;
	if ( !is_a($wp_styles, 'WP_Scripts') )
		$wp_styles = new WP_Styles();

	$query = $wp_styles->query( $handle, $list );

	if ( is_object( $query ) )
		return true;

	return $query;
}

I may be missing something, but should WP_Scripts be WP_Styles instead?

#3 @Denis-de-Bernardy
15 years ago

  • Component changed from Warnings/Notices to Permalinks
  • Keywords needs-patch added; reporter-feedback removed
  • Milestone changed from 2.9 to Future Release
  • Owner set to ryan
  • Summary changed from wp-includes E_STRICT errors to use instanceof instead of is_a()
  • Type changed from defect (bug) to enhancement

Opened #10283 for the second check.

For the switch to instanceof, it'll need to wait until php5.

#4 @Denis-de-Bernardy
15 years ago

  • Component changed from Permalinks to Performance
  • Owner changed from ryan to Denis-de-Bernardy

#5 @Denis-de-Bernardy
15 years ago

  • Owner Denis-de-Bernardy deleted
  • Status changed from new to assigned

#6 @Mittineague
15 years ago

I think I may have thought of a better solution then testing for support of the instanceof operator that will still be faster than is_a and not throw errors of any type.

eg.

//	if ( !is_a($wp_scripts, 'WP_Scripts') )
	if ( !( get_class($wp_scripts) == 'WP_Scripts' ) )

get_class is even a little faster than instanceof and was introduced in PHP version 4

#7 @Denis-de-Bernardy
15 years ago

for that particular one, is_object() might be a good enough shortcut, too.

get_class() won't deal with inheritance. I'm not entirely sure how much we want to deal with that.

#8 @Mittineague
15 years ago

Inheritance is a good point. To the best of my knowledge, The 7 files

wp-includes/class-feed.php		'SimplePie'
wp-includes/class-IXR.php		'IXR_Date' 'IXR_Base64' 'IXR_Error'
wp-includes/classes.php			'WP_Error'
wp-includes/functions.wp-scripts.php	'WP_Scripts'
wp-includes/functions.wp-styles.php	'WP_Styles'
wp-includes/general-template.php	'WP_Styles'
wp-includes/script-loader.php		'WP_Scripts' 'WP_Styles'

do not use is_a to test for classes that currently have subclasses in the WordPress Core. Of course this does not mean that they never will, nor that some themes and plugins won't break if inheritance is not taken into account.

Using get_class with is_subclass_of replicates the functionality of is_a

	if ( !is_a($wp_scripts, 'WP_Scripts') ) {
	if ( !( (get_class($wp_scripts) == 'WP_Scripts') || (is_subclass_of($wp_scripts, 'WP_Scripts')) ) ) {

When $wp_scripts is the class, it maintains the performance improvement over is_a, but when $wp_scripts is a subclass, is_sublass_of diminishes the improvement (30% and 80% respectively). Therefore, when a subclass, it's slower than instanceof, but still a bit faster than is_a

As the above code in most cases would only be a very slight improvement (one ten-thousandth of a sec.) at the expense of code readability, perhaps the only file where this should be considered as a replacement is the classes.php file where is_a runs many times per page. This would save about a hundreth to one five-hundreth of a second per page depending on whether or not $thing is a WP_Error or a subclass of WP_Error

*Note, in my comment of last week regarding the classes.php file's is_wp_error function which runs hundreds of times per page, I said 1/10'th sec. per page. This was wrong, it should read 1/100'th sec. per page.

#9 @Denis-de-Bernardy
15 years ago

  • Keywords 2nd-opinion added

Do we want to go through this for 1/100th of a second per page... It's probably useful for wp.com.

Let's get a core dev's feedback...

#10 @peaceablewhale
15 years ago

  • Cc peaceable_whale@… added

#11 follow-up: @Mittineague
15 years ago

I must admit this turns out to be more complex than I first imagined. There is PHP version support. The instanceof operator requires PHP 5+, but using version_compare() slows things down considerably. Both instanceof and is_subclass_of() have the potential for autoload induced fatal errors and neccessitate a work-around (easy enough I guess, but just the same). Using a work-around, get_class() and is_subclass_of() works in PHP 4,5 and is a bit faster than is_a() but makes the conditional less readable.

So it comes down to a question of a slight improvement in performance vs. code readability. And all this for an E_STRICT error that in ways isn't even a "real" error.

The decision is yours of course, but I think if it was up to me I'd probably wait until WordPress no longer supports PHP 4 and then use the instanceof operator with the work-around.

Thanks for your time and indulgence.

#12 in reply to: ↑ 11 @Denis-de-Bernardy
15 years ago

Replying to Mittineague:

The decision is yours of course, but I think if it was up to me I'd probably wait until WordPress no longer supports PHP 4 and then use the instanceof operator with the work-around.

Seems like the right thing to do, yeah. :D

#13 @nacin
14 years ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from assigned to closed

Going to wontfix for now.

When the time for PHP5 comes, I'm sure we'll take a systematic approach to performance improvements, which would include instanceof use.

Note: See TracTickets for help on using tickets.