Reconsider $object_subtype handling in `register_meta()`

[flixos90]

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.

[flixos90]

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.

[sc0ttkclark]

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

[tharsheblows]

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]

unit tests for custom post type meta

[flixos90]

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.

[tharsheblows]

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.

[kadamwhite]

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.

[kadamwhite]

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

[flixos90]

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. :)

[kadamwhite]

#39803 was marked as a duplicate.

[swissspidy]

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.

[westonruter]

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.

[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:

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

[flixos90]

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

[jnylen0]

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.

[flixos90]

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.

[flixos90]

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.

[flixos90]

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.

[flixos90]

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?

[flixos90]

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.

[flixos90]

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]

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.

[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

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.

[Shelob9]

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

[flixos90]

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.

[johnjamesjacoby]

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.

[flixos90]

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.

[kadamwhite]

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.

[kadamwhite]

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]

actual changes to register_meta()

[flixos90]

wrappers for register_meta() ease of use

[flixos90]

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.

[ocean90]

Replying to flixos90:

since comments and users don't have subtypes.

Don't we have trackback and pingback as subtypes for comments?

[tharsheblows]

with tests

[tharsheblows]

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.

[spacedmonkey]

Related: #44238 #43941

[tharsheblows]

This patch recombines the post and term wrappers with the extended register_meta function and includes tests with duplicate tests for the wrapper functions – all as discussed (I think) in last week's REST API weekly meeting on Slack starting here: ​https://wordpress.slack.com/archives/C02RQC26G/p1527182057000169

[tharsheblows]

post and term wrappers added back in with tests

[flixos90]

Thanks a lot for writing all these tests @tharsheblows!

I reviewed and made a few tweaks in 38323.10.diff​:

• Always return 'comment' as subtype for comments in get_object_subtype(), as we decided to not use comment subtypes at this point.
• Add ticket annotations to tests.
• Re-add test_register_meta_with_term_object_type_populates_wp_meta_keys() test that was unnecessarily removed.
• Remove some duplicate test code.
• Simplify tests for register_meta() and make them more comprehensive by using data providers.
• Remove some unnecessary cleanup code from some tests.
• Add further tests, particularly for other meta registration functions and subtypes.
• Remove duplicate tests for the wrapper functions. It is an unnecessary maintenance burden to have the same tests run twice, and to verify the wrapper functions work correctly, we only need to ensure that register_meta() / unregister_meta_key() are called correctly by them.
• Improve post meta test setup by only creating the necessary posts and user once.
• Fix array item alignments in REST post meta tests.

This appears to be in a solid state and very close to commit. I'm going to have a closer look at the REST API tests soon, as I didn't get to those yet.

[kadamwhite]

Thank you @flixos90, this is looking good. As discussed in Slack today I will review, and barring any major Q's should commit this in the coming week! Even if there's any questions that arise I suspect we'll be able to close this out by WordCamp Europe :)

[jeremyfelt]

I haven't had a chance to test this out in full yet, but the direction and patch are looking nice. :thumbsup:

One thing that caught my eye is how many line changes there are in existing tests. Most (almost all?) of this seems like it can be done without the object subtype handling (e.g. the conversion to static $author, $post_id, $post_id_2 throughout). I would commit that first to better convey what is actually changing in the tests vs being added as new in the introduction of subtype handling.

[spacedmonkey]

So I have been looking at the patch and I have some issues with get_object_subtype function as it stands.

I think we should return the comment_types back in as sub type. This is how the code currently works in capabilities.php and see is no good reason to change it and break any existing filters. There is a big difference between a comment, pingback and trackback. That is value is having that as a sub type.

The new filter $object_subtype = apply_filters( "get_object_subtype_{$object_type}", $object_subtype, $object_id ); should be moved to the return value and not just be there for the default. All subtypes should be filterable. Also, should sites (blogs) meta be in this switch statement?

I also have some worries about performance, as every time subtype is loaded, it now loads the parent object is it related. If post meta for example was loaded, outside of a WP_Query, it would do a single query (if the user is not using object caching) to post object for every different id. I have just loaded post meta outside of WP_Query, becuase I didnt want to overhead of getting the whole object. But now it is loading the whole object. I would like to see some profiling data before merge.

There are also contexts where the object id is not easily to hand and so loading the main object is impossible. Take is_protected_meta, where the type isnt always passed, as it isn't always known. This ticket effects #44238 which adds protected to the register meta and makes that patch much harder. It also effects #43941 of adding a meta default value. We should talk about these tickets here and create a roadmap for where we see register_meta going in the future.

[flixos90]

Replying to spacedmonkey:

I think we should return the comment_types back in as sub type. This is how the code currently works in capabilities.php and see is no good reason to change it and break any existing filters. There is a big difference between a comment, pingback and trackback. That is value is having that as a sub type.

I generally would agree with this for the sake of BC, but since the REST API doesn't consider comment types at all and since it is generally not really defined what comment types are, I think we should keep them out for now. The vast majority of developers has no idea that comment types are a thing (contrary to post types and taxonomies), so I doubt the filter is used in that way, also given that it has not been around for a long time and been undocumented, as it was an accidental remainder of #35658. There is no benefit from supporting comment subtypes at this point.

The new filter $object_subtype = apply_filters( "get_object_subtype_{$object_type}", $object_subtype, $object_id ); should be moved to the return value and not just be there for the default. All subtypes should be filterable. Also, should sites (blogs) meta be in this switch statement?

I agree with this, let's move the filter to work for every return value. I have thought about site meta here too, but I'd prefer if we add support for that in a follow-up ticket after this is merged.

I also have some worries about performance, as every time subtype is loaded, it now loads the parent object is it related. If post meta for example was loaded, outside of a WP_Query, it would do a single query (if the user is not using object caching) to post object for every different id. I have just loaded post meta outside of WP_Query, becuase I didnt want to overhead of getting the whole object. But now it is loading the whole object. I would like to see some profiling data before merge.

To what functionality exactly are you referring? If it's about getting the parent object in get_object_subtype(), I don't think that is an issue. When a query is used, all objects are already in cache anyway. And when a singular result is accessed, this applies as well.

[flixos90]

In 43339:

Tests: Improve performance of post meta tests.

See #38323.

[spacedmonkey]

My worry is about loading the main object in get_object_subtype. There have been times where I have not loaded meta using a list of ids (from say the options table) and haven't run the normal query class, like WP_Query. For custom migration code, where the whole object is not required is an example. This change may have massive performance effects on high end users, this change will not be massively sign posted and well documented.

I disagree with the comment about comment types, as core supports pingback and trackbacks, which are well known. I also don't understand why to limit the functionality? You are already loading the comment.

[flixos90]

In 43340:

REST API: Improve test coverage by providing tests for term meta.

See #38323.

[flixos90]

Replying to spacedmonkey:

My worry is about loading the main object in get_object_subtype. There have been times where I have not loaded meta using a list of ids (from say the options table) and haven't run the normal query class, like WP_Query. For custom migration code, where the whole object is not required is an example. This change may have massive performance effects on high end users, this change will not be massively sign posted and well documented.

I don't think we should take this into account, for the following reasons:

• When reading metadata, this only matters if you use the new function get_registered_metadata(), which is not commonly used by plugins at this point. get_metadata() (plus the wrappers) stays unaffected.
• When writing metadata, the function is called, but at this point you are already making DB queries, usually many at the same time (since metadata values are always updated one by one). One extra query here doesn't do significant harm, furthermore in most cases there won't even be the extra query since the current object is usually loaded already.

I disagree with the comment about comment types, as core supports pingback and trackbacks, which are well known. I also don't understand why to limit the functionality? You are already loading the comment.

People know that pingbacks and trackbacks exist, that's true for sure, but people don't commonly know the term "comment type" or what that is. To be honest, WordPress core doesn't fully know what it is itself, that's why I think we shouldn't include them. The REST API ignores comment types completely for that very reason.

[flixos90]

38323.11.diff​ is an updated patch, with the following adjustments:

• Move the filter in get_object_subtype() to fire at the very end, in order to always filter the returned value.
• Simplify tests for subtype post meta in the REST API by using data providers.
• Add tests for subtype term meta in the REST API.

I think this is good to go in. Handing it over to you @kadamwhite for final review. We can also discuss at WCEU as necessary.

[kadamwhite]

After @spacedmonkey raised performance concerns at WCEU contributor day about the way we retrieve and throw away an entire post object in get_object_subtype, 38323.12.diff streamlines the way we are looking up post type information to avoid this.

38323.12.diff also reinstates support for returning comment_type, when available, after further conversation with @flixos90 @rmccue and others.

[kadamwhite]

Improve performance of object subtype lookup, and reinstate comment_type support

[spacedmonkey]

In 38323.13.diff there are the following changes.

• Added blog to get_object_subtype as site meta is now in core.
• Added new sub_type param to is_protected_meta See #44238 for background.
• Remove comment type, as the comment rest endpoint controller doesn't support passing comment type as this time.
• Remove array type hint from function call, as not WordPress coding standards.

[spacedmonkey]

38323.14.diff

Fixed the unit tests running. In get_object_subtype was not passing object_id to get_post_type. Fixed now.

Also defaulted is_protected_meta to sub type to empty thing, like get_object_subtype.

