stream icon indicating copy to clipboard operation
stream copied to clipboard

[PHP 8] Error when updating widgets.

Open itsTheFae opened this issue 2 years ago • 5 comments

Bug Report

While Stream is active on PHP 8 host, adding or removing widgets in WordPress causes an error to be thrown. The named arguments added to PHP 8 create an issue with the call chain found at: /stream/connectors/class-connector-widgets.php lines 300, 301, as well as 360 and 361

Expected Behavior

The expected behavior is simply saving the widgets without delay or error messages.

Actual Behavior

When saving, update may take a moment and then report there was an error via pop-up at the bottom of the screen, which you can easily glance over if you're scrolling about.
If you have logging enabled, you will find the following message:
PHP Fatal error: Uncaught ArgumentCountError: array_merge() does not accept unknown named parameters in /var/www/dcr/wp-content/plugins/stream/connectors/class-connector-widgets.php:300

Steps to Reproduce the Problem

  1. Use PHP 8 with latest WordPress 5.8.1
  2. Install latest Stream plugin
  3. Create a couple widgets and then delete one to trigger the functions.
  4. Check your error logging for details.

System Information

  • Stream plugin version: 3.8.1
  • WordPress version: 5.8.1
  • PHP version: 8.0.11

Possible Solution

If you're using PHP 8, you can patch the error-prone lines by adding the parameter names to call_user_func_array so that lines 300, 301, as well as 360 and 361 look like this:

$all_old_widget_ids = array_unique( call_user_func_array( callback:'array_merge', args:$old ) );
$all_new_widget_ids = array_unique( call_user_func_array( callback:'array_merge', args:$new ) );

The issue with this patch is that it clearly is not syntactically compatible with any previous version of PHP. I'm sure there is a more compatible way to patch this, I just don't have the time to figure it out at the moment.

itsTheFae avatar Oct 14 '21 16:10 itsTheFae

WordPress trac has a reference to the problem function, and maybe a more compatible solution than my first.

A typical way to do this would be to use array_values() on the array before passing it to call_user_func_array(), though beware that will break code written for PHP 8 which actually expects the label to be used.

Taken from Task 3 on the ticket here: https://core.trac.wordpress.org/ticket/51553

With that in mind the lines 300, 301 as well as 360 and 361 should be changed respectively to the following:

$all_old_widget_ids = array_unique( call_user_func_array( 'array_merge', array_values($old) ) );
$all_new_widget_ids = array_unique( call_user_func_array( 'array_merge', array_values($new) ) );

This works, no errors, warnings, or unintended side effects to speak of with the versions noted in the first comment.

itsTheFae avatar Oct 15 '21 16:10 itsTheFae

Thanks for reporting the issue @itsTheFae!

I'm wondering what the original intention was for using call_user_func_array() here:

https://github.com/xwp/stream/blob/4654c31e494b3a96d87ea9b11d307e9a466d7848/connectors/class-connector-widgets.php#L299-L305

and here:

https://github.com/xwp/stream/blob/4654c31e494b3a96d87ea9b11d307e9a466d7848/connectors/class-connector-widgets.php#L359-L362

kasparsd avatar Oct 18 '21 12:10 kasparsd

call_user_func_array is at least faster than doing a manual loop. Unpack via ... isn't available until 5.6 which I guess we can assume no one uses php older than that and use something like array_unique(array_merge(...array_values($old))); instead.

itsTheFae avatar Oct 18 '21 22:10 itsTheFae

Any progress here? This is blocking my migration of a fair few sites to PHP8.

anthonyeden avatar May 17 '22 01:05 anthonyeden

I am facing the same issue while updating a new widget. It gives a 500 error. Is there any way to resolve this?

vishaldabhade avatar Aug 08 '22 15:08 vishaldabhade

@kasparsd What can I do to help patch this bug? Shall I fork and make a PR for one of the changes I've mentioned in this issue? Which method would you prefer?

I unfortunately don't have time to figure out what rational was behind the original choice of calls. WordPress core made use of this pattern, so the easy guess is someone else just borrowed that method when looking for ways to interact with the array of widget data. While I've been using WordPress for some years, I don't have expert input on their data structures and design choices.
It's been a few months since I opened the issue, and having moved across the states since then I'll have to dig back into the code and various docs to see if my previous patches are (still) sane.
So far, the patch is working without issue for me. Are there a battery of tests I can run against these changes?

I look forward to getting a patch done so we can use this plugin with WordPress and PHP 8.

Thanks for your time.

itsTheFae avatar Aug 16 '22 16:08 itsTheFae

We have also run into this issue with Stream on PHP 8 and patching class-connector-widgets.php with applying array_values() as @itsTheFae recommended fixed the issue and seems to be working fine, and it is properly logging widget adds/removals. I have created PR #1355 with that fix and hopefully, we will see a release soon with this fix included.

ParhamG avatar Aug 23 '22 15:08 ParhamG

Version 3.9.1 should be out on WP.org in a few minutes which includes this fix!

kasparsd avatar Aug 23 '22 17:08 kasparsd