WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years ago

#16661 closed enhancement (wontfix)

Review use of apply_filters_ref_array() and do_action_ref_array()

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

Description

do_action_ref_array()'s raison d'etre was PHP4's buggy handling of object references.

Since in PHP5 objects are passed by reference by default, it's not needed anymore.

The same goes for apply_filters_ref_array(), introduced in 3.0.

Attachments (12)

16661.diff (5.3 KB) - added by scribu 3 years ago.
16661.2.diff (27.4 KB) - added by kawauso 3 years ago.
16661.3.diff (28.2 KB) - added by kawauso 3 years ago.
No references on objects, no passing by reference for wp_default_scripts() and wp_default_styles()
16661.4.diff (28.2 KB) - added by kawauso 3 years ago.
Keep reference for 'http_api_curl'
16661.5.diff (28.8 KB) - added by kawauso 3 years ago.
Typo
16661.6.diff (28.8 KB) - added by kawauso 3 years ago.
Typo again. Urgh.
16661.7.diff (28.8 KB) - added by kawauso 3 years ago.
Missed a &$this
16661.patch (10.3 KB) - added by hakre 3 years ago.
review of apply_filters_ref_array() calls
16661.2.patch (8.8 KB) - added by hakre 3 years ago.
review of do_action_ref_array() calls
test-ref-array.php (301 bytes) - added by scribu 3 years ago.
test-ref-array.2.php (2.0 KB) - added by hakre 3 years ago.
more elaborated test-suite
16661.3.patch (18.7 KB) - added by hakre 3 years ago.
Merged the two .patch files about both functions

Download all attachments as: .zip

Change History (32)

scribu3 years ago

comment:1 scribu3 years ago

  • Keywords php5 added

comment:2 kawauso3 years ago

Rough patch to remove core usage of both functions shortly.

Getting these notices though:

Warning: Parameter 1 to wp_default_scripts() expected to be a reference, value given in \wp-includes\plugin.php on line 342

Warning: Parameter 1 to wp_default_styles() expected to be a reference, value given in \wp-includes\plugin.php on line 342

kawauso3 years ago

comment:3 scribu3 years ago

Yes, those functions can just remove the reference, as the parameter they're expecting is an object - the exact thing I mentioned in the description.

comment:4 scribu3 years ago

Also, in your patch, you can replace &$this with $this, &$user with $user etc.

kawauso3 years ago

No references on objects, no passing by reference for wp_default_scripts() and wp_default_styles()

comment:5 kawauso3 years ago

Roger that. Not overly familiar with references, so it seemed worth doing a pass without removing them first.

Last patch should remove all the references on objects, but leave strings and integers untouched.

kawauso3 years ago

Keep reference for 'http_api_curl'

kawauso3 years ago

Typo

kawauso3 years ago

Typo again. Urgh.

kawauso3 years ago

Missed a &$this

comment:6 hakre3 years ago

I think it's okay to review the use of apply_filters_ref_array() and do_action_ref_array() (Related: #16767), but please keep in mind that Call-time pass-by-reference has been deprecated since some time now. PHP 5.3 does warnings.

So as this was suggested in one of the patches (I pick this only as an example):

do_action( 'check_passwords', $user->user_login, &$pass1, &$pass2 );

Will trigger warnings which needs to be prevented.

do_action() or apply_filters() are making use of func_get_args() which does not support php references (see manual) which like the other php func_*() functions will always contain a copy of the argument anyway - even if call-time passed by reference as in the example above (!).

This means that the wordpress *_ref_array() functions are indeed still usefull because they pass all parameters in form of an array that will contain the same references even if you create a copy of the array. Therefore those two functions apply_filters_ref_array() and do_action_ref_array() should not be deprecated for the moment.

This would make this ticket invalid, however the usage of those two functions can still benefit from a review as how this ticket reported as well, we do not need to pass objects as references any longer. So part of the intention of this ticket looks still valid to me and I suggest to not close it as invalid but to adopt it a bit.

hakre3 years ago

review of apply_filters_ref_array() calls

hakre3 years ago

review of do_action_ref_array() calls

comment:7 hakre3 years ago

I've attached two patches. The first one is a review of the apply_filters_ref_array() calls, the second one of the do_action_ref_array() calls. Both functions are not deprecated as they are still in use and needed even after review (e.g. do_action_ref_array() ca. 7 times).

comment:8 hakre3 years ago

  • Summary changed from Deprecate do_action_ref_array() to Review use of apply_filters_ref_array() and do_action_ref_array()
  • Type changed from defect (bug) to enhancement

comment:9 scribu3 years ago

This means that the wordpress *_ref_array() functions are indeed still usefull because they pass all parameters in form of an array that will contain the same references even if you create a copy of the array.

test-ref-array.php demonstrates that do_action_ref_array() passes the arguments by value, just like do_action().

