WordPress.org

Make WordPress Core

Opened 11 months ago

Last modified 2 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: 5.3 Priority: normal
Severity: normal Version:
Component: Script Loader Keywords: has-patch
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 (1)

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

Download all attachments as: .zip

Change History (14)

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

  • Milestone changed from 4.9.9 to 5.0

#3 @pento
10 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
8 months ago

  • Milestone changed from 5.0.1 to 5.0.2

#5 @SergeyBiryukov
8 months ago

  • Milestone changed from 5.0.2 to 5.1

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

  • Milestone changed from 5.1 to 5.2

#9 @jqz
7 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
5 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
2 weeks 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
2 weeks ago

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

#12 @killerbishop
2 weeks 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
2 weeks ago

  • Keywords has-patch added; needs-patch removed
Note: See TracTickets for help on using tickets.