WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 5 weeks ago

#39959 closed defect (bug) (fixed)

Clarify parameters for callbacks for `register_rest_field()`

Reported by: flixos90 Owned by: TimothyBlynJacobs
Milestone: 5.6 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords:
Focuses: Cc:

Description

There is an issue going on with register_rest_field() in combination with the WP_REST_Meta_Fields classes: The method WP_REST_Meta_Fields::register_field() uses callbacks that accept an object ID only. However, the REST controller methods that handle the additional fields always pass an object to these callbacks.

Working with metadata via the REST API is still possible now, since the WP_REST_Meta_Fields::register_field() method is never used in Core, instead metadata is handled manually by each controller. So it is now a minor issue, but I think we should figure out what kind of parameters the register_rest_field() callbacks can accept (I could not find anything clear in the documentation) and then actually use WP_REST_Meta_Fields::register_field(), as I don't think there's a point in handling meta manually if we have this method already available in a more abstracted way.

Furthermore, this inconsistency gets more critical once developers start writing their own endpoints for custom object types that use their own metadata table. One might assume that the WP_REST_Meta_Fields base class can be leveraged to handle meta automatically, but because of the inconsistencies now, doing this would likely cause fatal errors and confusion.

Change History (13)

#1 @flixos90
4 years ago

  • Version set to 4.7

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


4 years ago

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


4 years ago

#4 follow-up: @flixos90
4 years ago

To give a precise example: https://core.trac.wordpress.org/browser/trunk/src/wp-includes/rest-api/endpoints/class-wp-rest-controller.php#L432 passes an object to the update_callback passed to register_rest_field(), but https://core.trac.wordpress.org/browser/trunk/src/wp-includes/rest-api/fields/class-wp-rest-meta-fields.php#L124 which is passed to register_rest_field() as an update_callback accepts an object ID only. So which of the two is right? Should register_rest_field() callbacks accept an object or an ID?

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


4 years ago

#6 @kadamwhite
4 years ago

My sense is that they should be allowed to accept objects, for consistency with what we're doing now; but other parts of WP generally pass IDs. @rmccue Can you speak to why this class method is not being leveraged by the native endpoints?

#7 in reply to: ↑ 4 @jnylen0
4 years ago

Replying to flixos90:

So which of the two is right? Should register_rest_field() callbacks accept an object or an ID?

They should accept an object. See the first code example at https://developer.wordpress.org/rest-api/extending-the-rest-api/modifying-responses/.

It sounds like the unused WP_REST_Meta_Fields::register_field() method was never tested, and it should be updated to accept an object. Also, we need to improve the register_rest_field documentation to indicate the types/signatures of the values accepted by its $args argument.

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


3 years ago

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


17 months ago

#10 @TimothyBlynJacobs
17 months ago

  • Owner set to TimothyBlynJacobs
  • Status changed from new to assigned

Going to take a look at this and how it relates to #44432.

#11 @TimothyBlynJacobs
6 months ago

I think it makes sense to deprecate WP_REST_Meta_Fields::register_field() because we can't really go from an object to an id value cleanly.

We could update the update_callback to do something like this...

register_rest_field(
        $this->get_rest_field_type(),
        'meta',
        array(
                'get_callback'    => function( $prepared, $field_name, $request ) {
                        return $this->get_value( $prepared['id'], $request );
                },
                'update_callback' => function( $data, $object ) {
                        if ( $object instanceof WP_Post ) {
                                $id = $object->ID;
                        } elseif ( $object instanceof WP_User ) {
                                $id = $object->ID;
                        } elseif ( $object instanceof WP_Comment ) {
                                $id = $object->comment_ID;
                        } elseif ( $object instanceof WP_Term ) {
                                $id = $object->term_id;
                        } else {
                                // now what?
                        }

                        return $this->update_value( $data, $id );
                },
                'schema'          => $this->get_field_schema(),
        )
);

But that only works for built in objects which wouldn't use register_rest_field anyways. And doesn't do anything for non-core meta types that might want to actually leverage the register_rest_field.

There is also no guarantee for the get_callback that the id property exists in the response data for the custom endpoint.

So from my perspective, the current API methods are the most flexible way to interact with the meta fields helper and trying to make register_field work will end up being less clean than the current API.

#12 @TimothyBlynJacobs
5 weeks ago

  • Milestone changed from Future Release to 5.6

#13 @TimothyBlynJacobs
5 weeks ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 49295:

REST API: Deprecate WP_REST_Meta_Fields::register_field().

This method never worked properly and cannot be fixed due to incompatible method signatures.

Props flixos90, kadamwhite, jnylen0, TimothyBlynJacobs.
Fixes #39959.

Note: See TracTickets for help on using tickets.