WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.35k stars 4.13k forks source link

addQueryArgs with boolean false isn't handled right in wp_rest_request->get_params() #30428

Open ivankruchkoff opened 3 years ago

ivankruchkoff commented 3 years ago

Description

buildQueryString converts boolean false to a string "false" which WP_REST_Request->get_params() in the rest api is then unable to parse as a boolean false as it is truthy. I created a mu-plugin to demo the issue.

Cross raised here and in WordPress trac as WP#52956 as both the js library and php code need to cooperate to ensure that true/false are handled / encoded correctly.

Step-by-step reproduction instructions

wp.url.buildQueryString( { foo: false } ) gives us "foo=false"

  1. wp.url.addQueryArgs( 'endpoint', { foo: false } ); will give us endpoint?foo=false
  2. We make a request with apiFetch
  3. In our rest route callback function, we return $request->get_params();

Expected behaviour

As we're sending foo = false, we would expect to receive foo = false, and to also be able to use php empty to correctly interpret this variable.

Actual behaviour

foo = "false" which is truthy in both js and php

Screenshots or screen recording (optional)

Screen Shot 2021-03-31 at 3 09 40 PM

Code snippet (optional)

I created a mu-plugin which highlights the bug: https://core.trac.wordpress.org/attachment/ticket/52956/poc.php

  1. We register_rest_route to create a handler that just returns $request->get_params
  2. var path = wp.url.addQueryArgs( '/poc/foo', { bar: false } );
  3. wp.apiFetch( { path: path } ).then( response => { console.log( response.bar === false ); } ); // response.bar = "false" not false

WordPress information

Device information

talldan commented 3 years ago

Just noting that the related trac ticket has a 'close' keyword added, so we may want to follow suit on this issue: https://core.trac.wordpress.org/ticket/52956#comment:2

But probably worth seeing how the discussion develops there.

gziolo commented 3 years ago

Quoting @TimothyBJacobs from the link in the shared comment:

There is no way to encode a boolean value in a URL. Instead, you need to define the expected type of your request argument using JSON Schema in your route's args definition: https://developer.wordpress.org/rest-api/extending-the-rest-api/schema/ rest_sanitize_boolean will then convert a 'false' string to a boolean false value.

I don't think we can do much about it, this is howJSON.stringify works with booleans:

Screen Shot 2021-04-14 at 15 38 09
ivankruchkoff commented 3 years ago

@gziolo yes, this is how JSON.stringify works, and it's unfortunate that there is no standard for encoding boolean values in query strings. There are valid scenarios where we can't reasonably determine our rest schema and unable to utilise rest_sanitize_boolean

This does create a developer experience where using two WordPress projects/libraries to interface with each other to lead to unexpected behaviours.

Perhaps one alternative is to update the documentation? https://developer.wordpress.org/block-editor/reference-guides/packages/packages-url/#addQueryArgs Something like: If the args object contains boolean values, ensure that your endpoint is configured to parse "true" and "false" as boolean true/false.

Reading the API docs linked in the core trac ticket: https://developer.wordpress.org/rest-api/extending-the-rest-api/schema/#type-juggling led me to believe that string "false" would become boolean false

Because the WordPress REST API accepts URL form encoded data both in the POST body or as the query portion of the URL, many primtive types perform type juggling to convert these string values into their proper native types.

boolean Booleans, the integers 0 and 1, or the strings "0", "1", "false", and "true". 0 is treated as false and 1 is treated as true.