The following causes an assert error:

    $url = Url::fromRoute('commerce_cart_api.jsonapi.cart_clear', ['commerce_order' => $cart->uuid()]);
    $response = $this->request('DELETE', $url, $this->getAuthenticationRequestOptions());
Exception : AssertionError: assert($document instanceof JsonApiDocumentTopLevel)
assert()() (Line: 105)

\Drupal\jsonapi_resources\Controller\JsonapiResourceController::ensureValidResponseDataResourceTypes should properly handle DELETE / 204 requests that have no body.

CommentFileSizeAuthor
#7 3090971-7.patch3.34 KBmglaman
#6 3090971-6.patch1.97 KBmglaman
#4 3090971-4.patch3.78 KBmglaman
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mglaman created an issue. See original summary.

mglaman’s picture

We should also relax \Drupal\jsonapi_resources\Routing\ResourceRoutes::ensureResourceImplementationValid if the method is only ['DELETE']

mglaman’s picture

Title: DELETE requests do not have a response body, but ensureValidResponseDataResourceTypes still expects a JsonApiDocumentTopLevel » DELETE / HTTP 204 requests will not return a response
mglaman’s picture

Status: Active » Needs review
FileSize
3.78 KB

Here is a patch. If the response code is not 204, then we ensure the resource types. I also skip the route validation if the methods are specifically only ['DELETE']

gabesullice’s picture

Overall, I'm 👍. Just two nits:

  1. +++ b/src/Controller/JsonapiResourceController.php
    @@ -62,7 +62,9 @@ final class JsonapiResourceController {
    +    if ($response->getStatusCode() !== 204) {
    +      $this->ensureValidResponseDataResourceTypes($response, $request);
    +    }
    

    Feels like this should go inside the method and if the response is 204, we should validate that there actually isn't any content.

  2. +++ b/src/Routing/ResourceRoutes.php
    @@ -166,24 +166,26 @@ final class ResourceRoutes implements EventSubscriberInterface {
    +    if ($route->getMethods() !== ['DELETE']) {
    

    Instead of creating another level of nesting, what about an early return?

mglaman’s picture

FileSize
1.97 KB

Feels like this should go inside the method and if the response is 204, we should validate that there actually isn't any content.

😂I originally had this and went "nah". I'll go back!

Going to try and make better words for this:

throw new ResourceImplementationException('HTTP 204 responses should not have content.');
Instead of creating another level of nesting, what about an early return?

👍

mglaman’s picture

FileSize
3.34 KB

I had bad logic in that last patch, 204 response still threw an assert error.

  • mglaman committed 53d5750 on 8.x-1.x
    Issue #3090971 by mglaman, gabesullice: DELETE / HTTP 204 requests will...
mglaman’s picture

Status: Needs review » Fixed

Unblock DELETE requests!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.