Add default value to register meta

[spacedmonkey]

Add a default value field to register meta. Meaning that if a metat value isn't set that is returns a default value. This bring the meta register api inline with register_setting, that already has a default value set.

[flixos90]

This should be tackled after #38323.

@spacedmonkey For the approach, I suggest we follow how option defaults are handled. Based on the work in the above ticket, we can introduce filters like default_{$object_type}_meta_{$meta_key} and default_{$object_type}_meta_{$meta_key}_for_{$object_subtype}. In register_meta(), when a default is passed, we can hook in a new filter function similar to filter_default_option(), that would take care of what you did in get_metadata_default() in meta_default.diff​.

[spacedmonkey]

In 43941.2.diff I have updated the patch after #38323 has been merged into core.

This patch adds the following filters default_{$object_type}_meta_{$meta_key}" and default_{$object_type}_meta_{$meta_key}_for_{$object_subtype}. This tries to keep the format of other filters like sanitize_{$object_type}_meta_{$meta_key}.

I have not included tests, as I am not sure, the approach is fixed and would love feedback on naming of functions and filters.

CC @kadamwhite @flixos90 @rmccue

[flixos90]

43941.2.diff​ is mostly on the right track IMO (filter names are good!). Just a few remarks:

• Only one of the two filters should be applied. Look at sanitize_meta() for example to see how it handles those filters. I'd suggest checking ! empty( $object_subtype ) && has_filter( "default_{$object_type}_meta_{$meta_key}_for_{$object_subtype}" ), and if so only run the subtype-filter and return immediately. Otherwise, fall back to "default_{$object_type}_meta_{$meta_key}".
• The function get_metadata_default() would be a lot simpler and more flexible if it only executed the filters and didn't touch $wp_meta_keys. The actual logic to get the registered default value should be added as a filter hook via register_meta() (and also removed again via unregister_meta_key(). This would be in line with how default values are registered for settings.
• The argument for the default value should be called default instead of default_value. This is in line with how it works for settings.
• get_metadata_default() as well as the two filters need to be aware of the value of $single, in order to allow developers full flexibility in handling this. The default value may need to be different, depending on how a meta value is requested.

And two smaller detail improvements:

• Instead of using null|array|string for the default value in docs, it should use mixed. A default may be anything, also an integer, float, bool or even object.
• The docblock for the function needs proper third-person description, a since annotation, plus parameter and return value descriptions.

[spacedmonkey]

Thanks for the feedback @flixos90 . My response

Only one of the two filters should be applied. Look at sanitize_meta() for example to see how it handles those filters. I'd suggest checking ! empty( $object_subtype ) && has_filter( "default_{$object_type}_meta_{$meta_key}_for_{$object_subtype}" ), and if so only run the subtype-filter and return immediately. Otherwise, fall back to "default_{$object_type}_meta_{$meta_key}".

I am not sure I like this idea. I personally hate conditional filters in core, it is means that developers have to route through code and fine out when filters are fired. This is confusing. Firing both filters here does the same thing functionally, not sure why we need to change it.

The function get_metadata_default() would be a lot simpler and more flexible if it only executed the filters and didn't touch $wp_meta_keys. The actual logic to get the registered default value should be added as a filter hook via register_meta() (and also removed again via unregister_meta_key(). This would be in line with how default values are registered for settings.

So my original patch did this, but I couldn't get it working without with either,

• Using an an anonymous function, which are not supported in PHP 5.2
• Using create_function, which is not supported in later versions of PHP.
• Create two new functions, that are single use. This feels a little pointless and weird. Might just be me. Am I missing something here?

The argument for the default value should be called default instead of default_value. This is in line with how it works for settings.

I would agree with that. I wanted it to be super clean, it we all believe default is fine, I will change it.

get_metadata_default() as well as the two filters need to be aware of the value of $single, in order to allow developers full flexibility in handling this. The default value may need to be different, depending on how a meta value is requested.

How do you intend this works? So in register_meta, should it have default_single and default_multi. If so, if default_multi isn't set, does it fallback to default_single.This doesn't feel right. I personally think of defaults as defaulting a single value. If anything, I wouldn't support defaulting the multiple values.

