#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: | 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)
Change History (19)
#1
@
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
This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.
5 years ago
#3
@
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
#6
@
5 years ago
Attached a patch scoping the default value application only to scalar types as discussed in #core-restapi.
#7
@
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
@
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
@
5 years ago
- Owner changed from TimothyBlynJacobs to kadamwhite
- Status changed from accepted to assigned
#11
@
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+.
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 ifget_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.