Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#42752 closed defect (bug) (fixed)

Rest api - can not convert array to string notice

Reported by: steffanhalv's profile steffanhalv Owned by: dd32's profile dd32
Milestone: 5.1 Priority: normal
Severity: normal Version: 4.9
Component: Query Keywords:
Focuses: Cc:

Description (last modified by dd32)

A PHP Notice can be generated on REST API requests

Notice: Array to string conversion in wp-includes/class-wp.php on line 305

One Request which can be used to trigger it is this:

http://localhost:9001/?_method=PUT&downloads[0][file]=http://localhost:9001/wp-content/uploads/2017/11/1600x1600___112808_1600x1600_v1-26.png&downloads[0][name]=1600x1600___112808_1600x1600_v1-26-png&downloads[1][file]=http://localhost:9001/wp-content/uploads/2017/11/1600x1600___pgs_es_spez_13_top_1600x1600_v1-167.png&downloads[1][name]=1600x1600___pgs_es_spez_13_top_1600x1600_v1-167-png

Please see link for details:

https://github.com/woocommerce/woocommerce/issues/17954#issuecomment-348028093

Thx

Change History (10)

#1 @dd32
7 years ago

  • Description modified (diff)

Hi @steffanhalv,

I've updated the description here to have a bit of the information from your woocommerce ticket for ease of searching.

This specific case is because something on your site appears to be defining downloads as a Query Parameter (which can only be strings), also using that parameter in the $_GET variables of a REST API request causes this kind of notice.

It may also cause the REST API request to return different results depending on what the plugin actually does.

I'm fairly certain this is a duplicate of another ticket, which I can't find right now - something about checking for is_scalar() in parse_request().

#2 @steffanhalv
7 years ago

  • Summary changed from Rest api - can not convert string to array notice to Rest api - can not convert array to string notice

#3 @steffanhalv
7 years ago

Thx, I can tell you that the put request is correctly performed (the updates is beeing made), but the notification response is a problem in our application.

Currently handled by catching errors and refetch which is not optimal.

Im glad you are working on it and looking forward for updates!

@dd32

Edit:

Are u saying that 'downloads' is not allowed as query parameter and that woocommerce should change its name?

Or that the queryparameter 'downloads' doesnt allow arrays? At least I do not have problem using arrays with other queryparams ex. Categories +++

Im still strugling getting the full picture of why this does not work, but perhaps the other ticket you seen explains it better?

Version 1, edited 7 years ago by steffanhalv (previous) (next) (diff)

#4 @thepanther71
7 years ago

I had this same issue with a plug-in I'm developing. I was trying to get the REST api to work with datatablesJS (datatables.net) in server mode. Their API sends a bunch of _GET parameters, including an array for "&search" and "&order". This caused the Wordpress API to fail on the same error code as &search and &order are expected to be strings.

For my testing (only 2 days so far), I was able to get things working by adding json_encode to the string cast in "wp-includes/class-wp.php" line 305

I havent done extensive testing for side effects, but this seems to be working for everything Ive tried so far. Basically, it will json encode parameters if they come in as an array...
<code>

     $this->query_vars[ $wpvar ][ $vkey ] = (string) json_encode($v);

The current structure allows $v as an array to enter this block.... Since you cant cast an array to a string we get that notice.

I'm not familiar enough with WP base to know where else this is used that may have an effect... testing for a few days before I formally send it as a bug fix, but maybe @dd32 has more of an idea if it should have unwanted side effects.

USE AT OWN RISK... I dont know the side effects on security... $v was cast as a string for an intentional reason... forcing a json_encoded array may need some additional casting inside.

That said, I have not seen any bad side effects with existing API's or my custom ones yet.

#5 @steffanhalv
7 years ago

@thepanther71 Thx, I can try this temp fix on my system to on monday to see if it works with my applications. Im heavingly using the rest api with multidim arrays in querystrings ++, So I think I would catch up if this creates other bugs pretty fast.

Last edited 7 years ago by steffanhalv (previous) (diff)

#6 follow-up: @steffanhalv
7 years ago

  • Keywords Has-commit added

@dd32 @thepanther71

Your fix did work for me to and I dont think casting to json is a big security risk. (Im not having big insights into WP core thought)

I added a commit, as this becomes a blocker for my use.

See:
https://github.com/WordPress/WordPress/pull/329

Last edited 7 years ago by steffanhalv (previous) (diff)

#7 in reply to: ↑ 6 @steffanhalv
7 years ago

Replying to steffanhalv:

@dd32 @thepanther71

Your fix did work for me to and I dont think casting to json is a big security risk. (Im not having big insights into WP core thought)

I added a commit, as this becomes a blocker for my use.

See:
https://github.com/WordPress/WordPress/pull/329

@dd32 I didnt know I couldnt contribute by github and I dont have time to get into the wp contribution workflow as it looks to time consuming to get into it for me.

Could also have a possible relationship with https://core.trac.wordpress.org/ticket/42961#ticket or what do you think? Its both with how querystring is handled at least.

This all makes my rest api code a bit "interesting" if you know what I mean. Its hard to code without commenting allot of why I did this and that.

#8 @dd32
7 years ago

  • Component changed from REST API to Query
  • Focuses rest-api removed
  • Keywords Has-commit removed
  • Milestone changed from Awaiting Review to 5.0
  • Owner set to dd32
  • Status changed from new to accepted

The issue at hand here is that the download GET variable is a registered query_var. Query Vars *cannot* be nested arrays.
The REST API and the rest of WordPress doesn't appear to play nicely in this situation - as on every REST API request you're still triggering the parsing of all the other WordPress routes query vars, resulting in this.

Last edited 7 years ago by dd32 (previous) (diff)

#9 @dd32
7 years ago

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

In 42419:

WP: Don't attempt to convert multiple-nested arrays to a string in WP->parse_request().

See #14330.
Fixes #42752.

#10 @johnbillion
6 years ago

  • Milestone changed from 5.0 to 5.1
Note: See TracTickets for help on using tickets.