Regarding docs, as for unit tests, the docs were not correct at all. Just there to help review . Once this patch is in a more final state, as in we have locked down the design, I will comment for real.

[flixos90]

Replying to spacedmonkey:

I am not sure I like this idea. I personally hate conditional filters in core, it is means that developers have to route through code and fine out when filters are fired. This is confusing. Firing both filters here does the same thing functionally, not sure why we need to change it.

We decided earlier for #38323 that only the subtype-specific filters should fire if used, with the other ones only executed as fallback. This is the case for both sanitize and auth filters, and it should be the same here.

So my original patch did this, but I couldn't get it working without with either,

• Using an an anonymous function, which are not supported in PHP 5.2
• Using create_function, which is not supported in later versions of PHP.
• Create two new functions, that are single use. This feels a little pointless and weird. Might just be me. Am I missing something here?

I agree that this would be far easier if we had anonymous functions, but it's also okay to introduce a one-off function for that. Please check how this is handled for options (see filter_default_option()).

get_metadata_default() as well as the two filters need to be aware of the value of $single, in order to allow developers full flexibility in handling this. The default value may need to be different, depending on how a meta value is requested.

How do you intend this works? So in register_meta, should it have default_single and default_multi. If so, if default_multi isn't set, does it fallback to default_single.This doesn't feel right. I personally think of defaults as defaulting a single value. If anything, I wouldn't support defaulting the multiple values.

