Empty string in database for boolean meta not cast to false

[chrisvanpatten]

In WordPress 5.3 RC1, when registering post meta as a boolean and saving a false value via the REST API, it is persisted to the database as an empty string, but the empty string is not cast back to false on output, and is instead rendered in the API as null.

<?php

register_meta(
	'post',
	'my_meta_key',
	[
		'type'         => 'boolean',
		'single'       => true,
		'show_in_rest' => true,
	]
);

Expected in API:

...
"meta": {
    "my_meta_key": false
}
...

Actual:

...
"meta": {
    "my_meta_key": null
}
...

[TimothyBlynJacobs]

The specific regression here is that we are instead validating and sanitizing the output of get_metadata instead of simply casting it to its primitive type. Discussing with @kadamwhite, I think the fix here is to check if get_metadata returns an empty string, and if so, return the default value instead.

This effects the settings controller in a similar way, but I think it'd be best to address that in a separate ticket for 5.4.

<?php 
function test_get_item_with_false_stored_for_integer_value() {
        wp_set_current_user( self::$administrator );

        register_setting(
                'somegroup',
                'mycustomsetting',
                array(
                        'type'         => 'integer',
                        'show_in_rest' => true,
                )
        );

        $request  = new WP_REST_Request( 'GET', '/wp/v2/settings' );
        $response = rest_get_server()->dispatch( $request );

        $this->assertSame( 0, $response->get_data()['mycustomsetting'] );
}

[kadamwhite]

This solution feels well-scoped and makes sense to me.

[TimothyBlynJacobs]

Attached a patch scoping the default value application only to scalar types as discussed in #core-restapi.

[kadamwhite]

I've been talking through this with @TimothyBlynJacobs in a slack thread, which was unfortunately within a DM; summarizing my thoughts here:

As I've considered this change my biggest concern is that we're conflating the concepts of an empty and a default value. Technically the method we've introduced as get_default_for_type() is really returning the value to use when we encounter an _empty_ meta value, not the schema default. We happen to use the result of this function as the default within the schema, but as Timothy said in the thread, this is an implementation detail. The regression in this thread is about the handling of empty values, not schema defaults.

The naming of the method we're using is therefore confusing. Timothy has proposed therefore that we should consider renaming this method, and I concur; we're going to alter it to read get_empty_value_for_type.

[kadamwhite]

The new patch looks good to me. Marking dev-feedback / commit, and seeking one more committer for review per RC guidelines.

[rmccue]

KAdam pinged me for a dev-feedback review on 48363.3.diff​ so notes below:

Using self:: makes it impossible to subclass. This should either use static:: (5.3+; assuming this is allowed in the coding standards now?) or $this-> (which can be used on static methods).

Otherwise, patch looks good to me, r+.

[TimothyBlynJacobs]

Added a patch to use static::, I can't find any rules forbidding it.

[rmccue]

Let's go with it.

[kadamwhite]

In 46563:

REST API: Cast empty meta values to correct scalar types in REST response.

Introducing complex meta value handling in [45807] unintentionally removed value casting for empty scalar meta values.

Props TimothyBlynJacobs, chrisvanpatten, rmccue, kadamwhite.
Fixes #48363.