WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

#17111 closed enhancement (wontfix)

Remove unnecessary copy-by-reference in do_action()

Reported by: scribu Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Performance Keywords: has-patch
Focuses: Cc:

Description

There's this weird check in do_action():

	if ( is_array($arg) && 1 == count($arg) && isset($arg[0]) && is_object($arg[0]) ) // array(&$this)
		$args[] =& $arg[0];

It was introduced in [4177], probably to fix PHP4 nastiness.

But, since we're on PHP5 now, the copy-by-reference is not needed anymore.

Attachments (3)

conservative.17111.diff (485 bytes) - added by scribu 4 years ago.
17111.diff (569 bytes) - added by scribu 4 years ago.
test-do-action.php (265 bytes) - added by scribu 4 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 @nacin4 years ago

  • Milestone changed from Awaiting Review to 3.2

@scribu4 years ago

@scribu4 years ago

comment:2 @scribu4 years ago

  • Component changed from General to Performance

It basically transformed:

array( array( &$this ), 'foo' )

into

array( &$this, 'foo' )

for the callback hooked to that action.

conservative.17111.diff simply removes the reference.

17111.diff removes the check entirely. Reasons:

  • not a useful behaviour anymore
  • WP core doesn't use it anywhere; instead, it uses the more flexible call_user_func_array()
  • unlikely that plugins call do_action( 'whatever', array( &$this ) );
  • speed improvement
Version 0, edited 4 years ago by scribu (next)

@scribu4 years ago

comment:3 @Denis-de-Bernardy4 years ago

unlikely that plugins call do_action( 'whatever', array( &$this ) );

If memory serves, that unlikely call used to be an documented best practice a few years back.

comment:4 follow-up: @scribu4 years ago

Link, or it didn't happen. :)

comment:5 @ericmann4 years ago

I've seen do_action( 'whatever', &$this ); but not do_action( 'something', array( &$this ) );:

If a plugin somewhere does pass array( &$this ), how bad would things break?

comment:6 in reply to: ↑ 4 @Denis-de-Bernardy4 years ago

Replying to scribu:

Link, or it didn't happen. :)

Sorry, not in much of a drive to google.com/archive.org the reference. FWIW, though, I vaguely recall seeing as an example of good things to do in the codex somewhere between 2005 and 2006. Plus, in case memory serves me wrong, that was the only means of getting the expected behavior (with php4 objects) back then anyway.

I'm arguably biased in some sense, since my own plugins called things that way back then. (Those versions are completely obsolete, so I don't care much; but pointing out why I'm raising the issue.)

This probably may no longer be a necessity with php5 objects (since that is what the array with a reference was all about)... But the rational behind the original behavior shouldn't be forgotten. And I dare suggest that a few test cases might be needed before things do change.

comment:7 @scribu4 years ago

The reason I'm skeptical is because do_action_ref_array() was also introduced in WP 2.1, when this change was made.

Eric & Denis: for a simple testcase, see test-do-action.php.

comment:8 @jane4 years ago

  • Milestone changed from 3.2 to Future Release

Not in by freeze, punting.

comment:9 @scribu3 years ago

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

Not worth the potential back-compat headaches.

Note: See TracTickets for help on using tickets.