WordPress.org

Make WordPress Core

Opened 19 months ago

Last modified 3 days ago

#38323 assigned enhancement

Reconsider $object_subtype handling in `register_meta()`

Reported by: flixos90 Owned by: flixos90
Milestone: 5.0 Priority: normal
Severity: normal Version:
Component: Options, Meta APIs Keywords: has-patch needs-unit-tests needs-dev-note
Focuses: rest-api Cc:

Description

While register_meta() was originally planned to support object subtypes (e.g. post types in the case of post meta, taxonomies in the case of term meta), this feature was dropped in 38095, mainly because it needed further thought.

I think we should continue to think how we can implement this feature. Many plugins use meta in ways that are specific to a subtype, so it doesn't make sense for them to use register_meta() now because it would mean that for example some e-commerce plugin's product data would suddenly be exposed on any post type. The current implementation can also lead to conflicts very easily: Imagine one plugin registers an email meta key for a custom post type, and another plugin registers an email meta key for another post type, and the metadata has a different structure - such thing can raise several issues.

I really like the new enhanced way register_meta() works, but without subtype handling it is still useless for many occasions. Therefore let's continue work on that function.

Attachments (11)

38323.diff (13.4 KB) - added by flixos90 19 months ago.
38323.2.diff (25.2 KB) - added by tharsheblows 17 months ago.
unit tests for custom post type meta
38323.3.diff (26.9 KB) - added by flixos90 16 months ago.
wp-object-type.diff (4.3 KB) - added by flixos90 14 months ago.
wp-object-type.2.diff (7.3 KB) - added by flixos90 14 months ago.
38323.4.diff (14.5 KB) - added by flixos90 6 months ago.
38323.5.diff (20.8 KB) - added by flixos90 4 weeks ago.
38323.6.diff (35.0 KB) - added by flixos90 2 weeks ago.
38323.7.register-meta.diff (28.7 KB) - added by flixos90 4 days ago.
actual changes to register_meta()
38323.7.wrappers.diff (6.4 KB) - added by flixos90 4 days ago.
wrappers for register_meta() ease of use
38323.8.register-meta.diff (37.8 KB) - added by tharsheblows 3 days ago.
with tests

Download all attachments as: .zip

Change History (69)

@flixos90
19 months ago

#1 @flixos90
19 months ago

  • Keywords has-patch needs-unit-tests 2nd-opinion added; needs-patch removed

I have worked on a patch that adds $object_subtype handling back in (see 38323.diff). It has of course many similarities to how things originally worked, but there are a few significant changes. Some notes about the approach:

  • One issue we previously ran into was that we would need to add the $object_subtype parameter to all the *_metadata() functions. The above patch introduces a new function get_object_subtype( $object_type, $object_id ) that automatically detects the subtype of an object. The function is used in several locations where we have these two parameters available in order to not require passing the object subtype.
  • The object subtype in this patch is not a required argument. That makes sense as there is meta that needs to be registered for an entire object type (think _thumbnail_id) and other meta that is specific to a subtype. The subtype being optional also makes using several functions easier (when using them for "object type-wide" meta). The only significant change this brings can be found in get_registered_metadata(): When retrieving all registered metadata for an object, we need to look at both meta registered for its general type and meta registered for its subtype. In the above patch the latter takes precedence in case there are meta keys registered for both variants (which shouldn't be done, but can be done). We might wanna think about a check when registering meta for a subtype that prevents addition if the meta is already registered for the overall type - let's see.
  • The $wp_meta_keys global structure has obviously changed (back to how it was before 38095) - is that okay in terms of BC? It shouldn't be accessed directly by a plugin anyway since there are functions available for everything I could imagine someone would wanna do with that data.

#2 @sc0ttkclark
19 months ago

+1, I see value in having $object_subtype support, however I don't have the energy to help push it forward or review patches at the moment. If I do get some time, I'll swing back around again unless this has already moved forward.

This ticket was mentioned in Slack in #core-fields by sc0ttkclark. View the logs.


19 months ago

#4 follow-up: @tharsheblows
17 months ago

I think this works already, at least for post types. What tripped me up originally was remembering to add 'supports' => array('custom-fields') when registering the post type -- also, register_meta() uses object types so custom post types are registered using post eg register_meta('post', 'meta_key', $args)

Attached are unit tests.

@tharsheblows
17 months ago

unit tests for custom post type meta

#5 in reply to: ↑ 4 @flixos90
17 months ago

Replying to tharsheblows:

I think this works already, at least for post types.

register_meta() currently works globally across all post types, but it does not support registering meta for individual post types, and that is what this ticket is about. The unit tests you provided are definitely helpful, but they test whether globally registered post meta (globally here means for all post types) works on a specific custom post type. I suppose we had a misunderstanding here, my ticket description should probably have been more clear.

#6 @tharsheblows
17 months ago

Your description was good, it was me! I misread it and conflated it with something else; it was totally down to my confusion. Thanks for being nice about it. :)

