Unnecessarily passing $this by reference in core

[jdgrimes]

WP_Widget::form_callback() passes $this by reference to an action. This is no longer necessary as of PHP5.

[johnbillion]

A quick grep for array( &$this shows there are still ~200 places in core where we're still using the unnecessary pass-by-reference operator. Should we replace them all in one go?

[jdgrimes]

First pass at removing all unnecessary instances of &$this

[jdgrimes]

That patch is a first attempt at ripping out all instances of &$this. It removes many unnecesary uses of apply_filters_ref_array() and do_action_ref_array() as a result of this as well. There is actually one place that requires passing $this by reference, which is the wp_default_scripts action, called in browser/trunk/wp-includes/class.wp-scripts.php.

This patch currently covers all of the WordPress package, in /src, /tests and /tools. It does not cover other included libraries.

There are several cases where objects other than $this were being passed by reference unnecessarily within the files covered, and I have removed those as well. However, I didn't touch any of the files where $this is not passed by reference, so there may be objects passing by reference unnecessarily in them.

A list of files covered by this patch:

./src/wp-admin/custom-background.php
./src/wp-admin/custom-header.php
./src/wp-admin/includes/class-wp-importer.php
./src/wp-admin/includes/class-wp-list-table.php
./src/wp-admin/includes/class-wp-ms-themes-list-table.php
./src/wp-admin/includes/class-wp-plugins-list-table.php
./src/wp-admin/includes/class-wp-upgrader.php
./src/wp-admin/includes/deprecated.php
./src/wp-admin/includes/list-table.php
./src/wp-includes/class-wp.php
./src/wp-includes/class.wp-scripts.php
./src/wp-includes/class.wp-styles.php
./src/wp-includes/comment.php
./src/wp-includes/plugin.php
./src/wp-includes/query.php
./src/wp-includes/rewrite.php
./src/wp-includes/user.php
./src/wp-includes/widgets.php
./tests/includes/utils.php
./tests/includes/wp-profiler.php
./tests/tests/admin/includesTheme.php
./tests/tests/filters.php
./tests/tests/meta.php
./tests/tests/theme/themeDir.php
./tests/tests/theme/WPTheme.php
./tests/tests/user/capabilities.php
./tools/i18n/makepot.php
./tools/i18n/not-gettexted.php
./tools/i18n/pot-ext-meta.php
./tools/i18n/t/NotGettextedTest.php

Files which contain &$this, but which aren't included (yet):

./src/wp-content/themes/twentyeleven/inc/widgets.php
./src/wp-includes/atomlib.php
./src/wp-includes/class-simplepie.php
./src/wp-includes/ID3/module.audio-video.asf.php
./src/wp-includes/ID3/module.audio-video.flv.php
./src/wp-includes/ID3/module.audio-video.matroska.php
./src/wp-includes/ID3/module.audio-video.quicktime.php
./src/wp-includes/ID3/module.audio-video.riff.php
./src/wp-includes/ID3/module.audio.ac3.php
./src/wp-includes/ID3/module.audio.dts.php
./src/wp-includes/ID3/module.audio.flac.php
./src/wp-includes/ID3/module.audio.mp3.php
./src/wp-includes/ID3/module.audio.ogg.php
./src/wp-includes/ID3/module.tag.apetag.php
./src/wp-includes/ID3/module.tag.id3v1.php
./src/wp-includes/ID3/module.tag.id3v2.php
./src/wp-includes/ID3/module.tag.lyrics3.php
./src/wp-includes/pomo/mo.php
./src/wp-includes/Text/Diff/Renderer/inline.php
./src/wp-includes/Text/Diff.php
./tests/data/plugins/wordpress-importer/parsers.php
./tests/data/plugins/wordpress-importer/wordpress-importer.php

This patch needs testing to make sure nothing is broken. I just ran the whole tests suite, and there seems to be some issues with all of the tests run by tests/tests/query/conditionals.php. I don't know if this is related to this patch (I failed to run the tests before applying it).

On a side note, while doing this I noticed that the tests for apply_filters_ref_array() may need to be revised - they are passing an object by reference.

[jdgrimes]

On another side note, I ran the tests suite in debug mode for the first time - I got ~80 errors. A few notices about undefined properties or variables, but by far mostly notices of using deprecated functions. If those are expected, shouldn't we be silencing them with @ or something? I know its a separate topic - should I open a ticket for it, or is there one already? Or is this behavior intended?

[wonderboymusic]

if you get 80 errors, you broke something. Most of this is okay at a glance, but src/wp-includes/class.wp-scripts.php wasn't removed properly

[SergeyBiryukov]

Replying to jdgrimes:

It removes many unnecesary uses of apply_filters_ref_array() and do_action_ref_array() as a result of this as well.

Previously: #16661

[jdgrimes]

Cleaning last patch.

[jdgrimes]

Replying to wonderboymusic:

if you get 80 errors, you broke something. Most of this is okay at a glance, but src/wp-includes/class.wp-scripts.php wasn't removed properly

I think that one needs to stay, but it should have remained do_action_ref_array() - that's fixed in the latest patch. I got these two errors without it.

14) Tests_Dependencies_jQuery::test_location_of_jquery
Parameter 1 to wp_default_scripts() expected to be a reference, value given

