Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#44979 closed defect (bug) (fixed)

Parameter 1 to wp_default_styles() expected to be a reference, value given

Reported by: jqz's profile jqz Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.4 Priority: normal
Severity: normal Version:
Component: Script Loader Keywords: has-patch needs-testing dev-feedback
Focuses: Cc:

Description

This is a follow-up to #37772.

The bug still manifests if the UOPZ extension is loaded.

(It is a PHP warning, so functionality is okay, but log files will get cluttered and performance hit as server writes to log file on each page request.)

Environment:

Fresh install of WordPress 4.9.8.

It can be fixed by removing the & from the function parameter declaration for wp_default_styles() and wp_default_scripts() - the & is not required as the parameter is an object and thus the original object is referenced in any case.

Attachments (2)

44979.diff (4.3 KB) - added by killerbishop 5 years ago.
Patch for 44979 - Switch from pass-by-ref on wp_default_styles() and wp_default_scripts()
44979.2.diff (1.4 KB) - added by jorbin 4 years ago.

Download all attachments as: .zip

Change History (31)

#1 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to 4.9.9
  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

Hi @jqz, welcome to WordPress Trac!

Thanks for the ticket, I was able to reproduce the issue with the UOPZ extension installed.

#2 @pento
5 years ago

  • Milestone changed from 4.9.9 to 5.0

#3 @pento
5 years ago

  • Milestone changed from 5.0 to 5.0.1
  • Version 4.9.8 deleted

I originally milestoned this for 5.0 as a possible PHP 7.3 compatibility issue. As it's to do with the UOPZ extension, I'm moving this to 5.0.1 for further evaluation.

I suspect the fix is going to be trickier than just removing the & and changing WP_Scripts::init() to call do_action() instead, given historical conversations around the behaviour of this code.

In particular, any fix will need to be tested with PHP 5.2 and 5.3, which had some weird object/reference behaviour that the handful of remaining do_action_ref_array() usages seem to exist to work around.

#4 @pento
5 years ago

  • Milestone changed from 5.0.1 to 5.0.2

#5 @SergeyBiryukov
5 years ago

  • Milestone changed from 5.0.2 to 5.1

#6 @aaronholbrook
5 years ago

Just wanted to chime in and say this is affecting every site we have on WP Engine that is attempting to run PHP 7 and above (7.1, 7.2) and is definitely clogging up our error logging system.

Any way to not kick this down the road would be greatly appreciated as we've been blocked from upgrading PHP for the time being.

#7 @pento
5 years ago

  • Keywords needs-patch added

This ticket needs a patch and significant testing to ensure changing it doesn't break everything.

Alternatively, writing a patch that works for PHP 5.6+, and waiting until WordPress 5.2 may make things easier, as we'll be dropping PHP 5.2-5.5 support.

#8 @pento
5 years ago

  • Milestone changed from 5.1 to 5.2

#9 @jqz
5 years ago

Alternatively, writing a patch that works for PHP 5.6+, and waiting until WordPress 5.2 may make things easier, as we'll be dropping PHP 5.2-5.5 support.

Why not drop (the problematic) PHP 5.2-5.3 support in WordPress 5.1? Is anyone really using the latest version of WordPress with such old versions of PHP?

Waiting till WordPress 5.2 will presumably mean waiting the best part of a year.

#10 @jorbin
5 years ago

  • Milestone changed from 5.2 to 5.3

Bumping this from 5.2 due to lack of progress.

As 5.2 drops support for php versions below 5.6, the old PHP versions is no longer a concern.

#11 @killerbishop
5 years ago

I've been working on making a patch for this issue and testing against PHP 5.6. Changing the reference requires changing other downstream functions that are called by it such as wp_default_packages(). I noticed in the wp-admin/load-scripts.php that the functions are called outside the class and pass a new instance directly. Is do_action() really needed here or can it just be replaced with a direct call like wp_default_scripts($this)?

Lastly, is there any issue with adding typehints like:

function wp_default_scripts( WP_Scripts $scripts )

I see very few uses of it grepping around, but the contributor's handbook makes no reference either way on this. Historically, I can see it being not used for compatibility with older PHP versions and perhaps is something being discussed still.

@killerbishop
5 years ago

Patch for 44979 - Switch from pass-by-ref on wp_default_styles() and wp_default_scripts()

#12 @killerbishop
5 years ago

I've added the patch with do_action() left intact. I did have to switch the noop.php script to disable do_action() instead of do_action_ref_array() since I changed to that. I tested to make sure scripts/styles are still loading as expected in prodction and debug mode. Here is my cloned repo's PR if needed:

https://github.com/killerbishop/wordpress-develop/pull/3

#13 @killerbishop
5 years ago

  • Keywords has-patch added; needs-patch removed

This ticket was mentioned in Slack in #core by marybaum. View the logs.


4 years ago

#16 @marybaum
4 years ago

  • Keywords needs-testing dev-feedback added
  • Milestone changed from 5.3 to Future Release

#17 @Mte90
4 years ago

In VVV we saw that this happen with the Tideways extension enabled.
So maybe because is code to support ancient PHP version with some extension create issues like with UOPZ.
In my testing this happen with PHP 7.2 and I can confirm that the patch works!
I am sad that now to get this fix we have to wait for WordPress 5.4...

This ticket was mentioned in Slack in #core by mte90. View the logs.


4 years ago

#19 @TJNowell
4 years ago

Can we get a clear Todo list on what needs to be done and what needs to be tested to get this resolved? This is becoming a significant support burden, I'd like to spend the energy fixing the issue rather than supporting the issue if I have a choice, but it's unclear what the blocker is from reading the ticket

@jorbin
4 years ago

#20 @jorbin
4 years ago

I've updated the patch so that 1) It doesn't include the type hints which aren't necessary for this and 2) continues to noop do_action_ref_array to prevent unnecessary side affects. I would like some additional people to test this and confirm it works.

#21 @SergeyBiryukov
4 years ago

  • Milestone changed from Future Release to 5.4

#22 @pcarvalho
4 years ago

latest patch 44979.2.diff doesn't quite work for me. Still get the warnings. It needs all the script-loader.php hunks. ( not sure if all, but applying first patch solves my warnings )

Last edited 4 years ago by pcarvalho (previous) (diff)

This ticket was mentioned in Slack in #meta by sergey. View the logs.


4 years ago

#24 @valentinbora
4 years ago

I've encountered these while having the tideways_xhprof extension installed and active (PHP 7.2)

#25 @Mte90
4 years ago

I can confirm that 44979.2.diff doesn't fix the problem with php 7.2

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


4 years ago

#27 @SergeyBiryukov
4 years ago

In my testing, the issue happens with:

  • PHP 7.2.x, php_tideways_xhprof extension (tested versions 5.0.1 and 5.0.2).
  • PHP 7.2.x, php_uopz extension (version 5.0.2).

Does not happen with:

  • PHP 7.2.x and later, php_uopz extension (version 5.1.0+).
  • PHP 7.3.x and later, php_tideways_xhprof extension.

Replacing do_action_ref_array() with do_action() was previously explored in #16661 and #25160. It might actually break things and cause the same warning if plugins hooked to wp_default_scripts still use a reference in the function signature, for example:

function my_scripts( &$scripts ) {
	...
}
add_action( 'wp_default_scripts', 'my_scripts' );

As noted in comment:23:ticket:25160, the warning is caused by call_user_func_array() usage in WP_Hook::apply_filters(). As a bit more complete illustration from comment:24:ticket:25160, this snippet:

include 'wp-load.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.

In my testing, do_action_ref_array() is not the issue here (there are ~90 instances of do_action_ref_array() and apply_filters_ref_array() in core that don't cause the warning).

The real issue is the reference in these function signatures:

function wp_register_tinymce_scripts( &$scripts, $force_uncompressed = false )
function wp_default_packages_vendor( &$scripts )
function wp_get_script_polyfill( &$scripts, $tests )
function wp_default_packages_scripts( &$scripts )
function wp_default_packages_inline_scripts( &$scripts )
function wp_default_packages( &$scripts )
function wp_default_scripts( &$scripts )
function wp_default_styles( &$styles )

Most of these were just copied from wp_default_scripts(). Removing the references appears to resolve the warnings without affecting the functionality.

#28 @SergeyBiryukov
4 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 47355:

Script Loader: Remove unnecessary reference sign from function definitions in script loader.

This resolves PHP warnings when extensions like php_uopz or php_tideways_xhprof are in use.

Object variables in PHP 5+ contain a reference to the object, and it's the reference that's passed around.

Props jqz, killerbishop, Mte90, TJNowell, jorbin, pento, pcarvalho, valentinbora, SergeyBiryukov.
Fixes #44979.

This ticket was mentioned in Slack in #core-editor by ocean90. View the logs.


4 years ago

Note: See TracTickets for help on using tickets.