Just to make sure I've got it correct this time: the issues here arise if people accidentally register the same meta key, ie don't use unique meta key names. That makes sense.

When retrieving all registered metadata for an object, we need to look at both meta registered for its general type and meta registered for its subtype. In the above patch the latter takes precedence in case there are meta keys registered

This also makes sense to me, especially because someone using a non-unique meta key name is, I'd guess, more likely to use the easiest form of register_meta() and register it for the object type without a subtype.

Unregistering object subtypes has the potential to be confusing, unregister_meta_key() won't work quite as expected if someone tries to unregister a meta key for an individual object subtype and the meta key is also registered for the object type eg

 register_meta( 'post', 'key', array( 'show_in_rest' => true ) );
 register_meta( 'post', 'key', array( 'show_in_rest' => true, 'object_subtype' => 'cpt' ) );
 unregister_meta( 'post', 'key', 'cpt' );

will still allow the meta key "key" to show up in the REST API for posts which have a cpt custom post type.

Also if someone uses a non-unique meta key that someone else has used and registers it with 'show_in_rest'=>true then whatever it's duplicated will show up in the REST API response in whichever object subtype it's used.

To mitigate both of those, it would be nice to be able to explicitly whitelist meta keys for a given object subtype. Either a filter on eg get_registered_fields() or add it maybe like add_post_type_support('cpt', 'custom-fields', array('key', 'key1', 'key2') or when the post type is registered. It would be nice if $wp_meta_keys could be a reliable whitelist but I don't think it can for subtypes. (Is that right?)

I think the change in the structure of $wp_meta_keys could be an issue but that's only because it's there so people might be using it as they do.

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 by flixos90. View the logs.


16 months ago

#9 @kadamwhite
16 months ago

To mitigate both of those, it would be nice to be able to explicitly whitelist meta keys for a given object subtype. Either a filter on eg get_registered_fields() or add it maybe like add_post_type_support('cpt', 'custom-fields', array('key', 'key1', 'key2') or when the post type is registered.

I sympathize with this. However, I feel like I would expect unregister_meta to unregister all meta for a key, whether the less-specific value was shadowed or not.

After working a little bit more with meta within the REST API, the capability to restrict a registered meta value to a specific type of resource feels pretty critical if meta is to be used within the API. I'd love to talk through this one in slack this week.

#10 @kadamwhite
16 months ago

or when the post type is registered

Additionally, I think that specifying the type during register_meta feels better to me than specifying the meta when registering the type, since the type has to exist before register_meta is called, but you do not necessarily have to register_meta if you have a custom post type. That is to say, it doesn't make sense to me to say "register CPT with whitelisted keys; don't register any meta", whereas "register CPT with ability to have meta; don't register any meta" makes sense (because there's other non-register_meta ways to use custom fields).

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


16 months ago

@flixos90
16 months ago

#12 @flixos90
16 months ago

I worked on a new patch 38323.3.diff with an updated behavior taking the previous comments into account and works quite differently from the original patch.

The patch introduces a new class WP_Meta_Key that improves how the registered meta information is stored. It could possibly be used to get rid of the global $wp_meta_keys too (through a static property) although the current patch doesn't do this.

In the patch the $wp_meta_keys global is still indexed by object type and then by meta key, similar like it is now. However instead of storing the array of $args for each $meta_key, an instance of WP_Meta_Key is stored, that includes all registered information for that key, whether global for an entire object type or specific to an object subtype. The class also ensures that it is impossible to register a meta key for an object type if that meta key is already registered for that object type in any other way, so it eliminates the problem of possibly having a meta key being registered for both the entire object type and a specific subtype of that object type. When retrieving registered meta for a specific subtype though, the class methods automatically fall back to meta registered to the entire object type if meta for the subtype is not available.

A short note on the naming: I called each set of $args for a meta key a "definition set". I don't think it's a great name, but I didn't wanna spend time thinking about a better one for now, and this can easily be refactored later.

@kadamwhite Let's definitely chat on how to handle this topic. Regarding using register_meta() vs register_post_type(), I think register_meta() is a more suitable location to manage these. Also, I know it's probably a bit too early for a patch since we still need to figure out several things, but I was in the mood to try things out. :)

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


