Problem/Motivation

An HTTP DELETE request to a JSONAPI Resource gets a 500 error response if the resource (a drupal entity) is configured by this module to not be deleted if other entities reference it.

This happens because EntityPredelete throws an error. It would be great if that error message could bubble up and be included in the JSONAPI response.

Steps to reproduce

Create entities that reference each other, configure them to not be deleted if referenced, and perform a DELETE request the JSONAPI endpoint: https://www.drupal.org/docs/8/modules/jsonapi/removing-existing-resource...

Proposed resolution

One solution would be to implement hook_entity_access, check that $operation == 'delete' and deny access to entities that have the referenced integrity constraint. This will return a 403 response with an optional AccessResultForbidden reason included in the response.

With this approach it's important to only deny access when the request is to a JSON:API resource. Otherwise the user will not have access to delete the entity in any other context - a good example is the "Delete" button on the Entity edit form which is not displayed if the user does not have access to delete the entity.

Remaining tasks

Include test coverage.

User interface changes

N/A

API changes

N/A

Data model changes

N/A

CommentFileSizeAuthor
#6 3195039-6.patch11.74 KBm.stenta
#5 3195039-5.patch11.6 KBm.stenta
Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

paul121 created an issue. See original summary.

paul121’s picture

Status: Active » Needs review

Added a fix as proposed above and included a functional test.

What's nice is that this approach does not depend on the JSONAPI module; hook_entity_access simply checks if the request is coming from the JSONAPI controller and returns otherwise. As noted in the JSONAPI classes, it is possible that their internal implementation could change and that we should not depend on its PHP code. This is why I re-implemented their logic rather than actually use their helper method which could change.

paul121’s picture

Thanks again for the review @Sam152 . I took a pass at implementing your requested changes, they make sense to me.

m.stenta’s picture

FileSize
11.6 KB

Attaching a patch for use with cweagans/composer-patches until the MR gets merged. Thanks for your work on this @paul121 and @sam152!

m.stenta’s picture

FileSize
11.74 KB

Ah, so we actually want to use both this patch AND the patch for #2831365: Make action confirm_form_route_name forms that delete entities disabled when protected, but applying them together creates a small conflict due to the use statements in entity_reference_integrity_enforce.module. So here is one more patch that can be applied AFTER applying the patch from #2831365. Again, though, disregard this in favor of the MR. :-)

  • Sam152 committed e972d97 on 8.x-1.x authored by paul121
    Issue #3195039 by paul121, m.stenta: 500 error when deleting referenced...
Sam152’s picture

Status: Needs review » Fixed

Committed, thanks.

I do wonder if perhaps it belonged in a sub-module before tagging a new release, but I don't feel that strongly about it.

m.stenta’s picture

Thanks @Sam152!! For this and #2831365!

m.stenta’s picture

@Sam152 - Any chance we can release 8.x-1.1 with these commits so that we can pin to it and remove the patches from our project?

Not urgent of course - if you have other things you'd like to include. Worth asking at least. :-)

Sam152’s picture

Status: Fixed » Closed (fixed)

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