Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#48363 closed defect (bug) (fixed)

Empty string in database for boolean meta not cast to false

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

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 5 years ago.
48363.2.diff (4.5 KB) - added by TimothyBlynJacobs 5 years ago.
48363.3.diff (4.7 KB) - added by TimothyBlynJacobs 5 years ago.
48363.4.diff (4.7 KB) - added by TimothyBlynJacobs 5 years ago.

Download all attachments as: .zip

Change History (19)

#1 @TimothyBlynJacobs
5 years 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.


5 years ago

#3 @kadamwhite
5 years 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.


5 years ago

#5 @SergeyBiryukov
5 years ago

  • Keywords commit added

#6 @TimothyBlynJacobs
5 years ago

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

#7 @kadamwhite
5 years 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
5 years 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.


5 years ago

#10 @kadamwhite
5 years ago

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

#11 @rmccue
5 years 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
5 years ago

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

#13 @rmccue
5 years ago

  • Keywords dev-reviewed added

Let's go with it.

#14 @kadamwhite
5 years 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.


5 years ago

Note: See TracTickets for help on using tickets.