Make WordPress Core

Opened 6 years ago

Last modified 4 years ago

#46238 new enhancement

REST API: Allow conditional field registration

Reported by: dmsnell's profile dmsnell Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: REST API Keywords: needs-patch needs-unit-tests
Focuses: Cc:

Description

register_rest_field() allows us to add additional fields to API call responses but it doesn't allow us to do so conditionally. in prior art we add a field and then remove it later when it's not needed.

why would inclusion of a field be conditional? what is wrong with returning null?

we came across this when wanting to add metadata to video files, which are post types and conflated with posts and other media attachments. we'd like to be able to just add fields to video file media requests and not pollute the responses for all of the other "subtypes." we'd also like to do it without resorting to surprising behaviors like adding and later removing them.

in this ticket I'd like to propose two approaches to accomplishing this goal. the first is a bit clever but should make it possible to extend existing behaviors without breaking any code and without introducing further confusion on how to add fields. it uses a "sentinel value" passed into the get_callback() function which can be returned to indicate that the field should not exist.

the second approach creates a new function register_conditional_rest_field() and provides a wrapped response format to indicate whether the field should be added. this approach is less idiomatic in my opinion as we're not used to returning wrapped values.

---

I thought for sure I'd seen a discussion around this before but I couldn't find any tickets with my search. that being said I'm curious why this doesn't already exist as it seems like it could be a common need, though I'm not sure how the fact that media attachments are a bit odd in comparison to custom post types comes into play.

Attachments (1)

46238a.diff (1.9 KB) - added by dmsnell 6 years ago.
add-the-sentinel approach

Download all attachments as: .zip

Change History (8)

@dmsnell
6 years ago

add-the-sentinel approach

#1 @mmtr86
6 years ago

Thanks for opening this ticket @dmsnell!

we came across this when wanting to add metadata to video files, which are post types and conflated with posts and other media attachments

For context, this is the GitHub PR where we adding this behavior. I'd like to clarify that we were actually adding the new REST field only to the media attachments with register_rest_field( 'attachment', ... ), so the posts were not affected. But indeed, this is adding the field to all the attachments and we only want it on video attachments.

it uses a "sentinel value" passed into the get_callback() function which can be returned to indicate that the field should not exist.

I like how this approach is backwards compatible with the existing behavior, and your suggested implementation looks elegant and simple to me.

I wonder however what implications this may cause to the schema. Since we cannot conditionally add the field to the schema, it could present an inconsistency:

  • When the field is declared on the schema but not present on the response.
  • When the field is undeclared on the schema but present on the response.

On the previous PR, we opted for declaring the field on the schema. This doesn't cause any failure to the responses not containing the field, but the inconsistency between schema and response is there.

I'm not familiar with API schemas, but maybe there is a way to define a field as optional so we make the schema to always match the response.

#2 @dmsnell
6 years ago

thanks for the feedback @mmtr86

I wonder however what implications this may cause to the schema.

I did some checking to see if I could explore your prompt more. in the process I noticed an oddity and a salient point about schema. I also noted that it was difficult for me to find clear documentation on the schema side of the API - particularly to understand how this schema fits in with the overall shape of the response

for one, I learned that I should be able to query the schema with a GET call to the API base…

curl 'http://localhost/wp-json/wp/v2/' | jq '.routes | ."/wp/v2/media/(?P<id>[\\\\d]+)" | .endpoints[0]'
{
  "methods": [
    "GET"
  ],
  "args": {
    "id": {
      "required": false,
      "description": "Unique identifier for the object.",
      "type": "integer"
    },
    "context": {
      "required": false,
      "default": "view",
      "enum": [
        "view",
        "embed",
        "edit"
      ],
      "description": "Scope under which the request is made; determines fields present in response.",
      "type": "string"
    }
  }
}

…but the schema for my fields didn't come though. interestingly enough they come through for the update endpoints.

curl 'http://localhost/wp-json/wp/v2/' | jq '.routes | ."/wp/v2/media/(?P<id>[\\\\d]+)" | .endpoints[1]'
{
  "methods": [
    "POST",
    "PUT",
    "PATCH"
  ],
  "args": {"vp_guid": {
      "required": false,
      "description": "Some GUID",
      "type": "string"
    },
    …
  }
}

It took me a while to figure out how to query the schema. This we do with an OPTIONS request.

curl -X OPTIONS 'http://localhost/wp-json/wp/v2/media/5' | jq '.schema .properties'
{"vp_guid": {
    "description": "Some GUID",
    "type": "string",
    "context": [
      "view",
      "edit"
    ],
    "required": false
  },
  …
}

If I interpret the entire response schema as a JSON-Schema then I think it means that we're covered. Without that explicit required => false indicator it won't show up as not required, but at the same time my understanding of JSON-Schema is that unless a field is explicitly indicated as required then it's not required.

In other words I don't think this carries any implications on the schema unless we've marked the field as required => true. If I take into consideration that it's less obvious how to even get the GET schema it seems to mark us even more safe. I don't understand the implications of the update endpoints though.

#3 @mmtr86
6 years ago

Wonderful, thanks for confirming @dmsnell!

#4 @dominic_ks
4 years ago

Hello all,

I could really use this functionality and am happy to provide a refreshed patch unless there is any reason that this change shouldn't go ahead?

I note the concerns about differences between schema and results of an actual request, happy to check if there's anything that can be done here due to any changes in the last 16 months.

Cheers,

#5 @TimothyBlynJacobs
4 years ago

  • Keywords needs-patch needs-unit-tests added

I can see how this is useful, but I would rather see this implemented as an optional skip_callback or include_callback, WordPress doesn't really use a sentinel pattern at all and I think a callback approach is much more WordPressy.

#6 @dominic_ks
4 years ago

Cheers for commenting on this @TimothyBlynJacobs. Re: the two approaches you mentioned, to check I understand:

Are you suggesting here that an extra param could be passed to register_rest_field that is then called to see if the field should be included or skipped for the current post? If so this could be relatively straight forward to add this and check it in add_additional_fields_to_object.

There's mention here of the schema, do you think anything is required here to highlight that a particular field may not be included in a response?

If I've understood correctly here I will prepare a patch and come back with it

#7 @TimothyBlynJacobs
4 years ago

@dominic_ks Yep, by adding it to the $args array.

I’m not sure there is a JSON Schema keyword that would make sense to indicate this.

Note: See TracTickets for help on using tickets.