WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#37570 closed defect (bug) (fixed)

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

Reported by: jbrinley Owned by: ocean90
Milestone: 4.6 Priority: normal
Severity: normal Version: 4.6
Component: Media Keywords: has-patch needs-testing commit
Focuses: Cc:

Description

When calling media_handle_sideload() with a failed download, the file information is passed (in _wp_handle_upload() to the upload error handler, which defaults to wp_handle_upload_error().

In, PHP7 (and maybe earlier), this throws a warning: "PHP Warning: Parameter 1 to wp_handle_upload_error() expected to be a reference, value given in /srv/www/public/wp/wp-admin/includes/file.php".

Per the notes at http://php.net/call_user_func, "the parameters for call_user_func() are not passed by reference", but it provides a workaround using call_user_func_array().

All of the calls to the error handler should be changed from:

call_user_func( $upload_error_handler, $file, $error_msg )

To:

call_user_func_array( $upload_error_handler, array( &$file, $error_msg ) )

Attachments (3)

37570.diff (3.1 KB) - added by jbrinley 3 years ago.
Replace call_user_func with call_user_func_array
37570.2.diff (3.3 KB) - added by ocean90 3 years ago.
37570.3.diff (987 bytes) - added by ocean90 3 years ago.

Download all attachments as: .zip

Change History (15)

@jbrinley
3 years ago

Replace call_user_func with call_user_func_array

#1 @jbrinley
3 years ago

  • Keywords has-patch needs-testing added

#2 @SergeyBiryukov
3 years ago

  • Component changed from General to Media

#4 follow-up: @ocean90
3 years ago

This is interesting. It looks like the warning was missing in PHP 7.0.0 - 7.0.8 and also in PHP 7.1 Alpha 1 + 2, see https://3v4l.org/fESuN.

The same function uses also a variable function (https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/file.php?rev=38151&marks=273#L245) which doesn't produce the warning.

#5 @ocean90
3 years ago

  • Milestone changed from Awaiting Review to 4.6

We should consider to fix this in 4.6.

The issue is not only the warning, it also prevents the execution of the function, which means that the upload gets treated as successful.

Let's say the $test_uploaded_file condition fails:

Expected:

{
  "success": false,
  "data": {
    "message": "Specified file failed upload test.",
    "filename": "test.png"
  }
}

Actual:

{
  "success": true,
  "data": {
    "id": 32,
    "title": "test",
    "filename": "",
    "url": "http:\/\/src.wordpress-develop.dev\/test\/",
    "link": "http:\/\/src.wordpress-develop.dev\/test\/",
    "alt": "",
    "author": "1",
    "description": "",
    "caption": "",
    "name": "test",
    "status": "inherit",
    "uploadedTo": 0,
    "date": 1470746608000,
    "modified": 1470746608000,
    "menuOrder": 0,
    "mime": "",
    "type": "",
    "subtype": "",
    "icon": "http:\/\/src.wordpress-develop.dev\/wp-includes\/images\/media\/default.png",
    "dateFormatted": "9. August 2016",
    "nonces": {
      "update": "b8499d3bd9",
      "delete": "19e4752d90",
      "edit": "975849a990"
    },
    "editLink": "http:\/\/src.wordpress-develop.dev\/wp-admin\/post.php?post=32&action=edit",
    "meta": false,
    "authorName": "admin",
    "compat": {
      "item": "",
      "meta": ""
    }
  }
}

@dd32, @pento, @jorbin: What do you think?

#6 in reply to: ↑ 4 @ocean90
3 years ago

Replying to ocean90:

This is interesting. It looks like the warning was missing in PHP 7.0.0 - 7.0.8 and also in PHP 7.1 Alpha 1 + 2, see https://3v4l.org/fESuN.

That was the wrong test script. The correct one is https://3v4l.org/p9E7N. It's the nested function and I'm assuming that it's because of https://bugs.php.net/bug.php?id=72508 which was fixed in 7.0.9, see https://github.com/php/php-src/blob/PHP-7.0.9/NEWS#L6.

#7 @jorbin
3 years ago

  • Keywords commit added

I also think this is worth fixing in 4.6. We should always run notice free.

#8 @jorbin
3 years ago

It's also worth noting that this will fail in 7.1 according to the upgrading file:

  . call_user_func() will now consistently fail to perform calls to functions
    that accept reference arguments. Previously this sometimes worked if
    call_user_func() was used outside a namespace.

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


3 years ago

#10 @ocean90
3 years ago

  • Owner set to ocean90
  • Status changed from new to accepted

@ocean90
3 years ago

@ocean90
3 years ago

#11 @ocean90
3 years ago

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

In 38235:

Media: In _wp_handle_upload() use call_user_func_array() to call the upload error handler.

The default error handler wp_handle_upload_error() expects a reference for the first parameter but call_user_func() doesn't pass parameters by reference. The current code didn't produce any issues until now. PHP 7.0.9 (and PHP 7.1) is now stricter and prevents calling the error handler with a warning:

PHP Warning: Parameter 1 to wp_handle_upload_error() expected to be a reference, value given.

To restore the error handler _wp_handle_upload() now uses call_user_func_array().

Props jbrinley.
Props jorbin for review.
Fixes #37570.

#12 @ocean90
3 years ago

In 38236:

Media: In _wp_handle_upload() use call_user_func_array() to call the upload error handler.

The default error handler wp_handle_upload_error() expects a reference for the first parameter but call_user_func() doesn't pass parameters by reference. The current code didn't produce any issues until now. PHP 7.0.9 (and PHP 7.1) is now stricter and prevents calling the error handler with a warning:

PHP Warning: Parameter 1 to wp_handle_upload_error() expected to be a reference, value given.

To restore the error handler _wp_handle_upload() now uses call_user_func_array().

Merge of [38235] to the 4.6 branch.

Props jbrinley.
Props jorbin for review.
See #37570.

Note: See TracTickets for help on using tickets.