WordPress.org

Make WordPress Core

Opened 17 months ago

Last modified 21 hours ago

#43941 assigned enhancement

Add default value to register meta

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

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 (5)

meta_default.diff (1.8 KB) - added by spacedmonkey 17 months ago.
43941.2.diff (2.5 KB) - added by spacedmonkey 15 months ago.
43941.diff (5.2 KB) - added by spacedmonkey 15 months ago.
43941.3.diff (3.7 KB) - added by spacedmonkey 6 months ago.
43941.4.diff (10.5 KB) - added by spacedmonkey 21 hours ago.

Download all attachments as: .zip

Change History (30)

#1 @flixos90
16 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.


16 months ago

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


15 months ago

#4 @spacedmonkey
15 months ago

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

Last edited 15 months ago by spacedmonkey (previous) (diff)

#5 @flixos90
15 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
15 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
15 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
15 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
15 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
15 months ago

#10 @spacedmonkey
15 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.


15 months ago

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


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


11 months ago

#15 @danielbachhuber
11 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.


9 months ago

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


7 months ago

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


7 months ago

#19 @spacedmonkey
6 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
5 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.


4 months ago

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


3 months ago

#24 @rmccue
4 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.

#25 @spacedmonkey
21 hours 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.

Note: See TracTickets for help on using tickets.