I absolutely agree we should not have two default values. However I think we need to think of the filters and the registered default value a bit more separately. The filters themselves need to be aware of how the meta value is requested. However, when registering, there should only be a single default value. For example, if a meta key is registered with single => false and default => array( 'my_value, 'my_value_2' ) and someone calls get_metadata() with $single set to true, the default filter callback would need to handle this, and only return the first value from the array (which here would be my_value). It is unfortunate that we still have this $single parameter although it could now automatically be handled. But since it is there, we definitely need to ensure the returned default value takes this into account.

[spacedmonkey]

That was going to be a my next question.

Should the default value, obey the single an type definitions? Should be cast the default value, to make sure it returns the right type. Say make sure it is returning a number instead an array for example.

It also seems like the rest api has decided for us, by only allowing a single value to returned as a default. See ​this line.

[flixos90]

I think we should discuss in detail in a REST API meeting whether register_meta() should require default to always be a $single value or whether it should dynamically adjust depending on whether $single is true or false. I'm thinking we need to do the latter because otherwise it would be impossible to register an array of default values where $single is false, which would be a perfectly valid use-case.

[spacedmonkey]

In 43941.diff all the logic has been moved to filters, to keep inline with other functionality in the meta api.

I also changed the name of the key to default and change the default value to null to keep inline with existing values in the rest api.

[danielbachhuber]

Per #core-restapi Slack conversation today, punting this out of 5.0 given:

• It's not a necessary requirement for Gutenberg.
• There's still some complexity around the unknowns.

[spacedmonkey]

In 43941.3.diff I have created a brand new patch after a chat IRL with @flixos90 and @swissspidy .

This new patch, allows developers to register single and multiple defaults. For single defaults (single => true), any value that passed as default, is used. However, defaults (single => false) REQUIRES a numbric array be passed. I think that this patch is a workable solution.

Once we are happy, I can add some unit tests.

[TimothyBlynJacobs]

I'm +1 to this approach.

I think it might be worth considering checking the provided defaults match the registered meta type as well.

[rmccue]

I just ran into a problem where 43941.3.diff​ would have solved it, so +1 on it.

One note: to make it possibly more consistent with options, a filter with the key in it might be useful, so that you don't have to hook in and check the arguments.

[spacedmonkey]

I have updated my patch with 43941.4.diff. Patch fixes an issue with update meta data with default data set. Also adds unit tests. It is one tests with lots of data passed to it. I think worked out all the edge cases, but maybe more.

[TimothyBlynJacobs]

Uploaded a patch to support a null default type as discussed in last week's office hours. I don't think supporting null is a blocker, but it'd be a bit difficult to patch in later due to the args filter in register_meta and null would be the best default for a date-time meta type that is optional.

<?php
register_meta( 'post', 'my_date', array(
    'type' => 'string',
    'show_in_rest' => array(
        'type' => array( 'string', 'null' ),
        'format' => 'date-time',
    ),
) );

[kadamwhite]

PHPCBF

[kadamwhite]

I've uploaded one more patch to remove the addition of 'default' => null to the testing arrays, as @TimothyBlynJacobs' latest patch swapping our default check to use array_key_exists means we can specify no default best through omission.

However, we've encountered an issue introduced in one of the earlier patches which may require us to punt to 5.4 early. Timothy will elaborate.

[kadamwhite]

Remove declaration of "null" default values from meta test registration arrays

[kadamwhite]

Infinite recursion on test startup was introduced in 43941.4.diff, Timothy will post further details. There may also be an issue where 'null' will be returned for the default if at least one meta key has default values registered for a given type. Punting to 5.4-early.

[TimothyBlynJacobs]

We found an infinite loop that occurred during wp_install() when bootstrapping PHPUnit. The issues comes down to creating a WP_User instance. To do that, the get_caps_data function is called which tries to load the wp_capabilities meta key.

When a new user is constructed, this meta key won't exist, so the get_metadata_default function is called. This calls the filter_default_metadata. That function uses get_object_subtype to determine the correct registered meta key entry to use. get_object_subtype then tries to create a WP_User instance. And now we loop.

I think this problem would occur anywhere where get_metadata is called during the bootstrapping of a WP model of some kind. I think this is constrained to WP_User, but I haven't been able to check for sure yet. It might be possible to workaround this by unhooking the default meta filter during the get_user_meta call.

---

Additionally, it looks like there might be an issue with the default meta value changing to return null instead of an empty string or array if no default value is registered is set when registering a meta key, but at least one is specified for the meta type.

We also need to decide how meta defaults should work with custom filters. Right now, in update_metadata we unhook the default filter to get the raw current value, but that won't work if someone provides a default by using a filter.

[TimothyBlynJacobs]

Blurgh, accidentally undid the keyword changes.

[davidbaumwald]

Per @TimothyBlynJacobs this is being moved to Future Release. If any maintainer or committer feels this can be included in time for 5.4 Beta 1, or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

Marking for a refresh for the @since as well, for whatever cycle it ends up landing in.

[mnelson4]

@johnjamesjacoby what do you think about adding a new parameter onto get_metadata that would get the unfiltered value? Otherwise we're needing to unhook and rehook filters like in https://core.trac.wordpress.org/attachment/ticket/43941/43941.9.diff

[spacedmonkey]

I have uploaded a new patch in 43941.10.diff.

Key changes.

• Add new param to get_metadata to get unfiltered values.
• Handle different types of arrays.
• Added more tests.
• Updated patch for 5.5.
• Linted patch.

Add a default value field to register meta. Meaning that if a metat value isn't set that is returns a default value. This bring the meta register api inline with register_setting, that already has a default value set.

Trac ticket: https://core.trac.wordpress.org/ticket/43941

Add a default value field to register meta. Meaning that if a meta value isn't set that is returns a default value. This bring the meta register api inline with register_setting, that already has a default value set.

Trac ticket: https://core.trac.wordpress.org/ticket/43941

[johnbillion]

I would like to discuss the $unfiltered parameter more, it's not a pattern I like elsewhere in WordPress as it doesn't give an indication of the real purpose of the parameter. Its usage in 43941.11.diff​ is misleading because it only disables some filters in get_metadata().

As a developer, when might I want or need an unfiltered value? Why isn't this parameter available in get_post_meta() and corresponding wrapper functions? Should it be? Why are some filters still applied when this parameter is set to true?

I would rather see a new function for returning the raw value of a meta field without its default value being applied. This makes the intention more explicit and is what's needed here.

[spacedmonkey]

Based on feedback I updated the patch / ​PR. I have added a new function called get_metadata_raw that gets the raw value without a default / filter.

Can you review again please @johnbillion ?

[spacedmonkey]

Updated patch to fix issue with unit test failing.

[spacedmonkey]

In 43941.15.diff I tweaked the filter_default_metadata function.

Basically, I looped for the $wp_meta_keys, checking if the meta type, meta key and if the default value is set, before loading the sub type. That means that basically the sub type will not be loaded.

I am happy with this tweak, as the unit tests are still passing.

[spacedmonkey]

New patch 43941.16.diff.

Use array_key_exists over isset as isset doesn't work on null values.

[davidbaumwald]

With 5.5 Beta 1 approaching, the time for early tickets has now passed, and this ticket is being moved to Future Release. If any maintainer or committer feels this can be quickly resolved for 5.5 or wishes to assume ownership of this ticket during a specific cycle, feel free to update the milestone accordingly.

@spacedmonkey can you rebase this?

@TimothyBJacobs Rebased. It seems to be a small conflict with a comment. See ​https://github.com/WordPress/wordpress-develop/commit/83b0d3709ede9382111e320dde4dbe1d4b0bc717

[spacedmonkey]

Sent a long time on this ticket over the weekend. Made some key improvements to the patch.

• Removed code from rest meta that was casusing a notice error on register meta ( this happens in core too ).
• Added validation of type for defaults. But if type is integer and string is provided, then a doing it wrong message is triggered. I think this makes sense here and will help developers quickly debug invalid default types.
• Added unit tests for post and term meta defaults.
• Improved handling of arrays and objects default data.

There seems to be some tests failing in my PR, but they seem completely unrelated to this change.

Pinging @TimothyBlynJacobs and @kadamwhite in hopes of another review.

It looks like the default handling in get_registered_fields still needs to be updated.

get_registered_fields

I am not sure I know what you mean here. I did keep this method in mind when making the original patch.

Take the following example
`

register_meta('post', 'my_meta', [

'object_subtype' => 'post',
'single' => false,
'show_in_rest' => [

'schema' =>[

'type' => 'array',
'items' => [

'type' => 'integer'

]

]

],
'type' => 'array',
'default' => array(233),

]);

`

Output this in the schema.

<img width="332" alt="Screenshot 2020-07-07 at 10 44 55" src="​https://user-images.githubusercontent.com/237508/86763326-28e83280-c03f-11ea-80b6-12bf08ce45bf.png">

This seems right to me.

Is there something I missed @TimothyBJacobs ?

This seems right to me. Is there something I missed @TimothyBJacobs?

I see, I expected it to be handled differently but this works fine!

[TimothyBlynJacobs]

Unfortunately we need to punt this to 5.6, we discovered some issues related to single array meta keys where the meta value returned would be invalid if requested with $single => false in get_metadata.

In the existing test cases, this worked for the REST API because of its sanitization. But it wouldn't be able to for more complex examples. For non REST consumers, the values would also be invalid.

​https://wordpress.slack.com/archives/C02RQC26G/p1594143017123200

I pushed some failing tests to cover cases when using an array type.

We have some wp_is_numeric_array() checks to seamlessly upgrade values when using a multiple type. I think we need to drop all these checks and always enforce that a user passes the correct defaults for the type.

In filter_default_metadata we should remove the numeric array check as well, and then also add handling to the non-single case to check if it was registered as a multiple. If not, it should wrap the returned value in an array.

[TimothyBlynJacobs]

In 48402:

REST API, Meta: Introduce support for default metadata values.

The register_meta() API now officially supports specifying a default metadata value. When get_metadata() is called for a meta key that does not yet exist for the object, this default value will be returned instead of an empty string.

A new function is introduced get_metadata_raw to retrieve the raw metadata value from the database, without applying the registered default.

Props spacedmonkey, flixos90, rmccue, kadamwhite, mnelson4, johnbillion, chrisvanpatten, TimothyBlynJacobs.
Fixes #43941.

[TimothyBlynJacobs]

As pointed out by @aaroncampbell the doc blocks for get_metadata and get_metadata_raw are reversed. Will fix after beta 1 is released.

[SergeyBiryukov]

In 48411:

Docs: Synchronize and correct documentation for various metadata functions and filters.

Follow-up to [47390], [47611], [48192], [48402].

See #49572, #43941, #45464.

[SergeyBiryukov]

Is it intentional that get_metadata_raw() and get_metadata_default() have different function signatures?

• get_metadata_raw( $meta_type, $object_id, $meta_key = '', $single = false )
• get_metadata_default( $meta_type, $meta_key, $single = false, $object_id = 0 )
• filter_default_metadata( $value, $meta_type, $meta_key, $single, $object_id )

Same goes for get_{$meta_type}_metadata and default_{$meta_type}_metadata filters:

• apply_filters( "get_{$meta_type}_metadata", null, $object_id, $meta_key, $single )
• apply_filters( "default_{$meta_type}_metadata", $value, $meta_type, $meta_key, $single, $object_id )

The latter has a $meta_type parameter, the former does not (in fact, none of the other 20 *_{$meta_type}_meta or *_{$meta_type}_metadata filters have it). I think it should at least be passed to get_{$meta_type}_metadata too.

43941.17.diff​ attempts to bring some consistency.

[spacedmonkey]

@SergeyBiryukov The reason that get_metadata_default has a different signatures, as it could be called without a object_id or single. These params are optional so have to be at the end.

[johnjamesjacoby]

Super excited to see this committed!! ❤️

These params are optional so have to be at the end.

In my own code (and in bbPress 2.x) nearly all function parameters are defined as optional but with correctly cast default values. This approach allows for maintaining consistent function signatures. It will not spill PHP notices when "required" parameter values are falsy. It forces every function (and the rest of the surrounding application) to internally consider and pre-flight check all of the parameter values before proceeding.

Having the signature more closely match get_metadata() and get_metadata_raw() seems preferable.

It is not immediately obvious in the code when or why get_metadata_default() would accept an empty $object_id. It seems obscure enough that a signature change like I'm proposing below might be less likely to hang people up (with accompanying docs to explain why it's different):