Fixed formatting.

[flixos90]

Replying to spacedmonkey:

• Added blog to get_object_subtype as site meta is now in core.

As discussed before, this should happen in a separate follow-up ticket. It needs further thought in terms of how to handle the naming blog vs site, and this is a separate issue from general subtype handling, which is the focus of this ticket. I created #44387 for it.

• Added new sub_type param to is_protected_meta See #44238 for background.

These changes should happen in #44238, not in here.

• Remove comment type, as the comment rest endpoint controller doesn't support passing comment type as this time.

At WCEU we decided to use comment types. I'm not sure why you removed them again, we didn't have any discussion in that regard. If you have concerns about comment subtypes, please let's discuss this in the REST API meeting this week. cc @kadamwhite

• Remove array type hint from function call, as not WordPress coding standards.

This is not a violation of the coding standards. Array is supported as a type hint for 5.2 and thus should be used as applicable. It is used already in other areas of core.

[flixos90]

38323.15.diff​ removes those changes that were made after 38323.12.diff​ and were actually part of separate ticket efforts. It also reinstates comment subtypes for now - whether it should remain that way is subject to discussion in the next REST API meeting.

It also fixes the object ID being correctly passed to the get_post_type() call in get_object_subtype(), as discovered by @spacedmonkey previously.

Except the comment type discussion, this is ready. After we have determined whether we need comment subtype support or not, let's implement that and all give the related tests a final run. When those pass, we can ship it.

Afterwards, we can focus on follow-up improvements, such as #43941, #44238 and #44387.

[spacedmonkey]

So my patch did 4 things.

1. Fixed the $object_id being passed to the get_post_type function.

This on is a no brainer and should remain in the patch.

2. Added support for site meta (blog meta) in get_object_subtype.

@flixos90 I am extremely confused why this would be removed in your patch. I worked on blog meta and I know that this check will work.
Blog meta has been in a core for months now and releasing this patch without support doesn't make any sense to me at all.
Can you please explain why it shouldn't be in this patch?

3. Add sub type to is_protected_meta.

After a long chat with @rmccue and @kadamwhite about this, we agree for the sake of completeness, that this work should be done.
Not adding it to protected meta, makes protected meta an outlier. If it doesn't go in this patch,
it should go in very soon after in another patch.

4. Removed comment type

So comment type is something that I really wanted in this patch.
However, when I looked into it, there is no comment_type passed in the comments rest controller. Both post and terms, pass post type / taxonomy into the controller, making this possible.
However there is no comment type and no comment type api. I was surprised to find that there is no register_comment_type in core. Without this basic ways of core know the comment type, it is impossible to support.
I want to work on register_comment_type but it is massively out of scope for this ticket.

TLDR.

• Fixed the $object_id is good.
• Site meta is should remain in, as it is in truck.
• is protected meta sub type should is good but should likely move to another ticket for the sake of cleaness.
• Comment types are impossible to support because a lack of register_comment_type

[flixos90]

Replying to spacedmonkey:
I’m sorry, I feel like there was some miscommunication here. Let me clarify a bit:

1. Fixed the $object_id being passed to the get_post_type function.

This on is a no brainer and should remain in the patch.

It absolutely is, I didn’t remove this.

2. Added support for site meta (blog meta) in get_object_subtype.

@flixos90 I am extremely confused why this would be removed in your patch. I worked on blog meta and I know that this check will work.
Blog meta has been in a core for months now and releasing this patch without support doesn't make any sense to me at all.

As I mentioned before, adding support for site meta is not as trivial as it seems: It needs further thought in terms of how to handle the naming blog vs site, and this is a separate issue from general subtype handling, which is the focus of #44387. It should definitely be part of 5.0, but it needs more thought and should be an iteration.

3. Add sub type to is_protected_meta.

After a long chat with @rmccue and @kadamwhite about this, we agree for the sake of completeness, that this work should be done.
Not adding it to protected meta, makes protected meta an outlier. If it doesn't go in this patch,
it should go in very soon after in another patch.

Absolutely. There is #44238 to implement this, and it should go into 5.0 as well. Let’s not overcomplicate this ticket, but rather iterate with the other.

4. Removed comment type

I want to work on register_comment_type but it is massively out of scope for this ticket.

It would be if it was just an extra feature, but we have to consider forward-compatibility here. @rmccue argued that not using the existing comment types will lock us in in the future because every comment would just have a subtype of “comment”, and I agree with it. I also understand your argumentation, it was actually why we didn’t initially include it. Long story short, let’s discuss this in the meeting this week. We can figure it out on Slack much better than with asynchronous communication.

[kadamwhite]

