Make WordPress Core

Opened 4 years ago

Closed 3 years ago

#49252 closed defect (bug) (wontfix)

REST API response not valid for null on 5.3.2

Reported by: jankadlec's profile jankadlec Owned by:
Milestone: Priority: normal
Severity: normal Version: 5.3
Component: REST API Keywords: reporter-feedback close
Focuses: Cc:

Description

There is one condition added that is preventing correct behaviour of REST
Expected behaviour:
when API call result is null for GET methods (where nothing is found e.g.), should be encoded as null in JSON.

Actual behaviour:
when API call result is null, empty string is returned producting EOF Error on receiving end.

in file wp-includes/rest-api/class-wp-rest-server.php line 404:

<?php
// The 204 response shouldn't have a body.
if ( 204 === $code || null === $result ) {
  return null;
}

should be fixed with:

<?php
// The 204 response shouldn't have a body.
if ( 204 === $code ) { 
  return null;
}

Change History (9)

#1 @TimothyBlynJacobs
4 years ago

  • Keywords reporter-feedback added
  • Severity changed from major to normal
  • Version changed from 5.3.2 to 5.3

Thanks for the ticket @jankadlec and welcome to trac!

Could you explain why you think that should be the case?

In the WP REST API if you are requesting a single item, and nothing is found, a 404 should be issue with an error response. If you are requesting a collection of items, and no items are found that match the request parameters, then an empty list is returned.

See #43691.

#2 @jankadlec
4 years ago

Hi @TimothyBlynJacobs

I have this REST resource: /product/2/metafield/_meta

If the product 2 is not mapped to any product, the REST API is returning 404 code - it's error, and client side of the API will tackle this as an error...

if the _meta metafield is not found on product 2, it's not error, that metafield just does not exists and null is expected with 200 code

There is no RFC that you can refer to state that is should be implemented it using 404 or returning null JSON + 200, it's just on API designer to specify this IMHO

Hardwiring this (result === null) condition you eliminate that finegrained controll of that API design and you force my client side to distinguish between "two 404" errors, treating one as non recoverable error, second as recoverable error, actually only "sort of flag, that is forcibly returned by WP design, as we have no other option to return 'not found' in soft way" - and moreover to parse 404 body to detect that type - which would break the simple API usage - 200 = OK, 404 = nonrecoverable error

Why was this introduced please?

Thank you in advance

Jan

#3 @jankadlec
4 years ago

In ticket 43691 there was asking for not sending body for 204 response code which is correct requirement IMHO.

Spreading it out to more (result === null) without any context is unwanted sideeffect of that change IMHO...

Best Regards

Jan

#4 @jankadlec
4 years ago

Hi @TimothyBlynJacobs

any update on this please?

Just I can decide if I need to parse 404 errors body or I can rely on fact that this fix will be included in some future verion of WP please?

Thank you in advance

Jan

#5 @TimothyBlynJacobs
4 years ago

  • Keywords close added

Hi @jankadlec,

Sorry for the delay here. We are taking null to mean the absence of a response here. I can see how that is unhelpful for your desired workflow. But I don't think we'll be changing it at this time. My recommendation would be to send a 204 response when the _meta field is not found.

Adding the close keyword, but if another maintainer feels differently feel free to remove.

#6 @chrisl27
4 years ago

Hi @TimothyBlynJacobs

I've found this ticket as a result of seeing "null" no longer be sent, in our case we are response.json()'ing the fetch responses from the API, which is now failing if the response has no body because JSON.parse("null") is valid, but JSON.parse("") throws. This is also a problem because the API is saying "Content-Type: application/json" but the response wasn't valid JSON - so there's not really a way (apart from try/catching) to determine whether we should expect the content to be valid JSON.

Should the fix have been && instead of || so as to only have this behaviour for 204 responses?

  if ( 204 === $code && null === $result ) {
    return null;
  }

#7 @jankadlec
4 years ago

Hi @TimothyBlynJacobs

We are taking null to mean the absence of a response here

yes, that's the problem. In PHP there is lot of overloading of null value, this is really not something that anyone asked for. It's mixing meanings of statuses and interpreting data values in general context, where you do not have enough information to extrapolate that.

In fact all what is needed is covered in underlying marshaling functions in WordPress, and it's done correctly. Why are you altering that from higher level please?

the code should be

if ( 204 === $code ) {
    return null;
  }

as RFC says that there MUST NOT be a content for 204.

Any additional "guesses" on $result beeing 0, [], null, [null, null, null] etc. are unwanted sideeffects IMHO

I'm interfacing Wordpress API for first time, but if you want someone to use Wordpress API, you need to manage it in more backward compatible way...

What about 205 http status? Shall I include it in my conditions as this will probably happen for it too?

Best Regards

Jan

#8 @jankadlec
4 years ago

Hi @TimothyBlynJacobs

The purpose of a GET method is to retrieve a resource. So, while allowed, an HTTP 204 is no the best choice since content IS expected in the response.

The RFC also specifically calls out an HTTP 204 as an appropriate response for PUT, POST and DELETE, but omits it for GET.

see https://tools.ietf.org/html/rfc2616#section-9.3

Best Regards

Jan

#9 @TimothyBlynJacobs
3 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Closing per my earlier comment:5. I'm sorry this created an issue for you, but I don't think we'll be changing this behavior again.

Note: See TracTickets for help on using tickets.