16 months ago

#14 @kadamwhite
16 months ago

  • Focuses rest-api added

#15 @kadamwhite
16 months ago

#39803 was marked as a duplicate.

#16 @swissspidy
16 months ago

The docs at https://developer.wordpress.org/rest-api/extending-the-rest-api/modifying-responses/ didn't mention the missing sub type handling but instead suggested using register_meta( 'my-cpt' …). I adjusted that now, but of course needs to be reverted once this feature lands.

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


16 months ago

#18 @westonruter
16 months ago

Just wanted to note that the Customize Posts plugin also wrestled with the problem of registering meta specifically to a dedicated post type. We came up with a register_post_type_meta method which takes three arguments: $post_type, $meta_key, $setting_args. The setting args includes the sanitize callback, default value, required capability, single vs multi, and other customizer-specific properties. Here is the method definition: https://github.com/xwp/wp-customize-posts/blob/f7b62df6744eb3eb54eb1aec9d877d51ee94cfff/php/class-wp-customize-posts.php#L230-L274

Hopefully this provides helpful real world usage patterns for an enhanced register_meta(), specifically here with the needs of the customizer in view.

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


15 months ago

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


14 months ago

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


14 months ago

#22 follow-up: @jnylen0
14 months ago

Some discussion in Slack today: https://wordpress.slack.com/archives/C02RQBWTW/p1491423937985603

The solution I like the best so far is the introduction of a new WP_Object_Type class (perhaps not the best name). This class would be a very simple pointer to one or more object types for which the meta key should be registered:

<?php
new WP_Object_Type( 'post' )
new WP_Object_Type( 'post', 'my-cpt-name' )
new WP_Object_Type( 'term' )
new WP_Object_Type( 'term', 'post_tag' )
new WP_Object_Type( 'comment' )
new WP_Object_Type( 'comment', 'pingback' )
new WP_Object_Type( 'user' )
// etc.

Then, register_meta could be updated to accept one of these objects as its first parameter, filling in the gap in functionality discussed in this ticket.

Another benefit of this approach is that register_rest_field could also receive the same update. Currently it's not possible to register a field for all post types at once; we could fill in that gap in functionality at the same time, as well as unifying the APIs for these two functions.

It was proposed to make WP_Object_Type a base class for WP_Post_Type and WP_Taxonomy. I disagree with this because the proposed class is not an object type itself, rather it is a pointer to one or more object types. Having a WP_Object_Type that can represent "all post types" is necessary, but it would break this encapsulation.

To make it clear that this is just a pointer to an object type, rather than a base for the implementation of object types, perhaps a better name for this class is WP_Object_Type_Filter.

There's an initial implementation over at 35658.17.diff. It's basically what I had in mind, but I think the constructor should use two different parameters ($general_type, $specific_type) for clarity and explicitness.

#23 in reply to: ↑ 22 @flixos90
14 months ago

Replying to jnylen0:

Some discussion in Slack today: https://wordpress.slack.com/archives/C02RQBWTW/p1491423937985603

The solution I like the best so far is the introduction of a new WP_Object_Type class (perhaps not the best name). This class would be a very simple pointer to one or more object types for which the meta key should be registered.

Then, register_meta could be updated to accept one of these objects as its first parameter, filling in the gap in functionality discussed in this ticket.

+1

It was proposed to make WP_Object_Type a base class for WP_Post_Type and WP_Taxonomy. I disagree with this because the proposed class is not an object type itself, rather it is a pointer to one or more object types. Having a WP_Object_Type that can represent "all post types" is necessary, but it would break this encapsulation.

I don't think this is a limitation. I think it makes sense to have WP_Object_Type as a non-abstract class that represents an object type. The WP_Post_Type and WP_Taxonomy (and other future subtype) classes can inherit that class and provide data to make them a representation of a subtype (as that's what they are). We have a chance to create a common ground for those classes, and I don't think it makes sense to add another standalone-class without integration that actually contains related functionality. Another advantage would be that you get the object subtype back when registering it (for example $post_type = register_post_type( ... )) so that you could then immediately register_meta() for that subtype.

wp-object-type.diff is a quick example of what I have in mind. Maybe it would even be more accurate to introduce a WP_Subtype class and inherit WP_Object_Type, and then WP_Post_Type and WP_Taxonomy could inherit WP_Subtype.

(Please not that all these names are probably not the best, the implementation is what we should focus on for now.)

#24 follow-up: @jnylen0
14 months ago

