#35658 closed task (blessed) (fixed)
Provide additional data for registered meta through register_meta()
Reported by: | jeremyfelt | Owned by: | helen |
---|---|---|---|
Milestone: | 4.6 | Priority: | high |
Severity: | normal | Version: | |
Component: | Options, Meta APIs | Keywords: | needs-unit-tests needs-testing has-patch rest-api |
Focuses: | Cc: |
Description
During a discussion at the REST API hack day, we determined it would be useful to expand the scope of register_meta()
so that specific information about that meta data could be registered in code rather than in the DB, similar to register_content_type()
. #metameta
For the benefit of the REST API, this information should at least include the expected data type and the description so that a more proper JSON Schema for the endpoint can be provided. We don't need to be strict on the data type as the meta value is output, but we can help to set expectations.
It may also be necessary to register the publicness of meta in a more explicit way through a show_in_rest
or similar argument.
There are likely other areas in core that could benefit from having additional information about registered meta.
Related REST API Tickets:
Attachments (50)
Change History (204)
#2
@
9 years ago
Results of some more discussion around this:
- We'll need to start using a
$wp_meta_keys
global to track registered meta keys. - Uniqueness is important. A meta key can technically exist for multiple types objects (post, comment, user, term) as well as types of those types of objects—custom post types. The global array should track these uniquely so that a proper JSON Schema can be output.
It's possible the first argument of register_meta()
should be an array:
$args = array( 'post' => array( 'post', 'cpt-one', 'cpt-two' ), 'term', ); register_meta( $args, 'my_meta_key', 'my_sanitize_callback' );
The third argument should be an array as well (as @swissspidy just mentioned):
$object_args = array( 'post' => array( 'post', 'cpt-one', 'cpt-two' ), 'term', ); $args = array( 'rest_data_type' => 'string', 'rest_description' => 'A description of this meta for the JSON Schema', 'sanitize_callback' => 'my_sanitization_callback', ); $register_meta( $object_args, 'my_meta_key', $args );
#3
@
9 years ago
$wp_meta_keys
may be strange because of uniqueness as well. The meta keys for object types and custom types of those object types need to be tracked separately.
$wp_meta_keys = array( 'post' => array( 'post' => array( 'meta_key' => array( 'rest_data_type' => 'string', 'rest_description' => 'This is the description of my meta', ), ), 'books' => array( 'meta_key' => array( 'rest_data_type' => 'string', 'rest_description' => 'This is the description of my meta', ), ), 'user' => array( 'user' => array( 'meta_key' => array( 'rest_data_type' => 'string', 'rest_description' => 'This is the description of my meta', ), ), ), );
#4
@
9 years ago
I would recommend using type
and description
instead of rest_data_type
and rest_description
, as these registration arguments are intended as low-level primitive attribute properties.
This ticket was mentioned in Slack in #core-fields by jeremyfelt. View the logs.
9 years ago
#6
@
9 years ago
Chatting today the issue of multiple meta key meta vs. single. Typically, a given meta registration will be either for a "single" key use case, or multiple. By specifying this in register_meta
we'll be able to use it in the REST api to indicate whether to show an array of values for the key, or a single value it's self. This could also be used in core potentially when calling get_post_meta
and defaulting the third param to the registered single / multiple flag.
Given this is a new API, we shouldn't have to worry about backwards compatibility for things like get_post_meta
.
#7
@
9 years ago
I think there's some overlap here between what the Fields API is aiming to do, so perhaps we should chat about this further in that context and see what we can do in the Fields API to address some of these needs.
#8
@
9 years ago
While it is a cool idea to default the last parameter of get_post_meta
according to the registered meta, I think we do have to worry about backwards compatibility there. Say your favorite meta UI plugin starts to use this API and the third parameter of get_post_meta
changes the default for those now registered fields. Now, things will suddenly break for code that has already been developed against the old behavior. To prevent that, the plugin cannot start using the new API without changing their API.
#9
@
9 years ago
A minor correction: for backwards compatibility the function would include a fourth parameter, $auth_callback which is a callback for capability checks.
So we ended the discussion @jeremyfelt mentioned with something like this - I've included all the suggestions above to see what it looks like:
// $object_args is the object type (post, user etc) and optional string or array of subtype(s) $object_args = array( 'post' => array( 'post', 'cpt-one', 'cpt-two' ), // a post object with post type post, cpt-one or cpt-two 'term', // a term object ); // $meta_key is the meta_key $meta_key = 'my_meta_key'; // $args is the array with all the new args. If is_callable, it is taken to be the sanitization callback. $args = array( 'data_type' => 'string', // to use in REST api schema 'description' => 'This is the meta key description.', // to use in REST api schema, 'single' => false, // to use in REST api schema and response as @joehoyle suggested 'show_in_rest' => true, // should it be included in REST api schema and response at all 'sanitize_callback' => 'my_sanitization_callback', // the custom sanitization callback ); // $auth_callback is the authorization callback and needed for back compatibility $auth_callback = 'my_auth_callback'; register_meta( $object_args, $meta_key, $args, $auth_callback );
There was also a discussion around how unwieldy the $wp_meta_keys global would be so we thought it would make it more manageable to have a function get_registered_meta which narrows it down a bit, something like:
// eg instead of $wp_meta_keys[$object_type][$object_subtype][$key]['description']; $object_args = array( 'post' => 'cpt-one' ); $key = 'my_meta_key'; $registered_meta = get_registered_meta( $object_args, $key ); $description = $registered_meta['description']; // this should be 'This is the meta key description.'
@sc0ttkclark This came about after a discussion about how to build the REST api schema for meta and determine whether or not a meta_key should be included at all in contexts which don't require authorization. We need to be able to get the meta key metadata (eg 'data_type', 'description') but there might be a better way or something which works better with the plans for the Fields api.
#10
@
9 years ago
- Keywords needs-testing has-patch added; needs-patch removed
Rabbit. Hole. :)
35658.diff is an initial exploration, combining the thoughts (I think) so far:
register_meta()
now supports an array of arguments as the 3rd parameter rather than individual parameters for$sanitize_callback
and$auth_callback
.- New arguments for
register_meta()
includeobject_subtype
,show_in_rest
,type
(data type),description
. register_meta()
now actually registers meta keys in the global$wp_meta_keys
, similar to post types, so that data about the meta keys can be retrieved separately from the values associated with those keys.- Introduce
unregister_meta_key()
as a way to remove a previously registered meta key. Also similar tounregister_post_type()
. - Introduce
get_registered_meta_keys()
to provide a list of registered meta keys for an object type and subtype. - Introduce
get_registered_metadata()
, effectively as a wrapper forget_metadata()
that returns only data associated with registered meta keys for a given object ID and type. - Update
sanitize_meta()
to support an$object_subtype
parameter. - Update
sanitize_meta()
to apply a newsanitize_{$object_type}_{$object_subtype}_meta_{$meta_key
filter for sanitization. - Change the
$meta_type
parameter to$object_type
insanitize_meta()
to better match$object_subtype
. - Some initial tests.
REST API hack day contributors to the discussion that kicked this patch off: @tharsheblows, @nullvariable, @ivdimova, @kvignos, @akibjorklund, @martingoldie, @potatomaster, @jaredcobb
Some thoughts during patch creation:
- Do we need to validate object types during
register_meta()
so that this doesn't accidentally become something it's not? Or do we already provide accidental support for different object types at this point AND want to provide an enhanced structure for that as well? - Once you have an object ID, the subtype of the object no longer matters as much, only as a way to verify that the meta key has been registered for that subtype. So we don't necessarily need to convert all existing filters around meta to handle subtypes (immediately at least), only the ones that do not support an object ID.
- Is it overkill to add all of the handling for subtypes to every type of object? Would it be more overkill to split the logic up into object types that support subtypes? It's probably safe to handle objects as if they could be split into types and subtypes.
- More rabbit holes appear when you start to dig into how to handle serialized data. This is something we can't support fully in the REST API, but is something that is used elsewhere. Could it make sense to say that registered metadata cannot be serialized? If we do that, do we then need to enforce it whenever storing meta as well? I guess this is more of a problem at the REST API layer, which could feasibly check for
is_serialized()
and avoid providing bad data. - It's possible
show_in_rest
isn't a good name. It may be enough to explicitly register aspublic
rather than the current behavior, which is registering as not protected. - The filter applied for sanitization is straight forward as it is applied for all object types (meta types). The auth callback is only applied for the
post
object type, so does not currently fire foruser
,comment
, orterm
. It may make sense to expand this, but it may also be an opportunity to revisit how authorization works around meta. /shrug - Adding to that, authentication is only checked when saving or editing post meta and not for reading post meta. Is something like
show_in_rest
enough? If meta is registered as public throughregister_meta()
, should we assume that its publicness then matches that of the parent object at that point? Probably. - Should a post type (or object subtype) exist before a meta key can be registered to it? That could cause confusion depending on the timing of registration. It may not hurt anything if registered out of order.
#11
follow-up:
↓ 12
@
9 years ago
In the Fields API, we're doing some stuff with register_meta
and register_rest_field
right now, I'm wondering if we should wait on this ticket and first discuss what the best way forward is:
- Merging the patch as is, Fields API adding integration with that
- Moving some of this logic into the Fields API
#12
in reply to:
↑ 11
@
9 years ago
Replying to sc0ttkclark:
In the Fields API, we're doing some stuff with
register_meta
andregister_rest_field
right now, I'm wondering if we should wait on this ticket and first discuss what the best way forward is:
+1, we do need to keep gathering requirements from all around. The above was a pretty big jump start.
- Merging the patch as is, Fields API adding integration with that
- Moving some of this logic into the Fields API
Looking at how the Fields API uses register_meta()
, it seems like option 1 should be fairly straight forward, though that depends on how register_rest_field()
is being relied on.
Anything registered as "show_in_rest" (public) with register_meta()
would be added to a list of all public meta key/values that would be included when public meta is requested from an object's endpoint. We decided that the current use of register_meta()
was not enough to explicitly define publicness. And we need a way to create a JSON Schema showing the registered public meta, even when not requesting the data.
Anything meta separately registered with register_rest_field()
would likely not be registered this way as it would be included with the object's endpoint as a custom field.
#13
@
9 years ago
I guess this is more of a problem at the REST API layer, which could feasibly check for is_serialized() and avoid providing bad data.
Currently it does and you're right, I think the verdict was that it wasn't safe to include. Should register_meta()
error if show_in_rest
is true and the data fails the serialization check?
The auth callback is only applied for the post object type, so does not currently fire for user, comment, or term. It may make sense to expand this, but it may also be an opportunity to revisit how authorization works around meta.
Adding to that, authentication is only checked when saving or editing post meta and not for reading post meta. Is something like show_in_rest enough? If meta is registered as public through register_meta(), should we assume that its publicness then matches that of the parent object at that point? Probably.
I had imagined authorization for the REST API would be handled by the REST API as it only affects the edit context and the create/ update/ delete permissions as you said and not the public contexts. Authorization would either go as the custom callback if there is one or the user's add/edit/delete capabilities with respect to the parent if there's not.
And yes, I think show_in_rest
is enough as it's explicit, consistent with register_post_type
and opt-in.
There could be the case where a protected meta key has show_in_rest => true
. In that case, which has priority? I would guess show_in_rest
but don't know -- how does the Fields API deal with protected meta keys?
#14
@
9 years ago
Fields API doesn't do anything special currently for protected meta keys, capabilities are set when you register the control to the section. Might in the future set default capability check to return restricted if it's a protected meta key.
#15
@
9 years ago
I think the REST API chain looks like this:
- Meta is either protected or not protected via the application an auth callback or with a
_
prefix. - Both protected and not protected meta can be registered for automatic inclusion in a REST API meta endpoint via
register_meta()
with "show_in_rest". - If a meta key has been registered with "show_in_rest", it will appear in the JSON Schema for the object. If the user is authenticated, information on protected meta will also appear in the schema.
And then:
- If a meta key has been registered with "show_in_rest", it will be available via
REST_API_URL/post/123/meta/
- If a meta key has been added as a custom field in
register_rest_field()
, it will be available as a property inREST_API_URL/post/123/
#16
@
9 years ago
I guess this is more of a problem at the REST API layer, which could feasibly check for is_serialized() and avoid providing bad data.
In the REST API we'd potentially be able to allow serialized data given we'd be able to sanitize the data on output and input. For example, if you register meta which is of type arrayOf
number
theres no reason we couldn't validate based off that. Right now we don't actually have support for that, but JSON Schema supports this and should be expected to be added in the REST API at some point.
I'd imagine arrayOfType will be useful elsewhere too, not just the REST API.
#17
@
9 years ago
It's very possible I'm entirely misunderstanding this, apols, so is this what you're thinking for serialized data? If I have this as a meta value for the user_details_array meta key:
$meta_value = array( 'name' => 'Bob', 'age' => 63, 'email' => 'bob@example.com' );
I could register it with the data type something like
$data_type = array( 'array' => array( 'name' => 'string', 'age' => 'number', 'email' => 'string' ) );
which would be used to sanitize and validate (in addition to custom callbacks?) and put in the schema something kind of like this?
... "user_details_array": { "type": "array", "properties": { "name" : { "type": "string" }, "age" : { "type": "number" }, "email" : { "type": "string" } }
Then when I went to update it, it would check against the schema and give me an error if I, eg, tried to change it to an object without changing the registered_meta data. There would also be a sensible default type for the array / object items (probably "string") so I could just use 'data_type' => 'array'
when registering the meta key and it'd still work. But I would be required to state the data type if I wanted to use serialized data.
This ticket was mentioned in Slack in #core by jeremyfelt. View the logs.
9 years ago
This ticket was mentioned in Slack in #core-restapi by jeremyfelt. View the logs.
9 years ago
This ticket was mentioned in Slack in #core-restapi by joehoyle. View the logs.
9 years ago
This ticket was mentioned in Slack in #core-restapi by rmccue. View the logs.
9 years ago
This ticket was mentioned in Slack in #core-fields by sc0ttkclark. View the logs.
9 years ago
This ticket was mentioned in Slack in #core-fields by eric. View the logs.
8 years ago
#25
@
8 years ago
@jeremyfelt thank you for starting the fire here.
I have some ideas in attachment:35658.2.diff:
- Let's introduce something like
class WP_Meta_Manager
to store state of what is registered and what isn't. This guards us from developers accessing (and modifying) public global arrays. - I've moved the business logic of some registry-related functions into it, and left the global functions as wrappers for them (
registered_meta_key_exists()
andunregister_meta_key()
)
Follow-up thoughts:
- Should the JSON data type value represent the entire JSON schema type attributes (items, properties, etc.) a la
<?php register_meta_data( 'review', 'post', array( 'json_data_type' => array( 'type' => 'array', 'items' => array( 'type' => 'array', 'items' => array( 'type' => 'object', 'properties' => array( 'reviewer_name' => array( 'type' => 'string' ), 'review' => array( 'type' => 'string' ) ) ) ), // No minimum or maximum. // 'minItems' => 0, // 'maxItems' => 0 ) ) );
#26
follow-up:
↓ 29
@
8 years ago
Would y'all be into working on this in a git branch off of Aaron's repo?
This ticket was mentioned in Slack in #core-restapi by eric. View the logs.
8 years ago
#28
follow-up:
↓ 36
@
8 years ago
- Create
schema
element in theregister_meta()
options array, which is a full schema definition of the meta field. Dropdescription
andtype
as both of these should be expressed in the schema. - Make
register_meta()
return true or WP_Error, bubbling up errors fromWP_Meta_Manager->register()
. - Make the registry work for taxonomies, comments and users.
#29
in reply to:
↑ 26
;
follow-up:
↓ 30
@
8 years ago
@ericlewis great progress! 35658.3.diff is looking :thumbsup:
Replying to ericlewis:
Would y'all be into working on this in a git branch off of Aaron's repo?
+1, that would be a nice place to iterate.
#30
in reply to:
↑ 29
@
8 years ago
Would y'all be into working on this in a git branch off of Aaron's repo?
I've moved the current patch into a pull request (also a branch and commit) for review and iteration. If you have changes to make feel free to comment on the PR, or make commits and send PRs to my branch and we can review and merge.
#31
@
8 years ago
I'm on board with this from the direction of the Fields API, looks sound and we can definitely make use and build on top from there!
This ticket was mentioned in Slack in #core-fields by sc0ttkclark. View the logs.
8 years ago
This ticket was mentioned in Slack in #core-fields by sc0ttkclark. View the logs.
8 years ago
This ticket was mentioned in Slack in #core-fields by sc0ttkclark. View the logs.
8 years ago
#35
@
8 years ago
I came here before of @ericlewis' post on Meta. Here is my comment from over there.
But as it seems this is the place for discussion, are we seriously considering storing complex data in a single meta field as not just an edge case where you are storing a JOSN blog for convenience?
IMO this is a really bad idea because it means you cannot use SQL to query the data. Let me beg the team to consider storing complex data like this. It is a format that really stood the test of time and many different websites, and it can handle any level of complexity. It can also support a generic mention of converting too and from a JSON blob:
https://bitbucket.org/newclarity/sunrise-1#markdown-header-post-meta-storage-format
ALSO, I would strongly urge against using nested arrays. It make for brittle code that can easily be mistyped and it makes it impossible to do a generic wp_parse_args() in order to merge in default attributes. If you MUST use nested attributes please consider using nested keys instead with colons to separate, like so; it is easily to extract the subarrays from these nested keys when you need it but this supports wp_parse_args() much better, and in my experience you really need that.
Here is Eric's example from the Make post reworked using nested keys:
register_post_meta( 'album', array( 'object_subtype' => 'artist', 'schema:type' => 'object', 'schema:properties:name:type' => 'string', 'schema:properties:genre:type' => 'string', 'schema:properties:genre:options' => array( 'Rock', 'R&B', 'Shoegaze' ), 'schema:properties:release-date:type' => 'date', 'schema:repeatable' => true ) );
But even that seems over-architected but I am really struggling to understand the example -- the semantics of the example are not clear to me -- so I can't give a simplified example.
But I do feel this area is critical to get right because getting it wrong dooms many people to writing too-complex and rather error prone code.
#36
in reply to:
↑ 28
@
8 years ago
Replying to ericlewis:
final class WP_Meta_Manager {
… a final class without an interface is broken code: impossible to replace during runtime and very hard to mock in tests. We do have too much of that already in the core.
Whatever you do: Please do not repeat this mistake.
This ticket was mentioned in Slack in #core-fields by sc0ttkclark. View the logs.
8 years ago
This ticket was mentioned in Slack in #core by sc0ttkclark. View the logs.
8 years ago
#39
follow-up:
↓ 46
@
8 years ago
I haven't tackled whether to remove usage of final
or not on WP_Meta_Manager, those decisions are above where I'm at and where I'm focused right now ;)
https://core.trac.wordpress.org/attachment/ticket/35658/35658-objects.diff
The latest patch I am offering up allows for meta properties to be registered with register_meta(), just like the patch from @ericlewis, however I've moved logic from inside register_meta() into the WP_Meta_Manager::register() to consolidate things better.
Also, while register_meta() only accepts an array, the WP_Meta_Manager::register() method accepts an array OR object allowing for forward-compatibility with the Fields API. In the future, register_meta() can accept objects if they pass an is_a() check, depending on the class / object structure we agree on in the future.
There remains a few @todo
markers you can see in the diff which need to be thought about.
And there also remains the decisions on exactly how to utilize the schema
property and what Data Schema to use. There will be further discussion about this in today's weekly dev chat meeting.
This ticket was mentioned in Slack in #core-fields by sc0ttkclark. View the logs.
8 years ago
This ticket was mentioned in Slack in #core by sc0ttkclark. View the logs.
8 years ago
#42
@
8 years ago
Based on further feedback, we know we need further discussion around usage of schema
and exactly what we intend with it. However, in the interest of moving this patch forward -- I have removed the schema
option from the code, but this does not at all mean it cannot be passed in and used as we further define the definition of it for both the Fields API and REST API.
https://core.trac.wordpress.org/attachment/ticket/35658/35658-objects.2.diff
This ticket was mentioned in Slack in #core-fields by sc0ttkclark. View the logs.
8 years ago
This ticket was mentioned in Slack in #core-fields by sc0ttkclark. View the logs.
8 years ago
This ticket was mentioned in Slack in #core-fields by danielbachhuber. View the logs.
8 years ago
#46
in reply to:
↑ 39
@
8 years ago
Replying to sc0ttkclark:
And there also remains the decisions on exactly how to utilize the
schema
property and what Data Schema to use. There will be further discussion about this in today's weekly dev chat meeting.
In a bit of hindsight, I think the debate about JSON Schema doesn't necessarily need to come to a conclusion to solve this ticket.
Back to @jeremyfelt's original description:
For the benefit of the REST API, this information should at least include the expected data type and the description so that a more proper JSON Schema for the endpoint can be provided.
Of JSON Schema, all we really need at this point is the data type for the attribute / meta we're registering. If we include data_type
or similar as a registration argument, where the value can be one of array, boolean, integer, number, null, object, or string, then we solve the immediate need of the ticket.
#47
@
8 years ago
Further steps for me are to ensure we have additional tests that cover what we want for register_meta()
.
I wonder why we need data_type immediately though if schema could end up being used in the future? What in core needs data_type and/or schema besides REST API and Fields API (both future patches)?
You can pass any args you want into register_meta
so we don't have to be stuck with anything here yet while things are still ironed out in the implementations of the proposed new argument(s).
This ticket was mentioned in Slack in #core by sc0ttkclark. View the logs.
8 years ago
This ticket was mentioned in Slack in #core-fields by sc0ttkclark. View the logs.
8 years ago
This ticket was mentioned in Slack in #core by chriscct7. View the logs.
8 years ago
This ticket was mentioned in Slack in #core by voldemortensen. View the logs.
8 years ago
This ticket was mentioned in Slack in #core-fields by sc0ttkclark. View the logs.
8 years ago
#55
@
8 years ago
Just wanted to note #29082, which has a patch that also changes the function signature of sanitize_meta()
.
#56
@
8 years ago
There were several conversations around register_meta()
during WordCamp Europe this weekend and the general consensus is that we should get something simple in now that provides a good option for future compatibility. /cc @helen @rmccue @danielbachhuber @ericlewis @joehoyle
I refreshed the initial patch as 35658.4.diff with this in mind. As we have only a handful of days left before beta 1, immediate feedback is helpful. :)
After re-reading the entire thread, I want to reword some assertions from a previous comment that I think still apply, but without REST API specific language:
- Meta is either protected or not protected via the application of an auth callback or with a _ prefix.
- Both protected and not protected meta should be registered with whitelisted arguments.
In the latest patch, the whitelisted arguments are:
sanitize_callback
auth_callback
object_subtype
(only accepted when object type is "post")type
description
show_in_rest
Using show_in_rest
rather than a more generic public
allows us to start solve an immediate problem at hand while not falling into the trap of defining one version of "public" that may get less clear in the future. (See register_post_type()
).
type
and description
should not necessarily be limited by core. The REST API (and others) can determine what is valid for that specific use.
Thank you @ericlewis and @sc0ttkclark for exploring the possibilities in the several other patches (!!!) on this ticket. I think this work will continue to guide where we need to go.
#57
@
8 years ago
@jeremyfelt Thanks for picking this back up. Aside from a few smaller issues, the patch looks reasonably good after a first read. It needs lots of unit tests too :)
To possibly pre-empt some of the debate about the patch, would it be a good idea to produce a first draft of the dev notes for these changes, documenting why we're adding what we're adding now, and what developers can expect to see in the future? I think doing so might focus the conversation around whether or not the patch meets the goals of the dev notes.
#58
follow-up:
↓ 59
@
8 years ago
@jeremyfelt just a small naming idea, I feel expose_to_rest
might better describe what it is actually for.
#59
in reply to:
↑ 58
;
follow-up:
↓ 64
@
8 years ago
Replying to [comment:57 danielbachhuber:
Aside from a few smaller issues, the patch looks reasonably good after a first read. It needs lots of unit tests too :)
A lot. :) Some really specific tests asserting current register_meta()
behavior are also needed.
To possibly pre-empt some of the debate about the patch, would it be a good idea to produce a first draft of the dev notes for these changes, documenting why we're adding what we're adding now, and what developers can expect to see in the future? I think doing so might focus the conversation around whether or not the patch meets the goals of the dev notes.
I agree, I think this would be helpful and gets us ready for dev notes anyway. :) If anybody would like to pick this up over the next 12 hours, please do. I can start on a draft later if not. I'd encourage a read of https://github.com/WP-API/WP-API/issues/2149, which may come close to explaining the "why".
Replying to jipmoors:
@jeremyfelt just a small naming idea, I feel
expose_to_rest
might better describe what it is actually for.
I think for consistency, we need to stay aligned with register_post_type()
, which uses show_in_rest
for a similar concept.
#60
@
8 years ago
I've published a document with an initial draft of dev notes for the new register_meta()
. Anyone with the link should be able to comment, please do.
This ticket was mentioned in Slack in #core by jeremyfelt. View the logs.
8 years ago
#62
@
8 years ago
@DrewAPicture Do you have time to do a docs review on the latest patch? I know there are a handful of places that could use some work still. :)
#63
follow-up:
↓ 67
@
8 years ago
Looking at the most recent patch (35658.4.diff) on line 967 I notice the addition of a $subtype
.
That worries me because it portends a proliferation of extra optional parameters having to be passed around making function signatures more complicated and harder to remember.
Our team went down that path with our Sunrise library several years ago and it was probably the biggest architectural mistake we made. What we found was that we ended up having to pass two values around as we were building plugins and it was an explosion of complexity that you can start to see in the patch. I explained it ''here''; scroll down to the section entitled "Use an "Object Type" Class."
Of course it took several years painful experience to realize we had made that mistake; it really did seem like a great idea at the time.
What we found wat that moving to a single value that encapsulated subvalues worked much better.
So instead please consider creating a combined type in one string in the form "{type}:{subtype}", or just "{type}" which defaults "{subtype}" to "any".
I have created an updated patch as 35658.5.diff that adds a WP_Object_Type
class to WordPress core. It has a __toString()
method so it can be used as a string. Using this class makes it easy to add subtype support in a backward compatible way to any functions that currently support a post type or other type.
A WP_Object_Type
class can also get rid of duplicate code like the following from the patch because it makes it the responsibility of the WP_Object_Type
object to police itself:
if ( empty( $object_subtype ) || 'post' !== $object_type ) { $object_subtype = $object_type; } if ( ! in_array( $object_type, array( 'post', 'comment', 'user', 'term' ) ) ) { return new WP_Error( 'invalid_meta_key', __( 'Invalid meta key. Not a core object type.' ) ); }
BTW, we built this class so we could implement Object Relationships and it is really needed there (which hopefully WordPress will add sometime soon.)
I am really hoping the core committers will consider this. There is about 7 years worth of WordPress-specific engineering behind this use-case.
#64
in reply to:
↑ 59
@
8 years ago
Replying to jeremyfelt:
I think for consistency, we need to stay aligned with
register_post_type()
, which usesshow_in_rest
for a similar concept.
Oh absolutely, wasn't aware it was used there already.
#65
follow-up:
↓ 66
@
8 years ago
As lead dev of CMB2 and dealing with lots and lots of meta for all object types, I would tend to agree with @MikeSchinkel here that we should probably attempt to handle the sub-typing a bit more elegantly if possible. (single responsibility principle and all that)
Mike, I like your patch, but there does seem to be some opinionated stuff in the WP_Object_Type class (like all the $_short_types
related business) that may be overkill for getting an MVP in to core. I'm also curious if we really need to support all the different methods for instantiation. Seems it would be prudent to limit that to a specific set like either a colon-separated string or array with 'type'
and 'subtype
' keys.
Also, can we pretty please NOT do final
on the class? (Mike, I know you're likely doing that to mirror existing core classes, but I'm hoping we can avoid it anyway). For instance, what if I wanted to extend this class for use in CMB2? Well.. I couldn't.
Also, nit-picky, but would prefer is_type_*
methods over is_*_type
so as to avoid confusion with is_post_type
.
#66
in reply to:
↑ 65
@
8 years ago
Replying to jtsternberg:
Mike, I like your patch, but there does seem to be some opinionated stuff in the WP_Object_Type class (like all the
$_short_types
related business) that may be overkill for getting an MVP in to core.
Cool.
The class I included was a hastily prepared copy of code we are using as a base for our projects. Hastily because I saw this ticket moving quickly and did not want to miss the chance to weigh in before a decision was made.
Clearly to make it into core the class will need to be massaged by committee so I assumed that if the idea was accepted my code would get changed quite a bit.
The short_type stuff can be pulled out no problem, it was just critical to an object relationships module we built, but if this made it into core I could easily put that code elsewhere.
I'm also curious if we really need to support all the different methods for instantiation. Seems it would be prudent to limit that to a specific set like either a colon-separated string or array with
'type'
and'subtype
' keys.
We did this so that we could have flexibility, especially related to being able to use Structural Typing (allowing other objects to be passed in ->type
and ->subtype
properties and still have them work.) But ultimately none of that is needed, it could just accept no more than either a string or an existing WP_Object_Type
instance.
Also, can we pretty please NOT do
final
on the class? (Mike, I know you're likely doing that to mirror existing core classes, but I'm hoping we can avoid it anyway). For instance, what if I wanted to extend this class for use in CMB2? Well.. I couldn't.
Funny, I am usually extremely frustrated when core uses final
, but in this case I felt it was warranted because IMO an WP_Object_Type
would effectively become a new standard data type and so locking it down and standardizing it made the most sense to me.
That said, the only parts I feel really strong about are:
- Having a class that encapsulates a major type and a minor type
- That can be instantiated by either a string or an existing instance of the same class, and
- That can be cast back to a string to the form "{type}:{sub_type}".
Anything else about it can be changed without harming the concept.
And If we could get this I think it would be a huge win for core, and given my experience with it I think everyone will be surprised at how often they discover it can simplify other code as time moves forward.
Also, nit-picky, but would prefer
is_type_*
methods overis_*_type
so as to avoid confusion withis_post_type
.
Like I said, I only really care about 1., 2. and 3. above. So whatever the consensus is. :-)
#67
in reply to:
↑ 63
;
follow-up:
↓ 72
@
8 years ago
Replying to MikeSchinkel:
That worries me because it portends a proliferation of extra optional parameters having to be passed around making function signatures more complicated and harder to remember.
Thanks for commenting @MikeSchinkel and @jtsternberg. I can appreciate the concern.
I'd like to avoid the overhead of a WP_Object_Type
object this time around. We only have "registered" subtypes for posts, though #12668 has made some things around comment types more friendly. Using this ticket to standardize on a new core notation for object type/subtype while trying to preserve forward compatibility seems a bit dangerous.
I don't dislike the syntax of register_meta( 'post:newsletter', ... )
that 35658.5.diff would create. It's a new one to core, but it's worth thinking about using a similarly formatted string in this context.
After reading through WP_Object_Type
, I realized that I may want to register a meta key to multiple post types. With some work on 35658.4.diff, we could turn that into:
register_meta( 'post', 'issue', array( 'object_subtype' => array( 'newsletter', 'magazine' ), ... );
By turning the first argument into one that contains both the object and object subtype, we would need to register it twice:
register_meta( 'post:newsletter', 'issue', $args ); register_meta( 'post:magazine', 'issue', $args );
We could turn that first argument into string|array
and support:
register_meta( array( 'post:magazine', 'post:newsletter' ), 'issue', $args );
But that starts to add some complexity to the handling of the first argument that I don't think we want to introduce now.
From a previous comment of mine that asked a bunch of questions:
Should a post type (or object subtype) exist before a meta key can be registered to it? That could cause confusion depending on the timing of registration. It may not hurt anything if registered out of order.
If we can assume that a post type has always been registered before the meta key, then we could accept the object subtype as the first argument and then look up it's parent object type. However, this would lock us into object subtype uniqueness as comments/posts/terms/users could one day have conflicting subtype slugs.
So instead please consider creating a combined type in one string in the form "{type}:{subtype}", or just "{type}" which defaults "{subtype}" to "any".
I think we should avoid the "any" subtype now and ask for explicit registration from developers.
With all that said:
- I think we should capture subtype information in
$args
. It's straight-forward and does the least to existing patterns while preserving what's possible for the future. - I'm okay with developers having to use individual
register_meta()
calls for each object/object subtype combination.
We've had quite a bit of scroll since my request for comments on this document with a draft of dev notes for register_meta, so I'm going to drop it in again. :)
This ticket was mentioned in Slack in #core by ocean90. View the logs.
8 years ago
#69
@
8 years ago
The inability to make use of subtype for taxonomies causes me some heartache, given the Fields API intent is to make use of subtypes for post types and taxonomies both -- now that taxonomies support meta.
I can understand though that the initial decision to only support post subtypes is where we stand, I just wanted to note my dissent here :)
#70
@
8 years ago
Also, for further use by plugins of this API (think get_metadata( 'my_plugin_type', $my_table_id )
) having restrictions when subtype is passed, could further hold back more use there.
#71
@
8 years ago
I've been dumping patches on here for sharing / checking out direction - they are unlikely to work in any sort of real way as-is.
In any case, here is what I see as being important for a first run based on the many discussions I had at WCEU:
- Enable REST API to start building out meta endpoints.
- Set a direction for a Fields API to build out on top of.
- Guide developers toward intended usage (related to below).
- Defend as-yet-undecided core directions and focus on forward-compat.
In my PoC patch, args are whitelisted using a filter. This is so that core can say "new ones will probably be added later and may conflict with custom stuff you add right now", but still give devs flexibility if they look in core code and are deliberate about what they're doing (essentially saying "yes, I acknowledge that I am voiding any back-compat warranty"). Reaching directly into the global is also voiding the warranty :)
As for the subtype stuff - I don't think we really need to enforce it for posts, but if we decide to do so in core for now, let's also hook it on like the args whitelist.
#72
in reply to:
↑ 67
@
8 years ago
Replying to jeremyfelt:
After reading through
WP_Object_Type
...
Thank you for taking the time to review it.
I'd like to avoid the overhead of a
WP_Object_Type
object this time around.
Understood. Your comment as well as @jtsternberg's comment made me rethink the approach of the 35658.5.diff patch and was something I wanted to address for a long time, which is to create a ::get_instance()
"~factory" method and to make WP_Object_Type
immutable.
Immutability means two(2) things: lower overhead in terms of memory use when used frequently -- only the cost of an object reference vs. the cost of a string references and all its characters -- and it makes WP_Object_Type
truly just a data typing mechanism which is effectively all I ever intended it to be.
In addition your comment made me think through the naming of the major type that had always bothered me because of its inconsistency (i.e. 'post'
, 'term'
, 'user'
and 'comment'
.) The ->type
property that I used in the earlier patch was too generically named, and calling it object_type
such as in 35658.8.diff would mean forcing the object_type
name for the major type and thus (possibly) inadvertently standardizing on a new core notation for object type/subtype which as you said might create a problem for forward compatibility.
So for the latest patch #35658.17.diff there is type_group
which seemed the best. In this context a 'post'
is a "type group" of post types. A 'term'
is a "type group" for taxonomies. And so on.
Back to immutability; the latest patch declares the ->_type_group
and ->subtype
properties to be private
and then exposed ->type_group()
and ->subtype()
methods. In addition I changed __construct()
to private
so it can only be instantiated by ::get_instance()
which will re-dispense an existing object anytime the same string representation for object type is requested (this is where the memory overhead is improved.)
We only have "registered" subtypes for posts, though #12668 has made some things around comment types more friendly.
Great point, and that make sense.
Given that I would take it one step further; use post_name
instead of subtype
in the code and have post_name
map to subtype
in the case of type_group
being equal to 'post'
, and that is what the latest patch offers. It has a ->post_type()
method that returns 'post' === $this->_type_group ? $this->_subtype : null
which ensures only post types are relevant at this point.
If the future when a movement is made to support all subtypes then the change will be able to be explicit in the code. Better to approach it that way for clarity now.
It's a new one to core, but it's worth thinking about using a similarly formatted string in this context.
It is actually not as you know that @ocean90 pointed out on Slack after your comment here. In fact is was what @nacin suggested last year (specifically using the the colon because of image:gif
and image:jpeg
) when reviewing the work I did on the Fields API.
I realized that I may want to register a meta key to multiple post types. With some work on 35658.4.diff, we could turn that into:
register_meta( 'post', 'issue', array( 'object_subtype' => array( 'newsletter', 'magazine' ), ... );By turning the first argument into one that contains both the object and object subtype, we would need to register it twice:
register_meta( 'post:newsletter', 'issue', $args ); register_meta( 'post:magazine', 'issue', $args );
Actually the use of a WP_Object_Type
does not preclude this at all. Nothing says that the first parameter can't be either an object type OR a type group, and if a type group then look for subtypes in the array. But I would suggest subtypes be provides using post_type
, taxonomy
, etc. Then inside register_meta()
you can inspect what is passed and combine as appropriate:
register_meta( 'post', 'issue', array( 'post_type' => array( 'newsletter', 'magazine' ), ... );
The new patch however does not attempt this yet because of uncertainty if this latest patch will even be considered.
From a previous comment of mine that asked a bunch of questions:
If we can assume that a post type has always been registered before the meta key, then we could accept the object subtype as the first argument and then look up it's parent object type. However, this would lock us into object subtype uniqueness as comments/posts/terms/users could one day have conflicting subtype slugs.
Accepting the subtype does not sounds like a good idea to me.
I think we should avoid the "any" subtype now and ask for explicit registration from developers.
I am wondering why you would object to this? Are you objecting just on the grounds of meta, or for the concept of object type in general?
I can see why it might not be a good idea for meta, but in general it can be quite useful. The latest patch does not specifically disallow use of 'any'
for meta, but it could...
Lastly, as you can tell I would really like to see WP_Object_Type
make its way into core. @sc0ttkclark and I discussed this a length last year and I think he is generally supportive of object type class. I do think an WP_Object_Type
class would help for meta though I certainly can appreciate how it would not be required.
I'd ask that you please consider it once more -- and consider how it can help fields soon and relationships in the future too -- but if you are not willing to tackle it now then I would at least suggest wiping all object_type
and subtype
terminology and just going with $args['post_type']
for now since all you want to have subtypes is 'posts'
. And maybe adopt the type_group
terminology too?
#75
follow-up:
↓ 80
@
8 years ago
Well, took just over an hour to find something wrong :) It looks like the fallback auth callback doesn't get applied for the old registration method anymore (I could be wrong, also this needs a test), and I think the logic could be easier to follow. I will likely commit in the near future, but in the meantime, an eye on 35658.19.diff would be great.
#76
@
8 years ago
- Owner changed from jeremyfelt to helen
- Status changed from assigned to accepted
I guess I own this now.
#77
@
8 years ago
Yay merged.
+ * @type string $sanitize_callback A function or method to call when sanitizing
$meta_key
data.
Shouldn't that line have been
+ * @type string|array $sanitize_callback A function or method to call when sanitizing
$meta_key
data.
? As callbacks can be arrays and stuff, if it's a class method? Also for the $auth_callback
item.
@
8 years ago
Fixed a bug in get_registered_metadata() and made sure 'single' is a valid option for registered meta
#79
follow-up:
↓ 82
@
8 years ago
I gave this a test and ran into a couple of bugs which are fixed in 35658.20.diff
- In
get_registered_metadata()
, some code was treating the variable$meta_keys
as the global$wp_meta_keys
, causing invalid index issues - In
get_registered_metadata()
, we were accessing the valuesingle
as though$meta_key_data
was an object when it's an associative array - In
register_meta()
, addedsingle
as a default option. Without that, you can't includesingle
as an option when registering meta, which causes the value to never be set for when it's used inget_registered_metadata()
Besides the items in the attached patch, I noticed some things that feel a little inconsistent between functions that I wanted to bring up for discussion:
- When
register_meta()
fails, it returns false, butunregister_meta_key()
returns aWP_Error
. It would be pretty valuable to know why register_meta() fails when it does, especially if the function is updated in the future and new points of failure are introduced get_registered_meta_keys()
does no specific check for$object_type
, whenregister_meta()
checks and calls_doing_it_wrong()
if an invalid object type is provided.get_registered_metadata()
also performs a check, but returns aWP_Error
. If I was using these functions, I probably would enjoy getting WP_Errors over a mix of false/WP_error/_doing_it_wrong.
Otherwise, the changes happening on this ticket are awesome and I look forward to thinking up how I can use these in the future :)
#80
in reply to:
↑ 75
@
8 years ago
Replying to helen:
It looks like the fallback auth callback doesn't get applied for the old registration method anymore (I could be wrong, also this needs a test), and I think the logic could be easier to follow.
So I am correct that this is the case; however, in looking at tests, map_meta_cap()
itself also falls back if nothing is hooked on for auth so nothing is broken practically speaking. That said, the code is still incorrect in its intention and is confusing to read, so going to commit that fix, and then look at the new feedback in this ticket.
#82
in reply to:
↑ 79
@
8 years ago
Replying to Faison:
I gave this a test and ran into a couple of bugs which are fixed in 35658.20.diff
This patch fix looks good to me.
- When
register_meta()
fails, it returns false, butunregister_meta_key()
returns aWP_Error
. It would be pretty valuable to know why register_meta() fails when it does, especially if the function is updated in the future and new points of failure are introduced
You know, we never returned before, I'd love to use WP_Error more often for new failed return values, this sounds like a good spot we could do that.
get_registered_meta_keys()
does no specific check for$object_type
, whenregister_meta()
checks and calls_doing_it_wrong()
if an invalid object type is provided.get_registered_metadata()
also performs a check, but returns aWP_Error
. If I was using these functions, I probably would enjoy getting WP_Errors over a mix of false/WP_error/_doing_it_wrong.
Our $object_type limitation is one I'm not entirely behind, but yeah the difference here seems odd, we should probably return and WP_Error instead of running just _doing_it_wrong() here, for consistency.
#87
@
8 years ago
I'm working on some tests today, I'll be on Slack in #core and #core-fields if you're looking for me.
This ticket was mentioned in Slack in #core-fields by sc0ttkclark. View the logs.
8 years ago
#89
@
8 years ago
I've got some fixes to register_meta() as well as new tests here:
https://github.com/aaronjorbin/develop.wordpress/pull/11/files
I'm still working on more coverage for other functions, but right now I've got expanded coverage for register_meta() and registered_meta_key_exists() now.
More soon.
#90
follow-up:
↓ 95
@
8 years ago
I've added tests for register_meta(), registered_meta_key_exists(), unregister_meta_key(), get_registered_meta_keys(), and get_registered_metadata().
See patch here: https://core.trac.wordpress.org/attachment/ticket/35658/37924-tests.diff
Props to @ericlewis for the initial test cases from our previous work together on the older patches. I was able to get a really good start based on that, then built out the rest of the test coverage.
#91
follow-up:
↓ 92
@
8 years ago
Can we remove the _doing_it_wrong()
for the object types? I don't see why the register meta functionality should be prevented from functioning with non-core object types when the other metadata APIs do.
#92
in reply to:
↑ 91
@
8 years ago
Replying to TimothyBlynJacobs:
Can we remove the
_doing_it_wrong()
for the object types? I don't see why the register meta functionality should be prevented from functioning with non-core object types when the other metadata APIs do.
I'm +1 on this, but it's not my decision.
#93
@
8 years ago
35658.26.diff was a start at making return values more consistent. However, it would make registering meta to a non-core object type impossible in its current state. To help figure out the best path, what is it that people are doing with non-core object types and what could go wrong if it wasn't restricted?
#94
@
8 years ago
Plugins that have custom meta-like tables (Gravity Forms has this for it's entries and forms). So plugins like these that are currently utilizing *_metadata() integrations for their custom types, can't properly register their own meta keys. The purpose of such registration? REST API endpoints with meta integration on their own endpoints -- could be useful.
Some of these plugins allow other plugins to add fields to their interfaces, perfect examples can be seen in Pippin's plugins -- https://pippinsplugins.com/extending-wordpress-metadata-api/
So now these plugins can't take advantage of these new metadata enhancements. I think we should open it up, we must, as we've already set precedent in other metadata functions to accept custom types there.
#95
in reply to:
↑ 90
;
follow-ups:
↓ 104
↓ 115
@
8 years ago
Replying to sc0ttkclark:
I've added tests for register_meta(), registered_meta_key_exists(), unregister_meta_key(), get_registered_meta_keys(), and get_registered_metadata().
See patch here: https://core.trac.wordpress.org/attachment/ticket/35658/37924-tests.diff
In 35658.29.diff, I've made several of the tests more explicit about what to expect from $wp_meta_kesy
once meta has been registered. There is still some work to do, but each test should also back out any of the changes that it makes so that results don't bleed over into other tests.
A couple things that bugged me:
unregister_meta_key()
needs to remove the filters thatregister_meta()
has added. Need tests for that as well.- We should consider removing
object_subtype
key from the meta key array. It doesn't really serve a purpose as its used to build the array containing the meta key instead. - We should split the
test_post_meta_caps()
test into more explicit methods. The current tests are failing there, possibly because of some bleed-over with register_meta and the auth filter.
Replying to helen:
To help figure out the best path, what is it that people are doing with non-core object types and what could go wrong if it wasn't restricted?
I don't know if we can consider it something gone wrong, but we'll be opening things up to more possibilities while we're still trying to figure this out. That's probably not a bad thing as long as we're aware of it and communicate intent well. Adding a filter here may help in the meantime.
#96
@
8 years ago
In 35658.30.diff, make sure $object_subtype
is defined before attempting to use it.
In 35658.31.diff, we do not need to pass $meta_key
to get_metadata()
as it is empty.
In 35658.32.diff, add initial tests for register_meta()
.
#98
@
8 years ago
Bringing this in from the changeset that didn't reference the ticket properly
In 37991:
(The changeset message doesn't reference this ticket)
#99
@
8 years ago
35658.35.diff refreshes register_meta()
tests to expect a WP_Error
rather than false
This ticket was mentioned in Slack in #core by jeremyfelt. View the logs.
8 years ago
#101
@
8 years ago
@ocean90 brought up a good point about the type
argument for a registered meta key. Right now we have no restriction, so developers could register with int
, integer
, bool
, boolean
, etc...
At some point, we may want to allow a broad list so that custom data types can be supported with something like JSON Schema, but for now it seems we have 2 options:
- Allow ambiguity. We can suggest in the dev notes that developers use proper type names like
boolean
andinteger
, but keep the list open to allow for easy experimentation. - Restrict to a specific list of data types and possibly filter that list.
I'm leaning toward 1. :)
#104
in reply to:
↑ 95
@
8 years ago
Replying to jeremyfelt:
unregister_meta_key()
needs to remove the filters thatregister_meta()
has added. Need tests for that as well.
35658.36.diff removes filters when a meta key is unregistered (with tests).
#105
@
8 years ago
As @sc0ttkclark mentioned earlier in #core, something like register_meta( 'post', 'my_key', null, 'auth_callback' );
does not currently work as expected. Instead of adding the old auth filter and skipping the old sanitize filter, it adds no filter and returns a WP_Error
about not providing an object subtype.
35658.37.diff treats the old auth callback argument the same as the old sanitize callback argument. If it is provided and callable, then we should ignore that any array of arguments was passed. No tests yet for this.
#106
@
8 years ago
https://core.trac.wordpress.org/attachment/ticket/35658/35658.37.2.diff does the same as the previous of it's version, however it just double checks that auth/sanitize hooks is_callable()
before adding the back-compat filters.
#110
@
8 years ago
35658.38.diff is an iteration on 35658.37.2.diff and includes tests for old style register_meta()
uses to make sure the proper filter is added:
register_meta( 'post', 'key', null, 'auth_callback' );
register_meta( 'post', 'key', 'sanitize_callback' );
register_meta( 'post', 'key', 'sanitize_callback', 'auth_callback' );
#112
follow-ups:
↓ 113
↓ 114
@
8 years ago
add_metadata()
does not currently support an $object_subtype
parameter. A new style sanitize filter will not be triggered because add_metadata()
calls sanitize_meta()
without the object subtype as well. We need to add subtype into this part of the process. 35658.39.diff does this, though 6 parameters on add_meta()
seems like it's stretching it. :/
#113
in reply to:
↑ 112
@
8 years ago
Replying to jeremyfelt:
add_metadata()
does not currently support an$object_subtype
parameter. A new style sanitize filter will not be triggered becauseadd_metadata()
callssanitize_meta()
without the object subtype as well. We need to add subtype into this part of the process. 35658.39.diff does this, though 6 parameters onadd_meta()
seems like it's stretching it. :/
This is also the case for update_metadata()
and update_metadata_by_mid()
, covered in https://gist.github.com/jeremyfelt/92c5f4f51716469d4d18582e29bddf65 because I can't get Trac to let me upload a file. :)
#114
in reply to:
↑ 112
@
8 years ago
Replying to jeremyfelt:
add_metadata()
does not currently support an$object_subtype
parameter. A new style sanitize filter will not be triggered becauseadd_metadata()
callssanitize_meta()
without the object subtype as well. We need to add subtype into this part of the process. 35658.39.diff does this, though 6 parameters onadd_meta()
seems like it's stretching it. :/
This makes the Object_Type and colon-separated object-type schema sound even more appealing. Now multiply this X every function/filter/etc where you now have to take into account a sub-type, or possibility for a sub-type. Retrofitting the existing type parameter w/ optional colon-separated-sub-type makes a ton of sense to me.
#115
in reply to:
↑ 95
;
follow-up:
↓ 117
@
8 years ago
Replying to jtsternberg:
Retrofitting the existing type parameter w/ optional colon-separated-sub-type makes a ton of sense to me.
This has become a lot more appealing to me in the last 24 hours. I've attached 35658.44.diff, which starts to make that conversion. The handling of $object_type
into a base/sub type is a little rough, but all tests are passing.
register_meta( 'post:post', 'my_meta_key' );
Replying to jeremyfelt:
- We should consider removing
object_subtype
key from the meta key array. It doesn't really serve a purpose as its used to build the array containing the meta key instead.
This is another side effect of collapsing the array, which makes me happy as well.
This ticket was mentioned in Slack in #core by ocean90. View the logs.
8 years ago
#117
in reply to:
↑ 115
;
follow-up:
↓ 118
@
8 years ago
Replying to jeremyfelt:
This has become a lot more appealing to me in the last 24 hours. I've attached 35658.44.diff, which starts to make that conversion. The handling of
$object_type
into a base/sub type is a little rough, but all tests are passing.
register_meta( 'post:post', 'my_meta_key' );
I'm curious why we need to remove the object_subtype
key from the array. Couldn't we support both ways of specifying the subtype? We could have the CURIE notation take precedence, but if someone doesn't specify it, but specifies the array key, we could still have it work.
As we are in Beta, it's a common thing to change something like this, but I'm worried especially because this first way of doing it has already been made public in the dev note. I've personally updated one of my plugins to use that syntax, and although I could change it again of course, I'm pretty sure a few others have too. Therefore I think we should work in a backwards-compatible way here - I don't see why that would cause problems in this case.
Actually, I think it might be useful to add wrapper functions for register_meta()
for a specific object type. These functions could look like register_post_meta( $post_type, $meta_key, $args )
or register_term_meta( $taxonomy, $meta_key, $args )
and would align with other meta wrapper functions that developers regularly use - that's something else though, let's focus on what register_meta()
will look like first.
Sorry for all these points in one comment, but I wanted to get this off my chest, especially since I won't be able to attend tomorrow's meeting. :(
#118
in reply to:
↑ 117
;
follow-up:
↓ 119
@
8 years ago
Replying to flixos90:
I'm curious why we need to remove the
object_subtype
key from the array. Couldn't we support both ways of specifying the subtype?
I'd prefer providing one way of doing it for 4.6. Otherwise we're looking at more code and more tests for a piece that we're still working through.
As we are in Beta, it's a common thing to change something like this, but I'm worried especially because this first way of doing it has already been made public in the dev note.
While not ideal, I think we're okay to change directions in beta at some level. I'd hope that plugin devs are testing and following the changes, but not actually shipping new versions of plugins until at least RC1.
Actually, I think it might be useful to add wrapper functions for
register_meta()
for a specific object type. [...] that's something else though, let's focus on whatregister_meta()
will look like first.
Yeah, I think there are several possibilities for the future. It's also possible that an object similar to the one discussed in previous comments starts to be realistic. This first iteration should be as bare bones as possible while providing the benefit we need.
Sorry for all these points in one comment, but I wanted to get this off my chest, especially since I won't be able to attend tomorrow's meeting. :(
No worries. These are great points, thanks for chiming in here!
#119
in reply to:
↑ 118
@
8 years ago
Replying to jeremyfelt:
Replying to flixos90:
I'm curious why we need to remove the
object_subtype
key from the array. Couldn't we support both ways of specifying the subtype?
I'd prefer providing one way of doing it for 4.6. Otherwise we're looking at more code and more tests for a piece that we're still working through.
I was working on 35658.45.diff before reading your response. It's just 4 additional lines of code (1060-1063) to keep it compatible. :) Another thing I adjusted in this patch is remove the object_subtype
key from the doc block.
This ticket was mentioned in Slack in #core by jeremyfelt. View the logs.
8 years ago
#121
follow-up:
↓ 130
@
8 years ago
35658.47.diff is a new iteration after the discussion today.
Highlights:
The word "subtype" has basically been eliminated thanks to CURIE syntax. :heart:
I think we're able to maintain back-compat filters and have the new filters by just using $object_type
and not worrying about logic to separate them.
The CURIE string flows pretty well through all existing _metadata()
functions, though we do need to strip it down to find the table and column names. This is very light weight.
Tests are still passing.
We shoulds:
I think we should ditch wp_object_type_exists()
and just allow things to work as they do with good documentation on expectations. /cc @helen
While the CURIE string flows well, several actions will end up changing to actionname_post:post_etc
by default without stricter stripping of the second piece. I *think* this is okay, but more thought is needed.
To retrieve individual registered meta for a custom post type and have it pass through registered sanitize/auth callbacks, a developer would need to use get_metadata( 'post:books', .... )
instead of get_post_meta()
*unless* we add another parameter for subtype to get_post_meta()
.
Revisit the error messages and return values. I may agree with @rmccue in #37345 that doing_it_wrong()
is better here, though I do like having WP_Error
to test against. (Can we test a doing it wrong?)
#122
@
8 years ago
With the latest patch, if meta is registered with register_meta( 'post', 'key', array( 'sanitize_callback' => 'etc' ) );
, then add_post_meta()
would not run the sanitize_post:post_meta_key
filter, even though registration handled it fine. Instead, add_metadata( 'post:post' .... )
would be needed. This is due to the automatic fallback for core object types and is something we need to figure out. :)
#123
follow-up:
↓ 124
@
8 years ago
Working on my first pass for WP_Curie -- here's a Gist of what I came up with after chatting with @MikeSchinkel:
https://gist.github.com/sc0ttkclark/1264c66297190548d138e785e39ab3d6
Still need to review it and make sure it's doing what we want/need it to do. But right now, it can parse a string and do what it needs to do. Open to feedback!
#124
in reply to:
↑ 123
@
8 years ago
Replying to sc0ttkclark:
Working on my first pass for WP_Curie -- here's a Gist of what I came up with after chatting with @MikeSchinkel:
https://gist.github.com/sc0ttkclark/1264c66297190548d138e785e39ab3d6
Still need to review it and make sure it's doing what we want/need it to do. But right now, it can parse a string and do what it needs to do. Open to feedback!
I thought it might be useful for this class to be flexible to support other use-cases than "type" and "subtype" for the future, so I worked on such an approach, based on your class:
https://gist.github.com/felixarntz/a678b7913e268111fda0666824fe4b31
This allows to set flexible keys (here it's "type" and "subtype"), for example in a derivated class. We could also go a little further and make the class and the set_keys()
method abstract, and then we would introduce another class named something like WP_Object_Type_Curie
that implements that method.
#125
follow-up:
↓ 129
@
8 years ago
If you want to provide a class for CURIEs, you should definitely stick to how the CURIE is officially defined. Otherwise, please don't call it a CURIE, as that comes with a certain set of expectations.
From https://www.w3.org/TR/curie/ :
The following definition makes the set of CURIEs a syntactic superset of the set of QNames. It is comprised of two components, a prefix and a reference. The prefix is separated from the reference by a colon (:). It is possible to omit both the prefix and the colon, or to omit just the prefix and leave the colon. To disambiguate a CURIE when it appears in a context where a normal [URI] may also be used, the entire CURIE is permitted to be enclosed in brackets ([, ]).
This means that you would need to change the code with the following in mind:
- The components are called
prefix
andreference
- It should still work when you omit the
prefix
and leave the colon - It should be possible to enclose it in brackets (this is called a
SafeCURIE
).
The currently proposed code snippets fail on some or all of these points.
This ticket was mentioned in Slack in #core by schlessera. View the logs.
8 years ago
#127
follow-up:
↓ 133
@
8 years ago
Naming things is tough. :) CURIE makes sense (IMO) to describe 'string:string', but we should probably stick to "colon delimited string".
That aside, @rmccue had some good feedback in Slack earlier today that kept the wheels turning away from colon delimited strings. /cc @danielbachhuber
One of the first comments on this ticket (me), stressed the importance of uniqueness based on our discussion at A Day of REST. It's worth rethinking the importance of this uniqueness.
I'm using the REST API here as it's our best example of how this will be used.
If one developer registers a meta key of rating
for a custom post type and sets show_in_rest
to false
and another developer registers a meta key of rating
for another custom post type and sets show_in_rest
to true
, does this conflict matter?
In the current defensive approach, it does matter. Private information is not accidentally exposed as public in an endpoint.
However, we do have situations like this throughout core where conflicting slugs or key names can cause problems. We don't always solve for every possibility.
In another approach, the onus is on the developer to choose a unique key name, especially for private data. This would allow us to skip all of the subtype handling and just start handling registered meta without adjusting filters or the function signatures of many _meta*()
functions.
Thoughts?
This ticket was mentioned in Slack in #core by jeremyfelt. View the logs.
8 years ago
#129
in reply to:
↑ 125
;
follow-ups:
↓ 131
↓ 132
@
8 years ago
Replying to schlessera:
If you want to provide a class for CURIEs, you should definitely stick to how the CURIE is officially defined. Otherwise, please don't call it a CURIE, as that comes with a certain set of expectations.
- The components are called
prefix
andreference
- It should still work when you omit the
prefix
and leave the colon
I've changed terminology in my class to use prefix/reference. It already worked if you didn't have a prefix (or reference) since it split by colon during handling.
- It should be possible to enclose it in brackets (this is called a
SafeCURIE
).
I'm not sure we need SafeCURIE coverage just yet, since it's only used when it's being used in context of a URI, and we haven't gotten there (yet?).
RE: Felix, I'm not sure we are straying from the CURIE standard and supporting multiple colon references separately, It's the prefix, and then whatever comes after the colon will be treated as the reference for now.
Still open to further feedback.
https://gist.github.com/sc0ttkclark/1264c66297190548d138e785e39ab3d6
#130
in reply to:
↑ 121
@
8 years ago
Replying to jeremyfelt:
To retrieve individual registered meta for a custom post type and have it pass through registered sanitize/auth callbacks, a developer would need to use
get_metadata( 'post:books', .... )
instead ofget_post_meta()
*unless* we add another parameter for subtype toget_post_meta()
.
Any reason that get_post_meta()
cannot effectively be:
function get_post_meta( $post_type, ... ) return get_metadata( "post:{post_type}", .... ); }
#131
in reply to:
↑ 129
@
8 years ago
Replying to sc0ttkclark:
RE: Felix, I'm not sure we are straying from the CURIE standard and supporting multiple colon references separately, It's the prefix, and then whatever comes after the colon will be treated as the reference for now.
Totally makes sense now. I didn't know about about the CURIE standard tbh :o
#132
in reply to:
↑ 129
@
8 years ago
Replying to sc0ttkclark:
Still open to further feedback.
I would like to advocate that WP_Curie
instances be immutable and that they be dispensed from a factory class that checks to see if one exists for the prefix/reference yet and returns it if so or instantiates it and keep track of it if not.
My former WP_Object_Type
class used this approach in 35658.17.diff.
#133
in reply to:
↑ 127
@
8 years ago
Any WP_Curie
class should be handled in another ticket. We aren't going that far down the path as part of this solution, even if we do stick with colon delimited strings.
Separately from the CURIE discussion, feedback is needed on this:
Replying to jeremyfelt:
In another approach, the onus is on the developer to choose a unique key name, especially for private data. This would allow us to skip all of the subtype handling and just start handling registered meta without adjusting filters or the function signatures of many
_meta*()
functions.
Thoughts?
#134
@
8 years ago
35658.48.diff registers meta based on the object type and does not do anything to guarantee uniqueness across object subtypes.
- Removes
wp_object_type_exists()
so thatregister_meta()
works alongside the other pieces of the meta API. - Removes
$object_subtype
throughout. - Alters the return type of
register_meta()
to boolean. Successful registration will returntrue
. Old style arguments will result infalse
. - Removes the
sanitize_{$object_type}_{$object_subtype}_meta_{$meta_key}
andauth_{$object_type}_{$object_subtype}_meta_{$meta_key}
filters. Without subtype, these are unnecessary. - Alters the return type of
unregister_meta_key()
to boolean. - Removes
WP_Error
as a possible return type forget_registered_metadata()
. - Adjusts tests accordingly.
Not tracking subtype in an initial iteration simplifies things significantly. This also leaves us open for other future approaches—colon delimited strings, some sort of WP_Object_Type
, a more thorough approach, etc...
This ticket was mentioned in Slack in #core by jeremyfelt. View the logs.
8 years ago
#136
follow-up:
↓ 137
@
8 years ago
Nice work in 35658.48.diff. register_meta()
still has a comment which references "subtype" (Global registry only contains meta keys registered in the new way with a subtype.) and Tests_Meta_Register_Meta::test_register_meta_with_valid_object_type_and_object_subtype_returns_true
should probably be renamed.
Do we want to add support for "some sort of WP_Object_Type
" now? We'd only have to cast the $object_type
to a string:
<?php class Foo { function __toString() { return 'foo:bar'; } } $foo = new Foo(); $a[ $foo ] = 'a'; // Illegal offset type $a[ (string) $foo ] = 'b'; // array(1) { ["foo:bar"]=> string(1) "b" }
#137
in reply to:
↑ 136
@
8 years ago
Replying to ocean90:
Nice work in 35658.48.diff.
register_meta()
still has a comment which references "subtype" (Global registry only contains meta keys registered in the new way with a subtype.) andTests_Meta_Register_Meta::test_register_meta_with_valid_object_type_and_object_subtype_returns_true
should probably be renamed.
35658.49.diff cleans up this and a couple other subtype remainders.
Do we want to add support for "some sort of
WP_Object_Type
" now? We'd only have to cast the$object_type
to a string:
I think we should leave it out for now. We've had trouble defining exactly what we want from object type/subtype, so it's probably best to keep our options open.
This ticket was mentioned in Slack in #core by sc0ttkclark. View the logs.
8 years ago
#142
follow-up:
↓ 143
@
8 years ago
@ocean90 I moved wp_parse_args()
after the filter in 35658.51.diff rather than add an empty check. What do you think?
#143
in reply to:
↑ 142
;
follow-up:
↓ 144
@
8 years ago
Replying to jeremyfelt:
It would be consistent with register_taxonomy()
and register_post_type()
. With this change we also define that our $defaults
are always part of $args
and can't be removed, which seems to be OK?
#144
in reply to:
↑ 143
@
8 years ago
Replying to ocean90:
It would be consistent with
register_taxonomy()
andregister_post_type()
. With this change we also define that our$defaults
are always part of$args
and can't be removed, which seems to be OK?
I think so. We should be able to rely on the default arg keys being available. Anyone wanting to remove a callback (or other argument) can just set it back to something non callable.
#147
follow-up:
↓ 149
@
8 years ago
So, why are we introducing yet another global, $wp_meta_keys
, instead of using a registry class of some sort?
A registry with some kind of register
/get
/check
/has_collision
, etc, would be hugely beneficial IMO, and we can keep the array protected. Also protects some of the workings of the internal API as we can keep the methods protected, which means we won't have as much back-compat to worry about as we progress with the register_meta
api.
#148
@
8 years ago
Personally, my hope is that we'll introduce the Fields API in a formal way for WP 4.7 and the suggestion not to use the global variable gives us a solution in the interim while letting things move forward.
#149
in reply to:
↑ 147
@
8 years ago
Replying to jtsternberg:
So, why are we introducing yet another global,
$wp_meta_keys
, instead of using a registry class of some sort?
Turns out, I'm just parroting Eric's earlier questions/observations: https://core.trac.wordpress.org/ticket/35658#comment:25
Exploring something like
function register_meta( $meta_type, $meta_key, $args )
would be interesting.Too bad I'm missing the hack day :-)