I'm guessing that's not what you meant, so please provide an example that illustrates the benefit of do_action_ref_array().

scribu3 years ago

comment:10 follow-up: scribu3 years ago

Nevermind, I forgot to pass by reference. Point taken.

hakre3 years ago

more elaborated test-suite

comment:11 in reply to: ↑ 10 hakre3 years ago

Replying to scribu:

Nevermind, I forgot to pass by reference. Point taken.

It was not obvious for me on first sight either, it's always better to look together over this stuff.

Thanks for creating some test-script, I've extended it into a suite of multiple tests and could run it against different PHP versions (as mu-plugin here). I've stripped the warnings for a better read. That's on an unpatched trunk and it's interesting to see that it differs between PHP 5.2 and 5.3:

PHP Version: 5.2.14 (allow_call_time_pass_reference: 0)

 #01: callback_std(), native invokation, standard parameter                = "original"
 #02: callback_std(), native invokation, call-time reference parameter     = "modified"
 #03: callback_std(), do_action(), standard parameter                      = "original"
 #04: callback_std(), do_action(), call-time reference parameter           = "original"
 #05: callback_std(), do_action_ref_array(), standard parameter            = "original"
 #06: callback_std(), do_action_ref_array(), call-time reference parameter = "modified"
 #07: callback_ref(), native invokation, standard parameter                = "modified"
 #08: callback_ref(), native invokation, call-time reference parameter     = "modified"
 #09: callback_ref(), do_action(), standard parameter                      = "original"
 #10: callback_ref(), do_action(), call-time reference parameter           = "original"
 #11: callback_ref(), do_action_ref_array(), standard parameter            = "original"
 #12: callback_ref(), do_action_ref_array(), call-time reference parameter = "modified"


PHP Version: 5.3.5 (allow_call_time_pass_reference: 0)

 #01: callback_std(), native invokation, standard parameter                = "original"
 #02: callback_std(), native invokation, call-time reference parameter     = "modified"
 #03: callback_std(), do_action(), standard parameter                      = "original"
 #04: callback_std(), do_action(), call-time reference parameter           = "original"
 #05: callback_std(), do_action_ref_array(), standard parameter            = "original"
 #06: callback_std(), do_action_ref_array(), call-time reference parameter = "modified"
 #07: callback_ref(), native invokation, standard parameter                = "modified"
 #08: callback_ref(), native invokation, call-time reference parameter     = "modified"
 #09: callback_ref(), do_action(), standard parameter                      = "original" -- !! hook function not invoked. !!
 #10: callback_ref(), do_action(), call-time reference parameter           = "original" -- !! hook function not invoked. !!
 #11: callback_ref(), do_action_ref_array(), standard parameter            = "original" -- !! hook function not invoked. !!
 #12: callback_ref(), do_action_ref_array(), call-time reference parameter = "modified"

The interesting part is that call_user_func_array() will issue a warning in PHP 5.3.5 if the hook callback expects the parameter to be passed by reference and it isn't. That will prevent the callback from being invoked even. So this influences callbacks with parameters by reference invoked by do_action() and do_action_ref_array().

Those callbacks are getting invoked in PHP 5.2.14.

Update: PHP Version: 5.3.5 (allow_call_time_pass_reference: 1) does not make a difference to that behavior.

Last edited 3 years ago by hakre (previous) (diff)

hakre3 years ago

Merged the two .patch files about both functions

comment:12 scribu3 years ago

  • Keywords 3.2-early removed
  • Milestone changed from Future Release to 3.2

comment:13 hakre3 years ago

  • Keywords reporter-feedback added

Based on the last patch provided, are there more places to look into?

comment:14 scribu3 years ago

  • Keywords reporter-feedback removed
  • Owner set to scribu
  • Status changed from new to reviewing

With 16661.3.patch, I get:

Hunk #1 FAILED at 442.
1 out of 1 hunk FAILED -- saving rejects to file wp-includes/meta.php.rej

Otherwise, looks good.

comment:15 jane3 years ago

  • Milestone changed from 3.2 to Future Release

Didn't get in by freeze.

comment:16 hakre3 years ago

I think it's worth to consider this patch before it gets much more stale. Otherwise it wil be much harder to re-create.

comment:17 hakre3 years ago

  • Keywords dev-feedback added

comment:18 hakre3 years ago

Related: #17443

comment:19 dd323 years ago

I think it's worth to consider this patch before it gets much more stale.

Unfortunately that's not going to happen since it's past freeze and it was said rather early we didn't want to make lots of changes (other than straight removals of code blocks). We can re-consider in 3.3+

comment:20 scribu2 years ago

  • Keywords dev-feedback removed
  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from reviewing to closed

Seems this change can actually produce warnings with some plugins:

http://core.trac.wordpress.org/ticket/18536#comment:70

Best to leave it alone and just avoid introducing new calls to these functions from now on.

Note: See TracTickets for help on using tickets.