WordPress.org

Make WordPress Core

Opened 2 weeks ago

Closed 10 days ago

Last modified 10 days ago

#48363 closed defect (bug) (fixed)

Empty string in database for boolean meta not cast to false

Reported by: chrisvanpatten Owned by: kadamwhite
Milestone: 5.3 Priority: normal
Severity: normal Version: trunk
Component: REST API Keywords: has-unit-tests has-patch commit dev-reviewed
Focuses: rest-api Cc:
PR Number:

Description

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
}
...

Attachments (4)

48363.diff (4.4 KB) - added by TimothyBlynJacobs 2 weeks ago.
48363.2.diff (4.5 KB) - added by TimothyBlynJacobs 13 days ago.
48363.3.diff (4.7 KB) - added by TimothyBlynJacobs 13 days ago.
48363.4.diff (4.7 KB) - added by TimothyBlynJacobs 10 days ago.

Download all attachments as: .zip

Change History (19)

#1 @TimothyBlynJacobs
2 weeks ago

  • Keywords has-patch has-unit-tests dev-feedback 2nd-opinion added
  • Milestone changed from Awaiting Review to 5.3
  • Owner set to TimothyBlynJacobs
  • Status changed from new to accepted

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'] );
}

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.


13 days ago

#3 @kadamwhite
13 days ago

  • Keywords 2nd-opinion removed

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

This ticket was mentioned in Slack in #core-committers by kadamwhite. View the logs.


13 days ago

#5 @SergeyBiryukov
13 days ago

  • Keywords commit added

#6 @TimothyBlynJacobs
13 days ago

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

#7 @kadamwhite
13 days ago

  • Keywords has-patch dev-feedback commit removed

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.

#8 @kadamwhite
13 days ago

  • Keywords has-patch dev-feedback commit added

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

This ticket was mentioned in Slack in #core-committers by kadamwhite. View the logs.


13 days ago

#10 @kadamwhite
13 days ago

  • Owner changed from TimothyBlynJacobs to kadamwhite
  • Status changed from accepted to assigned

#11 @rmccue
10 days ago

  • Keywords dev-feedback removed

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+.

#12 @TimothyBlynJacobs
10 days ago

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

#13 @rmccue
10 days ago

  • Keywords dev-reviewed added

Let's go with it.

#14 @kadamwhite
10 days ago

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

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.

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


10 days ago

Note: See TracTickets for help on using tickets.