Review use of apply_filters_ref_array() and do_action_ref_array()

[scribu]

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.

[kawauso]

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

[scribu]

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.

[scribu]

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

[kawauso]

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

[kawauso]

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.

[kawauso]

Keep reference for 'http_api_curl'

[kawauso]

Typo

[kawauso]

Typo again. Urgh.

[kawauso]

Missed a &$this

[hakre]

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.

[hakre]

review of apply_filters_ref_array() calls

[hakre]

review of do_action_ref_array() calls

[hakre]

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).

[scribu]

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().

[scribu]

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

[hakre]

more elaborated test-suite

[hakre]

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.

[hakre]

Merged the two .patch files about both functions

[hakre]

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

[scribu]

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.

[jane]

Didn't get in by freeze.

[hakre]

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

[hakre]

Related: #17443

[dd32]

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+

[scribu]

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.