/svn/wordpress/trunk/src/wp-includes/plugin.php:406
/svn/wordpress/trunk/src/wp-includes/class.wp-scripts.php:39
/svn/wordpress/trunk/src/wp-includes/class.wp-scripts.php:34
/svn/wordpress/trunk/tests/tests/dependencies/jquery.php:10

15) Tests_Dependencies_jQuery::test_dont_allow_deregister_core_scripts_in_admin
Parameter 1 to wp_default_scripts() expected to be a reference, value given

/svn/wordpress/trunk/src/wp-includes/plugin.php:406
/svn/wordpress/trunk/src/wp-includes/class.wp-scripts.php:39
/svn/wordpress/trunk/src/wp-includes/class.wp-scripts.php:34
/svn/wordpress/trunk/src/wp-includes/functions.wp-scripts.php:111
/svn/wordpress/trunk/tests/tests/dependencies/jquery.php:55

As for the errors, I don't believe that many of them were caused by this patch directly - e.g.:

31) Tests_Image_Size::test_shrink_dimensions_boundary
wp_shrink_dimensions is <strong>deprecated</strong> since version 3.0! Use wp_constrain_dimensions() instead.

[wonderboymusic]

To make this patch as committable as possible, I would break it into 2 patches:

1. removing the instances of &$this when present in a callable - meaning array( &$this, 'foo' )

2. Everything else - the *_ref_array() stuff - which may need more testing

[johnbillion]

Replying to wonderboymusic:

To make this patch as committable as possible, I would break it into 2 patches

+1

[jdgrimes]

First pass at split patch - this one covers *_ref_array() calls.

[jdgrimes]

First pass at split patch - this one covers array( &$this, 'method' ) calls.

[jdgrimes]

Replying to SergeyBiryukov:

Replying to jdgrimes:

It removes many unnecesary uses of apply_filters_ref_array() and do_action_ref_array() as a result of this as well.

Previously: #16661

I guess that means we probably won't be able to remove those calls. Darn. Although, the result only manifests itself as a PHP warning, so its not like we'd actually be breaking anything, right?

[jdgrimes]

Replying to jdgrimes:

On another side note, I ran the tests suite in debug mode for the first time - I got ~80 errors. A few notices about undefined properties or variables, but by far mostly notices of using deprecated functions. If those are expected, shouldn't we be silencing them with @ or something? I know its a separate topic - should I open a ticket for it, or is there one already? Or is this behavior intended?

I'd like to ask another question about this. If you run the following command:

$ phpunit ./tests/dependencies/jquery

With WP_DEBUG on, you'll get, e.g:

There was 1 error:

1) Tests_Dependencies_jQuery::test_dont_allow_deregister_core_scripts_in_admin
wp_deregister_script was called <strong>incorrectly</strong>. Do not deregister the <code>jquery</code> script in the administration area. To target the frontend theme, use the <code>wp_enqueue_scripts</code> hook. Please see <a href="http://codex.wordpress.org/Debugging_in_WordPress">Debugging in WordPress</a> for more information. (This message was added in version 3.6.)

