WordPress.org

Make WordPress Core

Opened 14 months ago

Last modified 4 weeks ago

#38323 new enhancement

Reconsider $object_subtype handling in `register_meta()`

Reported by: flixos90 Owned by:
Milestone: 5.0 Priority: normal
Severity: normal Version:
Component: Options, Meta APIs Keywords: has-patch needs-unit-tests 2nd-opinion
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 (6)

38323.diff (13.4 KB) - added by flixos90 14 months ago.
38323.2.diff (25.2 KB) - added by tharsheblows 12 months ago.
unit tests for custom post type meta
38323.3.diff (26.9 KB) - added by flixos90 10 months ago.
wp-object-type.diff (4.3 KB) - added by flixos90 8 months ago.
wp-object-type.2.diff (7.3 KB) - added by flixos90 8 months ago.
38323.4.diff (14.5 KB) - added by flixos90 4 weeks ago.

Download all attachments as: .zip

Change History (43)

@flixos90
14 months ago

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


14 months ago

#4 follow-up: @tharsheblows
12 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
12 months ago

unit tests for custom post type meta

#5 in reply to: ↑ 4 @flixos90
12 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
12 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.


11 months ago

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


10 months ago

#9 @kadamwhite
10 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
10 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.


10 months ago

@flixos90
10 months ago

#12 @flixos90
10 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.


10 months ago

#14 @kadamwhite
10 months ago

  • Focuses rest-api added

#15 @kadamwhite
10 months ago

#39803 was marked as a duplicate.

#16 @swissspidy
10 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.


10 months ago

#18 @westonruter
10 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.


10 months ago

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


8 months ago

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


8 months ago

#22 follow-up: @jnylen0
8 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
8 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
8 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
8 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
8 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.


7 months ago

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


6 months ago

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


5 months ago

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


4 months ago

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


3 months ago

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


3 months ago

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


3 months ago

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


2 months ago

@flixos90
4 weeks ago

#35 @flixos90
4 weeks 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 4 weeks ago by flixos90 (previous) (diff)

#36 @flixos90
4 weeks 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
4 weeks 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.

Note: See TracTickets for help on using tickets.