Skip to content
This repository has been archived by the owner. It is now read-only.

Implement new-style meta with register_meta #15

Open
wants to merge 36 commits into
base: master
from
Open

Conversation

@rmccue
Copy link
Member

rmccue commented Sep 19, 2016

This is a first pass at new-style meta support for the REST API.

With the new register_meta support in 4.6, we can now support meta fully, as it's actually a registered thing instead of being a vague mess.

This PR includes a brand new README as well, which documents how to use this. tl;dr set show_in_rest => true in your register_meta call, then use the meta field on the regular endpoints.

Important note: this is not ready for merge yet, and still requires security checks and further validation.

@rmccue rmccue added this to the 0.2.0 milestone Sep 19, 2016
array( 'key' => $name, 'status' => rest_authorization_required_code() )
);
}

This comment has been minimized.

Copy link
@websupporter

websupporter Sep 27, 2016

Member

I did just run into a problem, I think a lot of devs could run into. When I haven't registered the meta as single and want to update, I end up here. Lets say I run a POST request like this: http://example.com/wp-json/wp/v2/posts/1177?meta[test]=just-a-string
$values would be a string here, but needs to be an array. My workaround was on L170:

        if ( ! is_array( $values ) ) {
            $values = array( $values );
        }
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 28, 2016

Current coverage is 100% (diff: 100%)

No coverage report found for master at 70cdd51.

Powered by Codecov. Last update 70cdd51...d2c4126

@rmccue

This comment has been minimized.

Copy link
Member Author

rmccue commented Sep 29, 2016

Coverage is now at 100%, ready for review I think!

@@ -10,9 +10,9 @@ matrix:
- php: 5.6
env: WP_TRAVISCI=travis:phpunit WP_VERSION=nightly
- php: 5.6
env: WP_TRAVISCI=travis:phpunit WP_VERSION=latest

This comment has been minimized.

Copy link
@joehoyle

joehoyle Sep 30, 2016

We'll still need the tests to work with latest when this is merged to core for wp-api?

This comment has been minimized.

Copy link
@rmccue

rmccue Sep 30, 2016

Author Member

I think we should drop support for 4.6, IMO

}
/**
* Get the settings.

This comment has been minimized.

Copy link
@joehoyle

joehoyle Sep 30, 2016

Think this docblock might be incorrect?

}
/**
* Update settings for the settings object.

This comment has been minimized.

Copy link
@joehoyle

joehoyle Sep 30, 2016

Docblock needs updating.

* Update settings for the settings object.
*
* @param WP_REST_Request $request Full detail about the request.
* @return WP_Error|array

This comment has been minimized.

Copy link
@joehoyle

joehoyle Sep 30, 2016

This return looks wrong.

'prepare_callback' => 'meta_rest_api_prepare_value',
);
$default_schema = array(
'type' => null,

This comment has been minimized.

Copy link
@joehoyle

joehoyle Sep 30, 2016

It might be simpler to set the default type to $args['type'], as you wouldn't then need the double check below.

This comment has been minimized.

Copy link
@rmccue

rmccue Sep 30, 2016

Author Member

Can't do that, as then we can't check if the type was overridden in the user-specified schema below.

$default_args = array(
'name' => $name,
'single' => $args['single'],

This comment has been minimized.

Copy link
@joehoyle

joehoyle Sep 30, 2016

Is args['single'] always set?

This comment has been minimized.

Copy link
@rmccue

rmccue Sep 30, 2016

Author Member

Yep, it has a default in register_meta

$fields = $this->get_registered_fields();
$schema = array(
'description' => __( 'Post meta fields.' ),

This comment has been minimized.

Copy link
@joehoyle

joehoyle Sep 30, 2016

I think this should be Meta fields, as it's used on comments etc too. Also, CPTs are not really called posts in the API.

} else {
$rest_args['schema']['type'] = 'array';
$rest_args['schema']['items'] = array(
'type' => $args['type'],

This comment has been minimized.

Copy link
@joehoyle

joehoyle Sep 30, 2016

I didn't see anywhere that we are validating / casting the values of multiples based off the type => array, items => { type => 'number' } for example.

This comment has been minimized.

Copy link
@rmccue

rmccue Sep 30, 2016

Author Member

Good catch.

@tharsheblows

This comment has been minimized.

Copy link

tharsheblows commented Oct 5, 2016

I've been playing and noticed some things:

  • I can update a metakey to be anything (array, object, etc) if its registered type is not string, number or bool
  • a metavalue doesn't update or delete correctly, it would be nice to get a error (it does mostly)
  • if I try to update a metakey which was registered single=true with an array it saves it as a serialized array then throws an array to string conversion notice on GET

I haven't been thorough, not sure if that's the type of thing you're looking for.

@kadamwhite

This comment has been minimized.

Copy link

kadamwhite commented Oct 6, 2016

@tharsheblows Thanks for the feedback! We're discussing these in slack and the update function definitely needs to be tightened up, that array casting issue is definitely a bug and making the update stricter should address the first item too

@tharsheblows

This comment has been minimized.

Copy link

tharsheblows commented Oct 6, 2016

There are also a slight issue when deleting multiple identical meta_values for a given meta_key in update_multi_meta_value. So eg if you have postmeta_multiple_number: [ 11, 13, 13, 13, 13 ],

you'll get an error when you delete / update it because it tries to do all of the 13s one at a time and they all get done at once by delete_metadata I think. I had an issue when updating to multiple identical meta_values but I can't reproduce it.

I hate this rule.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants
You can’t perform that action at this time.