function get_metadata_default( $meta_type, $object_id = 0, $meta_key = '', $single = false ) {

[TimothyBlynJacobs]

What happens when you don't pass a meta key then? From looking at that function signature, it isn't possible to tell what parameters are required.

[spacedmonkey]

I strong disagree with the changing of the params here. $meta_type and $meta_key required here, as defaults are defined for type and key. If I was developing something I may want to do this.

<?php
 if( get_user_meta( 123, 'test', true ) !== get_metadata_default( 'user', 'test', true ) ) {
    // do stuff. 
 }

Note I did not pass object id here.

The only change I would think about is making less params optional. So

<?php
get_metadata( $meta_type, $object_id, $meta_key, $single = false )

[TimothyBlynJacobs]

Looking at the $object_id and $single parameters, while they are listed as optional in the function signature, they don't actually seem to be optional to me.

The $object_id parameter is needed to retrieve the subtype for an object. So omitting it wouldn't allow you to get the correct value for any of the built-in types.

The $single parameter defaults to false, but it isn't actually optional.

So it seems like we could safely make all those parameters optional and then match the order of the other functions.

Change order of param on get_metadata_default function.

Trac ticket: https://core.trac.wordpress.org/ticket/43941

[spacedmonkey]

I will take the recommendations of @TimothyBlynJacobs and @johnjamesjacoby . Here is ​PR with the change.

[SergeyBiryukov]

In 48502:

Meta: Reorder the get_metadata_default() signature to match get_metadata().

The order of parameters in get_metadata_default() did not match the signature of get_metadata(). This could be confusing for developers who are familiar with the existing metadata API.

Fixes #43941.
Props SergeyBiryukov, spacedmonkey, johnjamesjacoby.

[SergeyBiryukov]

In 48504:

Docs: Correct documentation for the $meta_key parameter of get_metadata_default().

The parameter is required, not optional.

Follow-up to [48502].

See #43941.

[spacedmonkey]

@SergeyBiryukov We should really change the params on this ​filter.

[SergeyBiryukov]

In 48505:

Options, Meta APIs: Reorder the parameters of default_{$meta_type}_metadata filter.

This brings consistency with the get_{$meta_type}_metadata filter and more closely matches the get_metadata_default() function signature.

Follow-up to [48502].

Props spacedmonkey.
See #43941.

[justinahinon]

Dev note link: ​https://make.wordpress.org/core/2020/08/04/registering-default-values-for-meta-data/