Make WordPress Core

Opened 16 months ago

Last modified 24 hours ago

#44979 accepted defect (bug)

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

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


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


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 6 months 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 5 weeks ago.

Download all attachments as: .zip

Change History (24)

#1 @SergeyBiryukov
16 months 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
16 months ago

  • Milestone changed from 4.9.9 to 5.0

#3 @pento
16 months 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
14 months ago

  • Milestone changed from 5.0.1 to 5.0.2

#5 @SergeyBiryukov
14 months ago

  • Milestone changed from 5.0.2 to 5.1

#6 @aaronholbrook
13 months 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
13 months 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
12 months ago

  • Milestone changed from 5.1 to 5.2

#9 @jqz
12 months 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
10 months 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
6 months 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.

6 months ago

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

#12 @killerbishop
6 months 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:


#13 @killerbishop
6 months ago

  • Keywords has-patch added; needs-patch removed

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

4 months ago

#16 @marybaum
4 months ago

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

#17 @Mte90
3 months 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.

6 weeks ago

#19 @TJNowell
5 weeks 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

5 weeks ago

#20 @jorbin
5 weeks 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
5 weeks ago

  • Milestone changed from Future Release to 5.4

#22 @pcarvalho
24 hours 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 24 hours ago by pcarvalho (previous) (diff)
Note: See TracTickets for help on using tickets.