WordPress.org

Make WordPress Core

Opened 18 months ago

Last modified 3 weeks ago

#43941 assigned enhancement

Add default value to register meta

Reported by: spacedmonkey Owned by: spacedmonkey
Milestone: 5.4 Priority: normal
Severity: normal Version: 4.6
Component: Options, Meta APIs Keywords: has-patch has-unit-tests early
Focuses: rest-api Cc:
PR Number:

Description

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.

Attachments (8)

meta_default.diff (1.8 KB) - added by spacedmonkey 18 months ago.
43941.2.diff (2.5 KB) - added by spacedmonkey 16 months ago.
43941.diff (5.2 KB) - added by spacedmonkey 16 months ago.
43941.3.diff (3.7 KB) - added by spacedmonkey 7 months ago.
43941.4.diff (10.5 KB) - added by spacedmonkey 4 weeks ago.
43941.5.diff (10.8 KB) - added by TimothyBlynJacobs 3 weeks ago.
43941.6.diff (11.6 KB) - added by kadamwhite 3 weeks ago.
PHPCBF
43941.7.diff (9.2 KB) - added by kadamwhite 3 weeks ago.
Remove declaration of "null" default values from meta test registration arrays

Download all attachments as: .zip

Change History (41)

#1 @flixos90
17 months ago

  • Focuses rest-api added
  • Keywords needs-unit-tests added; needs-testing dev-feedback removed
  • Milestone changed from Awaiting Review to 5.0

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.

This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.


17 months ago

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


16 months ago

#4 @spacedmonkey
16 months ago

In 43941.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

Version 0, edited 16 months ago by spacedmonkey (next)

#5 @flixos90
16 months ago

  • Owner set to spacedmonkey
  • Status changed from new to assigned

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.

#6 follow-up: @spacedmonkey
16 months ago

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.

#7 in reply to: ↑ 6 @flixos90
16 months ago

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.

#8 @spacedmonkey
16 months ago

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.

#9 @flixos90
16 months ago

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
16 months ago

#10 @spacedmonkey
16 months ago

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.

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


16 months ago

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


15 months ago

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


14 months ago

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


12 months ago

#15 @danielbachhuber
12 months ago

  • Milestone changed from 5.0 to Future Release

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.

This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.


10 months ago

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


8 months ago

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


8 months ago

#19 @spacedmonkey
7 months ago

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.

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


6 months ago

#21 @TimothyBlynJacobs
6 months ago

I'm +1 to this approach.

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

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


5 months ago

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


4 months ago

#24 @rmccue
8 weeks ago

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
4 weeks ago

#25 @spacedmonkey
4 weeks ago

  • Keywords has-unit-tests added; needs-unit-tests removed

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.

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


4 weeks ago

#27 @TimothyBlynJacobs
3 weeks ago

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',
    ),
) );

#28 @kadamwhite
3 weeks ago

  • Milestone changed from Future Release to 5.3

#29 @kadamwhite
3 weeks ago

  • Keywords commit added
  • Owner changed from spacedmonkey to kadamwhite

@kadamwhite
3 weeks ago

PHPCBF

#30 @kadamwhite
3 weeks ago

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
3 weeks ago

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

#31 @kadamwhite
3 weeks ago

  • Keywords early added; commit removed
  • Milestone changed from 5.3 to 5.4
  • Owner changed from kadamwhite to spacedmonkey

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.

#32 @TimothyBlynJacobs
3 weeks ago

  • Keywords commit added; early removed
  • Milestone changed from 5.4 to 5.3

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.

#33 @TimothyBlynJacobs
3 weeks ago

  • Keywords early added; commit removed
  • Milestone changed from 5.3 to 5.4

Blurgh, accidentally undid the keyword changes.

Note: See TracTickets for help on using tickets.