Thank you @flixos90 and @spacedmonkey for the patches, and the discussion. For the purposes of this ticket, and this ticket alone, this is what I'd like to see:

• Handle Comment Types in a follow-up patch, based on the complications we discovered extrapolating the change during WCEU contributor day. (Follow-up ticket should to be created)
• Handle blog site objects in a follow-up patch, as part of #44387
• Leave subtype in function signature for is_protected_meta for consistency, as we modify most other methods in that region. This has impact on #44238 but does not strictly overlap with the purpose of that ticket.

If we limit scope in that way, I think we're good to move forward on this!

[spacedmonkey]

My view on this is the following.

• Comment type is something I would really love to support, but I don't think that core currently supports it. There is no register comment type and no go way to workout type in the api. Until we have all that in place, I don't believe it should go in. However, I don't want this to block this ticket. I will open another ticket register comment types and how core can better support them.
• I am not sure what the confusion with site meta. The meta type is blog and there is nothing we can do about that. As this function is most going to be used by core and develop interaction with it is likely to be with the get sub types filter, I don't see why we don't just put the case statement found in my patch.
• I have updated on my patch for #44238 with 44238.2.diff. This requires this patch to be in for it work, so I was unable to test it. But this patch alone is already 10kb of code. I think we should do it as patch, just for the sake of cleaness of commit and ablity to revert if something is wrong with protected meta.

[flixos90]

I agree with the ideas that both of you @kadamwhite and @spacedmonkey are highlighting.

However, my main point is the following: Both of you are mentioning specific "extra parts" to registering metadata should be part of this ticket right here. I don't see why. Everything related to register_meta() should definitely be part of 5.0, but this ticket is indeed quite complex already, and it is much easier to take care of the rest in follow-up tickets, for the sake of overview and also granular commits like @spacedmonkey mentioned.

Instead of arguing why one thing should be part of this and another doesn't, I really think we should keep things simple here and do all the following things afterwards:

• Meta default values (#43941)
• Registerable protected meta values per subtype (#44238)
• Site meta support when registering meta (#44387)
• Comment types support or not? (ticket yet to be created)

[flixos90]

38323.16.diff​ is a small update that removes comment types from the patch.

With that, the patch now only contains everything that is crucial to registering metadata for subtypes, as mentioned above.

[kadamwhite]

In 43378:

REST API: Support meta registration for specific object subtypes.

Introduce an object_subtype argument to the args array for register_meta() which can be used to limit meta registration to a single subtype (e.g. a custom post type or taxonomy, vs all posts or taxonomies).

Introduce register_post_meta() and register_term_meta() wrapper methods for register_meta to provide a convenient interface for the common case of registering meta for a specific taxonomy or post type. These methods work the way plugin developers have often expected register_meta to function, and should be used in place of direct register_meta where possible.

Props flixos90, tharsheblows, spacedmonkey.
Fixes #38323.

[spacedmonkey]

So I was going to make another ticket for this, but I am not sure it makes sense. It is a small change and it think it is cleaner if the conversion continues here.

In 38323-improved-filter.diff I refactored the code so the get_object_subtype_{$object_type} now passed the object, as this is already in memory. It add better context to the filter.

[SergeyBiryukov]

Replying to spacedmonkey:

So I was going to make another ticket for this, but I am not sure it makes sense.

Please do, closed tickets don't get much attention. Passing the object to get_object_subtype_{$object_type} seems like a good idea.

[kadamwhite]

Reopen for backport.

And yes, @spacedmonkey let's add that additional filter parameter in a new ticket, and get it in for 5.0!

[kadamwhite]

In 43510:

REST API: Support meta registration for specific object subtypes.

Introduce an object_subtype argument to the args array for register_meta() which can be used to limit meta registration to a single subtype (e.g. a custom post type or taxonomy, vs all posts or taxonomies).

Introduce register_post_meta() and register_term_meta() wrapper methods for register_meta to provide a convenient interface for the common case of registering meta for a specific taxonomy or post type. These methods work the way plugin developers have often expected register_meta to function, and should be used in place of direct register_meta where possible.

Props flixos90, tharsheblows, spacedmonkey.

Merges [43378] to the 4.9 branch.
Fixes #38323.

[kadamwhite]

In 43520:

Docs: Update @since version to 4.9.8 for meta registration subtype handling.

This feature has been backported to 4.9.8 so 5.0.0 is inaccurate.

See #38323.

[kadamwhite]

In 43521:

Docs: Add $object_subtype param documentation for sanitize_meta().

Props timothyblynjacobs.

See #38323.

[flixos90]

Dev note published: ​https://make.wordpress.org/core/2018/07/27/registering-metadata-in-4-9-8/