WordPress.org

Make WordPress Core

Opened 19 months ago

Closed 18 months ago

Last modified 8 months ago

#42752 closed defect (bug) (fixed)

Rest api - can not convert array to string notice

Reported by: steffanhalv Owned by: 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
19 months 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
19 months 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
19 months 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 key 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?

Last edited 19 months ago by steffanhalv (previous) (diff)

#4 @thepanther71
19 months 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
19 months 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 19 months ago by steffanhalv (previous) (diff)

#6 follow-up: @steffanhalv
18 months 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 18 months ago by steffanhalv (previous) (diff)

#7 in reply to: ↑ 6 @steffanhalv
18 months 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
18 months 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 18 months ago by dd32 (previous) (diff)

#9 @dd32
18 months 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
8 months ago

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