WordPress.org

Make WordPress Core

Opened 22 months ago

Closed 6 months ago

Last modified 6 months ago

#25160 closed defect (bug) (fixed)

Unnecessarily passing $this by reference in core

Reported by: jdgrimes Owned by:
Milestone: 3.7 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch 3.9-early
Focuses: performance Cc:

Description

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

Attachments (7)

25160.patch (639 bytes) - added by jdgrimes 22 months ago.
25160.2.patch (47.5 KB) - added by jdgrimes 22 months ago.
First pass at removing all unnecessary instances of &$this
25160.3.patch (46.2 KB) - added by jdgrimes 22 months ago.
Cleaning last patch.
25160-ref-array.1.patch (13.7 KB) - added by jdgrimes 22 months ago.
First pass at split patch - this one covers *_ref_array() calls.
25160-callables.1.patch (32.4 KB) - added by jdgrimes 22 months ago.
First pass at split patch - this one covers array( &$this, 'method' ) calls.
25160-callables.2.patch (32.6 KB) - added by jdgrimes 22 months ago.
Updating callbles patch to revision 25209.
25160.diff (6.4 KB) - added by wonderboymusic 6 months ago.

Download all attachments as: .zip

Change History (32)

@jdgrimes22 months ago

comment:1 @johnbillion22 months ago

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?

@jdgrimes22 months ago

First pass at removing all unnecessary instances of &$this

comment:2 follow-up: @jdgrimes22 months ago

  • Component changed from Widgets to General
  • Keywords needs-testing added
  • Summary changed from Unnecessarily passing object by reference in WP_Widget::form_callback() to Unnecessarily passing $this by reference in core

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.

comment:3 follow-up: @jdgrimes22 months ago

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?

comment:4 follow-up: @wonderboymusic22 months ago

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

comment:5 in reply to: ↑ 2 ; follow-up: @SergeyBiryukov22 months ago

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

@jdgrimes22 months ago

Cleaning last patch.

comment:6 in reply to: ↑ 4 @jdgrimes22 months ago

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.

comment:7 follow-up: @wonderboymusic22 months ago

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' )
  1. Everything else - the *_ref_array() stuff - which may need more testing

comment:8 in reply to: ↑ 7 @johnbillion22 months ago

Replying to wonderboymusic:

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

+1

@jdgrimes22 months ago

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

@jdgrimes22 months ago

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

comment:9 in reply to: ↑ 5 @jdgrimes22 months ago

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?

comment:10 in reply to: ↑ 3 @jdgrimes22 months ago

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

comment:11 @nacin22 months ago

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.

comment:12 @toscho22 months ago

  • Cc info@… added

@jdgrimes22 months ago

Updating callbles patch to revision 25209.

comment:13 @jdgrimes22 months ago

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

comment:14 @SergeyBiryukov22 months ago

  • Milestone changed from Awaiting Review to 3.7
  • Severity changed from trivial to normal

comment:15 @wonderboymusic22 months ago

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.

comment:16 @WraithKenny22 months ago

  • Cc Ken@… added
  • Keywords needs-codex added

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

comment:17 @jdgrimes22 months ago

  • Keywords dev-feedback added

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?

comment:18 @wonderboymusic21 months ago

  • Milestone changed from 3.7 to 3.8

We can look at part 2 in 3.8

comment:19 @wonderboymusic19 months ago

  • Keywords 3.9-early added; dev-feedback removed
  • Milestone changed from 3.8 to Future Release

Will look at this more closely after 3.8 drops

comment:20 @nacin18 months ago

  • Component changed from General to Performance

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

comment:21 @nacin17 months ago

  • Component changed from Performance to General
  • Focuses performance added

@wonderboymusic6 months ago

comment:22 follow-ups: @wonderboymusic6 months ago

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

comment:23 in reply to: ↑ 22 @jdgrimes6 months ago

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?

comment:24 in reply to: ↑ 22 @wonderboymusic6 months ago

  • Keywords needs-testing needs-codex removed
  • Resolution set to fixed
  • Status changed from new to closed

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.

Last edited 6 months ago by wonderboymusic (previous) (diff)

comment:25 @wonderboymusic6 months ago

  • Milestone changed from Future Release to 3.7
Note: See TracTickets for help on using tickets.