Fix broken is_bool() check on boolean request args - #2630

Current coverage is 94.56% (diff: 100%)

Merging #2630 into develop will increase coverage by 0.01%

@@            develop      #2630   diff @@
==========================================
  Files            11         11          
  Lines          3611       3622    +11   
  Methods         172        172          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           3414       3425    +11   
  Misses          197        197          
  Partials          0          0          

Powered by Codecov. Last update 1ae7f9c...3beb9ad

@WP-API/amigos ##reviewmerge

I think, we just moved the problem a bit further.

If we do ?hide_empty=false

We will get
if ( $prepared_args['hide_empty'] ) echo "hello"; //echos hello although set to false

what is not intended and finally in get_terms() it is only checked
if ( $args['hide_empty'] && !$hierarchical ) { $where .= ' AND tt.count > 0'; }

so basically, it would add the AND tt.count > 0 because the string "false" is true.

I think, if we loosen the is_bool() check we need to sanitize afterwards in get_items().This was my initial idea, why I thought a function with a "tripod"-return (true|false|WP_Error) might be helpful, so we wouldn't need a function to validate and a function to sanitize.

I want to go these days through the plugin and see, what other booleans we have in the schemas, because we would need to go through all of them, right?

I like this PR. Could we add test cases for each of the four strings, or is that not necessary?

When using WP_REST_Controller::get_endpoint_args_for_item_schema(), is there a potential case where accepting such a wide array of values would be a bad thing?

I would suggest strictly sanitizing the value to a true boolean, which can then be validated with is_bool(). That way a lot of the logic in the endpoint can act on strict comparison instead of loose. You can put the logic in rest_sanitize_request_arg() and from there on out the rest of the code can act as though the value is indeed a real boolean.

so basically, it would add the AND tt.count > 0 because the string "false" is true.

This is the main reason why I believe we would want a sanitization function to take loose values and conform them to a strict boolean value.

I want to go these days through the plugin and see, what other booleans we have in the schemas, because we would need to go through all of them, right?

I think we would definitely want to do this. We also need to do this with arrays at some point.

I think should be a sanitization transformation on the property rather than validity. Having it so would also address @websupporter concern about 'false' still being in the request object's data. (string) "false" is a quirk of URL params, so the sooner we can make it a "real" boolean the better. So the flow would be:

1. Send hide_emtpy=false
2. Sanitize_callback rest_boolval => "false" => (bool) false
3. Validate callback is_bool (as usual) => true

@joehoyle

I think should be a sanitization transformation on the property rather than validity.

Right now we don't have any sanitization, and when we do add it the sanitization would run after the validation. I agree, we should correctly set the type in the sanitization function. However, if we ONLY sanitize the value folks can end up with unexpected results due to how booleans are cast: http://php.net/manual/en/function.boolval.php

I think we should do both.

@websupporter @BE-Webdesign I agree we need to sanitize the value as well, filed in #2633. I originally was hoping to keep that a separate (while still VERY related) issue. That does not sound like it is possible due to the inconsistencies of our conditional checks within the different endpoints.

@joehoyle
validation is done earlier than sanitization. I was wondering myself, was introduced some weeks ago:
https://core.trac.wordpress.org/changeset/37943
https://core.trac.wordpress.org/ticket/37192

@websupporter ahh I hadn't noticed that - I added a comment to https://core.trac.wordpress.org/ticket/37192 with my thoughts. Personally I think that was a mistake, but further discussion on that should probably go on https://core.trac.wordpress.org/ticket/37192

Should we merge this when sanitize PR is ready?

@rachelbaker do you think we should support (int) 0 / (int) 1 here? URL params are not the only way to get data in, a JSON blob could be passed in, and I don't think the rest_is_bool function can handle ints?

Labeled needs refresh to account for integers, @BE-Webdesign is going to work on updating the patch.

This was added in #2704.