WordPress.org

Make WordPress Core

Opened 15 months ago

Last modified 8 weeks 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: Future Release Priority: normal
Severity: normal Version:
Component: Script Loader Keywords: has-patch needs-testing dev-feedback
Focuses: Cc:
PR Number:

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 (1)

44979.diff (4.3 KB) - added by killerbishop 4 months ago.
Patch for 44979 - Switch from pass-by-ref on wp_default_styles() and wp_default_scripts()

Download all attachments as: .zip

Change History (18)

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

  • Milestone changed from 4.9.9 to 5.0

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

  • Milestone changed from 5.0.1 to 5.0.2

#5 @SergeyBiryukov
12 months ago

  • Milestone changed from 5.0.2 to 5.1

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

  • Milestone changed from 5.1 to 5.2

#9 @jqz
11 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
8 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
4 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.

@killerbishop
4 months ago

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

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

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

#13 @killerbishop
4 months ago

  • Keywords has-patch added; needs-patch removed

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


2 months ago

#16 @marybaum
2 months ago

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

#17 @Mte90
8 weeks 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...

Note: See TracTickets for help on using tickets.