WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 3 weeks ago

Last modified 2 days ago

#43941 closed enhancement (fixed)

Add default value to register meta

Reported by: spacedmonkey Owned by: TimothyBlynJacobs
Milestone: 5.5 Priority: normal
Severity: normal Version: 4.6
Component: Options, Meta APIs Keywords: has-patch has-unit-tests has-dev-note
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 (18)

meta_default.diff (1.8 KB) - added by spacedmonkey 2 years ago.
43941.2.diff (2.5 KB) - added by spacedmonkey 2 years ago.
43941.diff (5.2 KB) - added by spacedmonkey 2 years ago.
43941.3.diff (3.7 KB) - added by spacedmonkey 17 months ago.
43941.4.diff (10.5 KB) - added by spacedmonkey 11 months ago.
43941.5.diff (10.8 KB) - added by TimothyBlynJacobs 11 months ago.
43941.6.diff (11.6 KB) - added by kadamwhite 11 months ago.
PHPCBF
43941.7.diff (9.2 KB) - added by kadamwhite 11 months ago.
Remove declaration of "null" default values from meta test registration arrays
43941.8.diff (8.9 KB) - added by spacedmonkey 7 months ago.
43941.9.diff (8.9 KB) - added by spacedmonkey 7 months ago.
43941.10.diff (11.7 KB) - added by spacedmonkey 3 months ago.
43941.11.diff (12.5 KB) - added by spacedmonkey 3 months ago.
43941.12.diff (13.1 KB) - added by spacedmonkey 3 months ago.
43941.13.diff (13.2 KB) - added by spacedmonkey 3 months ago.
43941.14.diff (13.4 KB) - added by spacedmonkey 2 months ago.
43941.15.diff (13.6 KB) - added by spacedmonkey 2 months ago.
43941.16.diff (13.6 KB) - added by spacedmonkey 2 months ago.
43941.17.diff (5.7 KB) - added by SergeyBiryukov 4 weeks ago.

Download all attachments as: .zip

Change History (112)

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


2 years ago

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


2 years ago

#4 @spacedmonkey
2 years 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

Version 1, edited 2 years ago by spacedmonkey (previous) (next) (diff)

@spacedmonkey
2 years ago

#5 @flixos90
2 years 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
2 years 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
2 years 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
2 years 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
2 years 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
2 years ago

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


2 years ago

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


2 years ago

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


2 years ago

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


22 months ago

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


20 months ago

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


18 months ago

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


18 months ago

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


16 months ago

#21 @TimothyBlynJacobs
16 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.


15 months ago

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


14 months ago

#24 @rmccue
12 months 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
11 months 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.


11 months ago

#27 @TimothyBlynJacobs
11 months 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
11 months ago

  • Milestone changed from Future Release to 5.3

#29 @kadamwhite
11 months ago

  • Keywords commit added
  • Owner changed from spacedmonkey to kadamwhite

@kadamwhite
11 months ago

PHPCBF

#30 @kadamwhite
11 months 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
11 months ago

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

#31 @kadamwhite
11 months 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
11 months 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
11 months ago

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

Blurgh, accidentally undid the keyword changes.

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


7 months ago

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


7 months ago

#36 @audrasjb
7 months ago

  • Keywords needs-dev-note added

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


6 months ago

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


6 months ago

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


6 months ago

#40 @davidbaumwald
6 months ago

  • Keywords needs-refresh added
  • Milestone changed from 5.4 to Future Release

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.

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


5 months ago

#42 @mnelson4
5 months ago

@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

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


5 months ago

#44 @kadamwhite
5 months ago

  • Milestone changed from Future Release to 5.5

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


4 months ago

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


4 months ago

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


3 months ago

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


3 months ago

#49 @spacedmonkey
3 months ago

  • Keywords needs-refresh removed

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.

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


3 months ago

This ticket was mentioned in PR #255 on WordPress/wordpress-develop by spacedmonkey.


3 months ago

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

This ticket was mentioned in PR #256 on WordPress/wordpress-develop by spacedmonkey.


3 months ago

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

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


3 months ago

#54 @spacedmonkey
3 months ago

  • Owner changed from spacedmonkey to johnbillion
  • Status changed from assigned to reviewing

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


3 months ago

#56 @johnbillion
3 months ago

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.

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


3 months ago

#58 @spacedmonkey
3 months ago

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 ?

#59 @spacedmonkey
3 months ago

Updated patch to fix issue with unit test failing.

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


2 months ago

#61 @spacedmonkey
2 months ago

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.

#62 @spacedmonkey
2 months ago

New patch 43941.16.diff.

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

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


2 months ago

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


2 months ago

#65 @davidbaumwald
6 weeks ago

  • Milestone changed from 5.5 to Future Release

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.

#66 @prbot
6 weeks ago

TimothyBJacobs commented on PR #256:

@spacedmonkey can you rebase this?

#67 @prbot
6 weeks ago

spacedmonkey commented on PR #256:

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

#68 @spacedmonkey
6 weeks ago

  • Milestone changed from Future Release to 5.5
  • Owner changed from johnbillion to TimothyBlynJacobs

#69 @spacedmonkey
5 weeks ago

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.

#70 @prbot
5 weeks ago

TimothyBJacobs commented on PR #256:

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

#71 @prbot
5 weeks ago

spacedmonkey commented on PR #256:

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 ?

#72 @davidbaumwald
5 weeks ago

  • Keywords early removed

#73 @prbot
5 weeks ago

TimothyBJacobs commented on PR #256:

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

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

#74 @TimothyBlynJacobs
5 weeks ago

  • Milestone changed from 5.5 to 5.6

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

#75 @prbot
5 weeks ago

TimothyBJacobs commented on PR #256:

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.

#76 @TimothyBlynJacobs
5 weeks ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

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.

#77 @TimothyBlynJacobs
5 weeks ago

  • Milestone changed from 5.6 to 5.5

#78 @TimothyBlynJacobs
5 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

#79 @SergeyBiryukov
4 weeks ago

In 48411:

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

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

See #49572, #43941, #45464.

#80 @SergeyBiryukov
4 weeks ago

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.

#81 @spacedmonkey
4 weeks ago

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

#82 @johnjamesjacoby
4 weeks ago

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 ) {

#83 @TimothyBlynJacobs
4 weeks ago

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.

#84 @spacedmonkey
4 weeks ago

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 )

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


3 weeks ago

#86 @TimothyBlynJacobs
3 weeks ago

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.

This ticket was mentioned in PR #409 on WordPress/wordpress-develop by spacedmonkey.


3 weeks ago

Change order of param on get_metadata_default function.

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

#88 @spacedmonkey
3 weeks ago

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

Last edited 3 weeks ago by spacedmonkey (previous) (diff)

#89 @SergeyBiryukov
3 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

#90 @SergeyBiryukov
3 weeks ago

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.

#91 @spacedmonkey
3 weeks ago

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

#92 @SergeyBiryukov
3 weeks ago

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.

This ticket was mentioned in Slack in #core by justinahinon. View the logs.


10 days ago

#94 @justinahinon
2 days ago

  • Keywords has-dev-note added; needs-dev-note removed
Note: See TracTickets for help on using tickets.