Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#38926 closed defect (bug) (wontfix)

REST API: Validate thumbnail ID in response

Reported by: iseulde's profile iseulde Owned by:
Milestone: Priority: normal
Severity: minor Version: 4.7
Component: REST API Keywords: close
Focuses: Cc:

Description (last modified by iseulde)

I'm not sure if this is expected or not, but I'll report it because it seems unexpected to me.

Maybe this is also not really the problem of the REST API, but it affects it regardless.

If the post has a thumbnail ID that is invalid (e.g. the post was imported with invalid IDs), it is still returned by the API. If linked resources are embedded, there will be a 404 error inside the response.

Of course when someone uses the API, they should check for errors in the embeds, but it would be nice if linked resources don't 404.

has_post_thumbnail doesn't seem to check this either...

Change History (7)

#1 @iseulde
8 years ago

  • Description modified (diff)

#2 @joehoyle
8 years ago

Good question - I'm not totally sure of the behaviour too. Perhaps we could not link to it if it's not found, but still put the value in the response. As WordPress actually just lets you store any ID in the field, it's plausible that people could be using it for something other than an attachment ID I guess.

This is part of a broader question that has come up before: what do we do with data that is already stored, that is technically not valid. Ignoring the data from the REST API output also makes round-trips of that data very difficult, as we have to make a lot of exceptions on data validation.

I'm tempted to err on the side of a clean API, rather than misconfigured data, though that's probably a controversial view. If there's an invalid ID stored in thumbnail_id, the API would output it as null, if you updated the resource, it would then set the thumbnail ID to null.

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


8 years ago

#4 @helen
8 years ago

  • Milestone changed from Awaiting Review to Future Release

Doesn't seem critical for 4.7 - moving to future release for the team to decide what to do.

#5 @rachelbaker
8 years ago

  • Keywords close added

Thank you @helen for moving this out of the 4.7 milestone.

Do we want to fix this at all?? This sounds like an example of "garbage in, garbage out" to me - and where my views differ from @joehoyle.

Our current behavior when it comes to related object ids is return what is in the database on GET requests, and validate on create/update.
Adding validation logic for GET requests could have a performance impact (especially on collection requests on large sites) because we would have to add additional queries to look up these objects. This wouldn't only impact looking up attachment posts either, we would have to apply the same validation to the author and parent ids of posts, the post id of comments, the term ids of posts, the list goes on.

I don't think the added cost is worth the perceived benefits, but willing to let my opinion be overruled.

#6 @joehoyle
8 years ago

Do we want to fix this at all?? This sounds like an example of "garbage in, garbage out" to me - and where my views differ from @joehoyle.

I'm fine with Garbage In, Garbage Out too - I think generally we should try to stick to one of these principles. This can make validation harder though, what is there is a string in the _thumbnail_id field? Personally I don't think we should be so worried about those edgecases, but it's probably just worth us saying (if we do so think): The rest api will act weirdly and have issues if data that can technically be stored in an "incorrect" way.

#7 @joehoyle
8 years ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from new to closed

It seems the status-quo here is to stick with garbage-in, garbage-out. Changing the data for output also means the data likely being pushed back to the database too (from update requests) so I'm going to keep this as "no cleaning garbage" and close this out.

Note: See TracTickets for help on using tickets.