WordPress.org

Make WordPress Core

Opened 8 months ago

Last modified 3 months ago

#25160 new defect (bug)

Unnecessarily passing $this by reference in core

Reported by: jdgrimes Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch needs-testing needs-codex 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 (6)

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

Download all attachments as: .zip

Change History (27)

jdgrimes8 months ago

comment:1 johnbillion8 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?

jdgrimes8 months ago

First pass at removing all unnecessary instances of &$this

comment:2 follow-up: jdgrimes8 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: jdgrimes8 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: wonderboymusic8 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: SergeyBiryukov8 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

jdgrimes8 months ago

Cleaning last patch.

comment:6 in reply to: ↑ 4 jdgrimes8 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: wonderboymusic8 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 johnbillion8 months ago

Replying to wonderboymusic:

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

+1

jdgrimes8 months ago

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

jdgrimes8 months ago

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

comment:9 in reply to: ↑ 5 jdgrimes8 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 jdgrimes8 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 nacin8 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 toscho8 months ago

  • Cc info@… added

jdgrimes8 months ago

Updating callbles patch to revision 25209.

comment:13 jdgrimes8 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 SergeyBiryukov8 months ago

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

comment:15 wonderboymusic8 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 WraithKenny7 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 jdgrimes7 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 wonderboymusic7 months ago

  • Milestone changed from 3.7 to 3.8

We can look at part 2 in 3.8

comment:19 wonderboymusic5 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 nacin3 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 nacin3 months ago

  • Component changed from Performance to General
  • Focuses performance added
Note: See TracTickets for help on using tickets.