/svn/wordpress/trunk/src/wp-includes/functions.php:3016
/svn/wordpress/trunk/src/wp-includes/functions.wp-scripts.php:129
/svn/wordpress/trunk/tests/tests/dependencies/jquery.php:55

This is the expected and correct result of that test.

So, are we not supposed to run tests in debug mode as a norm, or do we want it to give these errors loudly when we do, rather than @suppressing them so we can see any real errors? I was just assuming that, since developing in debug mode is correct, testing with it on was expected too, and I was expecting silencing of expected errors.

I'm sure there's a better place to ask this question, just point me in the right direction...

[nacin]

That test is specifically trying to trigger that notice. So the proper thing to do would be to deliberately suppress the notice, but also catch it and confirm it would have occurred. We have similar stuff in tests/functions/deprecated.php for deprecated methods, but we don't really have a solid foundation yet for these.

[jdgrimes]

Updating callbles patch to revision 25209.

[jdgrimes]

As these patches span many files, they will probably take a lot of maintenance. So if this portion of the patch is going to be committed, it should be sooner rather than later (this patch affects only array( &$this, 'method' ) usage). What is the next step to getting this committed? What other files should this cover (see my comment above)?

[wonderboymusic]

In 25254:

Remove lingering instances of call time pass-by-reference, limited to instances of callable - use $this instead of &$this.

Props jdgrimes.
See #25160.

[WraithKenny]

Adding "needs-codex" to signify that any examples on the Codex can also be updated.

[jdgrimes]

The second part of this patch covers do_*_ref_array() calls. The problem with my proposed patch for that is that functions hooking into these actions/filters may be explicitly expecting references to be passed to them, not values. There is even at least one instance of this in core itself. It would be no problem, except that this results in a PHP warning - which unlike notices won't be silenced automatically. So as much as I would like to see these converted to regular do_action() and apply_filters() because they are unnecessary and confusing, we probably can't just rip them out. We'd have to deprecate something.

Ideally, we would catch the errors and then give a deprecated notice when WP_DEBUG is on. Not deprecating the hooks themselves, just the passing of the argument(s) by reference.

Is that worth it? Any thoughts?

[wonderboymusic]

We can look at part 2 in 3.8

[wonderboymusic]

Will look at this more closely after 3.8 drops

[nacin]

Touching ref_array calls is less than appetizing. Still necessary? Might be worth spinning off into a new tracking ticket.

[wonderboymusic]

In 25160.diff​, I think these are all safe, but will sit on it for now.

do_action_ref_array( 'action', array( &$this ) );

becomes do_action( 'action', $this );

Only touches actions that pass one arg: $this

[jdgrimes]

Replying to wonderboymusic:

In 25160.diff​, I think these are all safe, but will sit on it for now.

do_action_ref_array( 'action', array( &$this ) );

becomes do_action( 'action', $this );

Only touches actions that pass one arg: $this

I really wish we could do this, but I don't think the patch is safe as is. It will cause warnings to be thrown. There is detail as to why this is in #16661 as well as above.

There is even at least one example of this in core itself. The wp_default_scripts() function, which is hooked to the 'wp_default_scripts' action is an example.

Test code:

call_user_func_array( 'wp_default_scripts', array( $GLOBALS["wp_scripts"] ) );

This will result in:

PHP Warning:  Parameter 1 to wp_default_scripts() expected to be a reference, value given in...

Interestingly, this doesn't give an error:

$func = 'wp_default_scripts';
$func( $GLOBALS["wp_scripts"] );

The warning only appears when using call_user_func_array() (at least on PHP 5.6.3). This may provide an out, but to take advantage of it we would have to modify do_action() and apply_filters() not to use call_user_func_array(). Thoughts?

[wonderboymusic]

You are correct - I hate PHP:

class Test_Me {
    function __construct() {
        do_action( 'woo', $this );
    }
}

function __woo( &$ref ) {

}

add_action( 'woo', '__woo' );

new Test_Me();
exit();

Produces:
PHP Warning: Parameter 1 to __woo() expected to be a reference, value given

We aren't gonna touch those, marking as fixed.