I'm still not really convinced that this inheritance chain is a useful abstraction:

  • Does WP_User also inherit from WP_Object_Type? Probably not, but doesn't this indicate that WP_Object_Type is not actually an ancestor class of all the things it describes?
  • What is the functionality that's being factored out into the base class? It needs to be common to as many object types as possible, at least the classes that inherit from it.
  • What if, in the future, we wanted to support registering meta for multiple object types at once (e.g. post of a certain type and user)? Even if we don't implement this right away, this seems like another place where this abstraction falls apart.

#25 in reply to: ↑ 24 @flixos90
14 months ago

Replying to jnylen0:

  • Does WP_User also inherit from WP_Object_Type? Probably not, but doesn't this indicate that WP_Object_Type is not actually an ancestor class of all the things it describes?

WP_User does not and should not, but if Core ever introduces a WP_User_Type or similar, that would in my understanding.

  • What is the functionality that's being factored out into the base class? It needs to be common to as many object types as possible, at least the classes that inherit from it.

WP_Post_Type and WP_Taxonomy are the only subtype classes that currently exist, and they already have several things in common that we could abstract out into WP_Object_Type (the constructor as well as the set_props(), add_hooks() and remove_hooks() methods for example) - however I think it should rather be a WP_Subtype class that would contain these methods (as mentioned in my previous comment as an alternative). Correcting my previous approach, I think WP_Object_Type should stand on its own as a pointer to an object type, but not a subtype. WP_Subtype should be an abstract base class that WP_Post_Type and WP_Taxonomy inherit; this addresses the concerns that an object type and subtype are something different. register_meta() can then just check whether it is a WP_Object_Type or WP_Subtype and then call the methods to determine which object type/subtype of object type.

  • What if, in the future, we wanted to support registering meta for multiple object types at once (e.g. post of a certain type and user)? Even if we don't implement this right away, this seems like another place where this abstraction falls apart.

I think we could add support for that by allowing an array of objects to be passed to register_meta() once we need it.

#26 @flixos90
14 months ago

wp-object-type.2.diff is a possible implementation for what I described above, it is much more clear than the my first ad-hoc implementation. Again, please don't put too much value on the names at this point.

The patch introduces:

  • WP_Object_Type class (represents a top-level object type, such as 'post', 'term', 'comment', 'user')
  • abstract WP_Object_Subtype class that must be inherited by a more specific class that determines the "parent object type" (represents a subtype of a specific object type, such as 'post', 'page', 'category', 'post_tag')
  • WP_Type interface used by WP_Object_Type and WP_Object_Subtype

register_meta() could then accept either of the above classes (both of which implement the WP_Type interface). That makes it possible to register meta for an entire object type, but also for specific subtypes.

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


13 months ago

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


12 months ago

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


11 months ago

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


10 months ago

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


9 months ago

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


9 months ago

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


8 months ago

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


8 months ago

@flixos90
6 months ago

#35 @flixos90
6 months ago

  • Milestone changed from Awaiting Review to 5.0

38323.4.diff is a refreshed patch based on 38323.diff. @kadamwhite and I recently revisited this ticket and agreed that, for the first iteration the patch should remain simple to focus on the functionality. We could still add a class structure to some of it at a later stage, if it makes sense.

Compared to the original patch the following things are changed:

  • The new function get_object_subtype() now uses logic that is currently part of map_meta_cap() already. I just noticed that while reviewing, so apparently this part of core still supports object subtypes already (maybe this was overlooked in the revert [38095]?). Anyway, that's good for us - I took the logic from there and put it in the new function, which as mentioned before is now also used by several other metadata functions that interact with an object ID. In addition, there's a new filter get_object_subtype_{$object_type} that allows devs to enhance the function for custom object types they might possibly have built (think about a custom database table where each object has an ID and the core metadata API should be used to extend it with arbitrary additional data).
  • The sanitize_{$object_type}_meta_{$meta_key} filter is not passed the subtype (as in the original patch), since it should only be used for sanitization that is independent of a subtype. We might consider adding that back in at some point (because it can have a purpose), but let's leave it out for now for simplicity's sake and since the other new filter already covers that use-case.
  • Version numbers have been updated to use 5.0.

We should start working on unit tests soon. The tests in 38323.2.diff do not actually test any subtype behavior, but they may very well be useful as a foundation for writing those ones that deal with the REST API part. We still need basic tests for register_meta() and the other related functions dealing with object subtypes.

Last edited 6 months ago by flixos90 (previous) (diff)

#36 @flixos90
6 months ago

