Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Currently Exception responses are handled and created in the RequestHandler class. This is a tight coupling, it is hard to change the request handling for JSON API routes. (Almost) the same code to generate exception responses is in three different places in this class.
Proposed resolution
Use a decoupled solution that uses KernelEvents::EXCEPTION.
Remaining tasks
Discuss the solution direction.
Write tests
Finalize the solution
Consider moving other operations to an event subscriber too, e.g. serialization (follow-up issue).
API changes
Add an event subscriber.
Comment | File | Size | Author |
---|---|---|---|
#14 | 2810293--exception-handling--14.patch | 9.01 KB | e0ipso |
#11 | interdiff-2810293-5-8.txt | 3.76 KB | Sutharsan |
#5 | jsonapi-exception-event-2810293-4.patch | 7.54 KB | Sutharsan |
#4 | interdiff-2810293-2-4.txt | 1.92 KB | Sutharsan |
#2 | jsonapi-exception-event-2810293-2.patch | 7.16 KB | Sutharsan |
Comments
Comment #2
Sutharsan CreditAttribution: Sutharsan commentedAttached patch is to illustrate the solution direction I propose. It is not working yet.
Comment #3
e0ipsoThis is fantastic! I like where this is going, we should explore more.
Comment #4
Sutharsan CreditAttribution: Sutharsan commentedFirst working version. Time for writing tests...
Comment #5
Sutharsan CreditAttribution: Sutharsan commentedComment #8
e0ipsoPlease use the shorthand syntax
[]
All JSONAPI routes have an option flag called
_is_jsonapi
that we may want to check as well.Comment #9
e0ipsoAlso, there are some tests failing (uninitialized container?).
We will also need testing for the new subscriber. We already have test coverage for the exception serialization somewhere else, but we'll need to test the other stuff as well. Hopefully a unit test that checks the expected output for:
onException
applies
Comment #10
e0ipsoComment #11
Sutharsan CreditAttribution: Sutharsan commentedComments from #8 addressed.
Comment #12
e0ipsoThanks @Sutharsan. It seems that the actual patch is missing, there is only the interdiff :-P
Comment #13
Wim Leers+1 for the changes to
RequestHandler
.-1 for the addition of that new exception event subscriber.
\Drupal\serialization\EventSubscriber\DefaultExceptionSubscriber
already does that, in a more generic way.Comment #14
e0ipsoTaking this into an slightly different direction.
Comment #15
e0ipsoComment #17
e0ipsoFixed and merged. I decided to grant authorship to Sutharsan, because he is awesome and did a great job reporting this and taking it to near completion.
Thanks all!
Comment #18
Wim LeersWhy do you need this class?
Why does it need to run so early?
Comment #19
e0ipsoThanks for the review Wim. I should've mentioned you in the commit credits as well. Shame on me.
Because the
\Drupal\serialization\EventSubscriber\DefaultExceptionSubscriber
is opinionated on how I have to normalize my exceptions in thesetEventResponse
. Serializing an array['message' => $exception->getMessage()]
is not good enough for jsonapi.I think that core should ship with HttpException normalizers that do the
['message' => $exception->getMessage()]
, if that is the desired format, in the normalizer instead of the event subscriber.No reason, it needs to take precedence over the parent. Since the parent is at
-75
and the grandparent at0
I thought that-50
could be a sensible number. Do you think it's too early?Comment #20
Sutharsan CreditAttribution: Sutharsan commentedComment #21
Wim LeersThat makes sense! Can you open a new issue for that? We still have to clean up the current HTTP exception mess for non-HTML formats. So we might as well fix that at the same time.
Thanks for the clarifications!
Comment #22
e0ipsoComment #23
e0ipsoAdded a reference to the core issue.