One of the most important questions that we still need to answer is: How strict do we want to be about collisions? This was already discussed a short bit on the original ticket, particularly in https://core.trac.wordpress.org/ticket/35658#comment:127.

Example: If someone does register_meta( 'post, 'my_key', array() ) (i.e. for all post types) and someone else does register_meta( 'post', 'my_key', array( 'object_subtype' => 'my_post_type' ) ) (i.e. for the 'my_post_type' post type only), should the second one be prevented if the meta key is already registered on the entire object type? Or vice-versa? Or should it always be "whoever comes first"? I assume most use-cases for using register_meta() would actually deal with certain post types, not entire object types (like all post types), so it may be something that rarely happens.

Should we prevent one of the two above calls at all? Or should it be figured out whenever that key is used, by giving precedence to one or the other?

#37 @flixos90
6 months ago

Another idea: For ease of use, we might wanna introduce functions specific to object types, that could be used whenever someone wants to register data for a subtype. Such as:

  • register_post_meta( $post_type, $meta_key, $args )
  • register_term_meta( $taxonomy, $meta_key, $args )

These would be much more intuitive to use, particularly because of clarity in naming (everybody knows what $post_type and $taxonomy mean, opposite of the new term "subtype"). They could simply wrap register_meta() though, passing the $post_type / $taxonomy as subtype in the arguments array of it.

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


4 months ago

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


4 months ago

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


5 weeks ago

@flixos90
4 weeks ago

#41 @flixos90
4 weeks ago

  • Keywords 2nd-opinion removed
  • Owner set to flixos90
  • Status changed from new to assigned

38323.5.diff is a comprehensive update on the patch, which we should iterate on from now on. Here is what it does:

  • Add an object_subtype argument to the $args used in register_meta(). This can be a post type (in case $object_type is 'post') or a taxonomy (in case $object_type is 'term'). For comments and users it can be ignored. Aside: While comments have comment types, they are used in a completely different way that doesn't apply here (for example there is are no REST controllers per comment type, there is just one global comments controller).
  • Restructure $wp_meta_keys global so that registered meta keys are nested under their object type and object subtype. The latter can be an empty string for meta keys registered on the entire object type.
  • Add optional $object_subtype parameters to all related metadata functions that need it. By default it is an empty string in which case only meta registered on the entire object type is considered.
  • Introduce a get_object_subtype() function which retrieves the subtype that needs to be used based on a given object type and object ID of that type. For example, if the object type is 'post', the post type is returned. A filter get_object_subtype_{$object_type} can be used by plugin developers who add their own database tables with accompanying metadata to define similar behavior for their content.
  • When looking at registered metadata in a context where both object type and subtype are available, metadata registered specifically for the subtype takes precedence over more general metadata for the entire object type. However, such "conflicts" are generally an undesired scenario, so we need to educate developers and possibly implement restrictions.
  • Add support for object subtypes to the REST API meta classes.
  • Adjust existing tests that directly look at the $wp_meta_keys global to account for the restructuring of that object.

Things that we need to discuss:

  • How do we deal with possible conflicts (i.e. when someone registers a meta key for an object type without specifying an object subtype and someone else registers the same meta key for the same object type, but with a subtype)?
  • Should we make use of comment types (i.e. allow meta to be registered specifically for pingbacks, trackbacks and regular comments) or keep it simple and not use subtypes for comments?
  • Should we introduce wrapper functions to make the concept of subtypes more obvious (such as register_post_meta( $post_type, $meta_key, $args ), register_term_meta( $taxonomy, $meta_key, $args ) etc.) and recommend to use those?

I will publish a post on make.wordpress.org/core/ later this week with a few more details on the issue itself and the discussion points that we still need to figure out. We will then have a REST API meeting dedicated to it next week (on May 3rd).

@flixos90
2 weeks ago

#42 @flixos90
2 weeks ago

38323.6.diff implements the decision (and suggestions) we made in the REST API chat on May 3rd. These are the changes compared to the previous patch:

  • Meta keys registered for a specific object type and object subtype now overrule meta keys only registered for a specific object type. That means: If you call register_meta( 'post', 'mykey', array( ... ) ), the registration data from that call will determine the behavior of the key on every post of any post type. However, if you also have register_meta( 'post', 'mykey', array( 'object_subtype' => 'post', ... ) somewhere else, the registration data from that call will determine the behavior on the posts of the 'post' post type, overriding the other, less-specific registration. For all other post types, the behavior will still be based on the registration data for the entire object type. So registration without specifying an object subtype essentially acts as a fallback. That specifically means:
    • A new auth_{$object_type}_meta_{$meta_key}_for_{$object_subtype} filter is introduced and called in map_meta_cap() for capability checks related to metadata. If a function is hooked into that filter, that filter will fire. Otherwise, the previously existing, less-specific auth_{$object_type}_meta_{$meta_key} will fire. To clarify, they are exclusive to each other, and only one of them will fire for the same check. This must be clearly highlighted in the dev-note. Since conflicts should be rare, most code will continue to work, but it will be recommended to switch to registering for a specific subtype.
    • The auth_{$object_type}_{$object_subtype}_meta_{$meta_key} filter which appears to be an accidental remainder of the original plans from #35658 is now deprecated, to reduce the chance for conflicts. The name of the new filter is more accurate and prevents weird edge-cases where the filter name for an object type "post" and subtype "post" could theoretically conflict with the filter name for an object type called "post_post" (bad example, but you get the gist).
    • A new sanitize_{$object_type}_meta_{$meta_key}_for_{$object_subtype} filter is introduced in sanitize_meta(). Similar to the auth filters, if that filter is being used, it will fire, and only otherwise the previously existing, less-specific sanitize_{$object_type}_meta_{$meta_key} will fire. This as well must be clearly highlighted in the dev-note.
    • The flow of determining the capabilities for metadata capabilities in map_meta_cap() was adjusted to be easier-readable. First of all the default value for a given meta key is determined by calling is_protected_meta(). Then, it is filtered through either auth_{$object_type}_meta_{$meta_key}_for_{$object_subtype} or auth_{$object_type}_meta_{$meta_key}. Then, it is further filtered through the deprecated auth_{$object_type}_{$object_subtype}_meta_{$meta_key} (only if still being used, which is unlikely due to the relatively short existence of this filter). After that, we have the final result whether to allow or disallow the action.
    • In the REST API, the behavior of a meta key is based on whether it is registered for the current object type and subtype. Only if there is no such registration present, it falls back to a registration for the object type without subtype (if available).
  • Easy-to-understand utility functions wrapping around register_meta() and unregister_meta_key() have been introduced, similar like we have functions like get_post_meta(), get_term_meta() etc. The new functions are:
    • register_post_meta( $post_type, $meta_key, $args )
    • unregister_post_meta( $post_type, $meta_key )
    • register_term_meta( $taxonomy, $meta_key, $args )
    • unregister_term_meta( $taxonomy, $meta_key )
    • register_comment_meta( $comment_type, $meta_key, $args )
    • unregister_comment_meta( $comment_type, $meta_key )
    • register_user_meta( $user_type, $meta_key, $args )
    • unregister_user_meta( $user_type, $meta_key )

We may wanna discuss the signature of these functions. Some explanation on my approach here: I put the subtype parameter first, as we wanna encourage developers to register their metadata per subtype (i.e. make it a required parameter). While comments and users don't really have subtypes at this point, I still think we should be consistent in all these functions, and by that also keep them future-proof (if we decide to make actual use of comment types or introduce user types). Furthermore, in each of the functions it is documented that an empty string may be passed as first parameter to ignore the subtype and still use the functions for the entire object type - so they still allow for full flexibility.

#43 @flixos90
2 weeks ago

  • Keywords needs-dev-note added

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


12 days ago

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


12 days ago

#46 follow-up: @johnjamesjacoby
12 days ago

I don’t like the “subtype” naming:

  • it doesn’t exist anywhere in the database schema or code
  • it conveys there is only 1 additional layer of variation
  • we would not want subsubsub types
  • it’s too ambiguous - what a “subtype” means for a post is different than a taxonomy and a user and a comment and so on

I’m +1 that register_meta needs a way to understand object variations, but I’m -1 on all of the current suggested approaches. They are too limiting, and only serve very specific purposes.

I would much prefer a way to provide an array of arguments that get parsed and compared against the object, making the “subtype” more of a set of properties than just a hard-coded restriction.

For example, I want to register meta that’s only for posts of a certain type and status. If the type and status are not a match, then don’t do anything.

This is bigger than types or subtypes. This is a meta variation, or a meta model, or a meta map. I’m not sure exactly on what it should be called, but subtypes locks us into it being a omething it shouldn’t be.

#47 @Shelob9
12 days ago

I wanted to cross reference this Gutenberg issue. The schema for block attributes supports a default. If a block attribute uses meta storage, Gutenberg relies on the meta API for the default, which would be perfect if register_meta() supported the default argument like block attributes and REST API endpoint args do.

https://github.com/WordPress/gutenberg/issues/4494

#48 in reply to: ↑ 46 @flixos90
8 days ago

Replying to johnjamesjacoby:

I don’t like the “subtype” naming:

  • it doesn’t exist anywhere in the database schema or code
  • it conveys there is only 1 additional layer of variation
  • we would not want subsubsub types
  • it’s too ambiguous - what a “subtype” means for a post is different than a taxonomy and a user and a comment and so on

While I agree that object subtypes are a rather abstract concept (and thus possibly ambiguous), so are object types, and in scope of how WordPress is used and has been used for years, basing that abstraction on sub-types makes sense because a post's / term's behavior for example is substantially different based on its type/taxonomy - not based on its status, author or other field. In the same way, there are REST API controllers per post type/taxonomy. Then, since 'post' itself is an object type, that makes a post type an object subtype.

Object subtypes do not exist in the database schema, but that is a flaw in the database schema, given that metadata is largely used based on them (metadata for all posts of a specific post type, not for all posts of all post types). Furthermore, object subtypes do already exist in code, they are used in map_meta_cap().

To me the most important point though is that we are introducing wrapper functions for register_meta() which "clarifies" how to use the function. It should be perfectly clear what for example register_post_meta( $post_type, $meta_key, $args ) does. While we are using an abstract concept in subtypes (which IMHO is just coherent with how abstract the Meta API itself is), the wrapper functions make it obvious how to use it.

This is bigger than types or subtypes. This is a meta variation, or a meta model, or a meta map. I’m not sure exactly on what it should be called, but subtypes locks us into it being a omething it shouldn’t be.

I don't disagree that we could introduce more possibilities of "categorization" in that regard in the future. However, at this point we don't want to further overcomplicate solving the immediate problem.

Replying to Shelob9:

I wanted to cross reference this Gutenberg issue. The schema for block attributes supports a default.

We've already briefly talked about this in a multisite meeting, default values for metadata make complete sense. It's great to hear that there's already a Gutenberg use-case for #43941. We are planning to tackle this ticket once this one here is done, both should be in 5.0.

#49 follow-up: @johnjamesjacoby
8 days ago

I don’t need a re-explanation of the concepts being proposed; it’s in the ticket, and I understand what you’re hoping to do and why.

It is not a good idea to reuse terms. Types and subtypes are being specced out as names for things that have nothing to do with actual types or subtypes, which have been part of all programming languages for longer than we’ve been alive to use them.

Code in WordPress shouldn’t be confusing on purpose because it’s easy, and it definitely shouldn’t get more confusing because it’s already kinda confusing.

After adding clarity to multisite-specific code, why lobby for confusing code here? And I disagree that making this feature more useful will take more time. Parsing array arguments isn’t any harder than comparing strings.

Here’s one link to one PHP developer talking about types and subtypes without thinking of WordPress: https://www.stitcher.io/blog/what-php-can-be

The only way to fully move forward with the types and subtypes literally being talked about here is to invent a base WP_Type and WP_Sub_Type class that define what registered types and subtypes are, so taxonomies and post types and such can be connected to them.

Guessing at what core object types are, and not letting plugins declare their own object types and subtypes, leaves this fragile and unfinished. Just squatting a slug and hoping nothing collides was necessary ten years ago, and everyone has worked hard to code away from that idea.

Not to mention that these aren’t taxonomy subtypes, these are term subtypes. Terms are already a subtype of their taxonomy.

Types and subtypes are not good names for what’s being recommended here, unless we fill in all of the other gaps I mentioned above.

#50 in reply to: ↑ 49 @flixos90
8 days ago

Replying to johnjamesjacoby:

It is not a good idea to reuse terms. Types and subtypes are being specced out as names for things that have nothing to do with actual types or subtypes, which have been part of all programming languages for longer than we’ve been alive to use them.

The full terms are "object types" and "object subtypes", and at least the first are not a new concept in core. The latter are also found in one location in core. But if it's these names feel off to you, we can think about alternatives. Content types and content subtypes? Meta types (this is the current name in the metadata functions) and meta subtypes?

Code in WordPress shouldn’t be confusing on purpose because it’s easy, and it definitely shouldn’t get more confusing because it’s already kinda confusing.

Definitely. Which of the ideas do you find confusing?

The only way to fully move forward with the types and subtypes literally being talked about here is to invent a base WP_Type and WP_Sub_Type class that define what registered types and subtypes are, so taxonomies and post types and such can be connected to them.

I personally like this idea and it was discussed before (see https://core.trac.wordpress.org/ticket/38323#comment:22), but later not pursued. On the other hand, such classes might clarify the concepts, but they aren't a necessity for the first iteration if we have a clear definition for these concepts anyway and document it.

Guessing at what core object types are, and not letting plugins declare their own object types and subtypes, leaves this fragile and unfinished. Just squatting a slug and hoping nothing collides was necessary ten years ago, and everyone has worked hard to code away from that idea.

Plugins can add their own object types, for example by introducing their own database table. When they connect a custom metadata table to it, the metadata functions need to be called with the respective object type that identifies the table name, but also affects the hooks fired. For that very purpose, there is a filter in get_object_subtype() so that such a custom object type can also take care of defining how a subtype for it is detected on an entity.

Not to mention that these aren’t taxonomy subtypes, these are term subtypes. Terms are already a subtype of their taxonomy.

In my understanding, terms are entities of the object type "term", and their behavior is further specified by the object subtype "taxonomy".

Types and subtypes are not good names for what’s being recommended here, unless we fill in all of the other gaps I mentioned above.

As I mentioned in the beginning of this comment, please suggest alternative naming ideas from "object type" and "object subtype". I personally don't see the "type" and "subtype" parts of the name problematic. Maybe we should rather focus on the "object" part of the name. In the current meanings, the core object types are 'post', 'term', 'comment' and 'user', each of which is defined by the fact that its entities are identified by a unique ID and they support meta. In multisite, 'blog' and 'site' (old naming conventions here because tied to DB) are also object types, for the same reason. Since this is all connected to metadata, maybe the names should be "meta type" and "meta subtype". Just thinking out loud here.

#51 @kadamwhite
6 days ago

I want to make sure that we get this naming right, so I've mentioned this discussion in the meeting minutes I just posted and we'll try to get a broader set of eyes on this ticket at the core meeting today, too. However, I will also be trying to cut off debate around the naming at a reasonable point so that we don't end up blocking the needed functionality changes this patch seeks to introduce.

Regarding the wrapper functions @flixos90, I'm actually now changing my mind around register_user_meta and register_comment_meta: I'm feeling now that we don't need these wrappers. I'd rather introduce the wrappers for the types where they are essential, rather than introduce new methods that may never end up utilized, especially if it causes us to over-think the (otherwise pretty clear) function signature of the new wrappers we know we need.

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


6 days ago

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


5 days ago

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


4 days ago

#55 @kadamwhite
4 days ago

Per discussion in Slack today, we have come to an agreement on the discussion between @johnjamesjacoby and @flixos90 detailed above -- these are valid concerns to raise but many of them exceed the scope of this ticket, and we will continue with the implementation as described in the most recent patch.

The only remaining debate is whether the "object subtype" (post_type or taxonomy) parameter in the wrapper methods should come first or last, and whether we should omit the wrappers for comment and user meta for the time being. I am soliciting an outside opinion from other contributors because all of us involved in the discussion today are likely too close to the issue to have an objective viewpoint :)

@flixos90
4 days ago

actual changes to register_meta()

@flixos90
4 days ago

wrappers for register_meta() ease of use

#56 follow-up: @flixos90
4 days ago

As discussed in yesterday's chat, 38323.7.register-meta.diff is an update that only contains the actual changes to register_meta() and the surrounding infrastructure. 38323.7.wrappers.diff contains the wrapper functions (the latter patch requires the first one to work).

This allows for a better overview:

  • The first patch is now pretty much final and only needs unit tests and a review.
  • The second patch is what is still up for debate. In the current state, it simply adds all wrapper functions with subtype as first parameter - which is likely not what we want, since comments and users don't have subtypes. But we can go from there and tweak the patch after we have come to a decision.

Going forward, please iterate on the patches separately.

#57 in reply to: ↑ 56 @ocean90
4 days ago

Replying to flixos90:

since comments and users don't have subtypes.

Don't we have trackback and pingback or any other $comment->comment_type as subtypes for comments?

Last edited 4 days ago by ocean90 (previous) (diff)

@tharsheblows
3 days ago

with tests

#58 @tharsheblows
3 days ago

This adds in some tests for the patch which extends register_meta to include subtypes. I've only done them for posts and terms as additional tests don't add anything for comments and users.

For comments, although there are comment subtypes, there's nothing like register_comment_type which can be used to add proper REST API support for custom comment types. There's discussion of that in #35214 but I think probably beyond the scope here.

One thing I noticed doing them was that because register_meta for comments and users doesn't change at all, the suggestion by @helen on Slack seems to me to make sense, ie wrapper functions now for the objects which have subtypes and adding them in later for the others when they get subtypes.

Note: See TracTickets for help on using tickets.