The EntityResource
controller class has a deserialize()
method which takes care of building the entity object from the submitted request data.
At some point, if either UnexpectedValueException
or InvalidArgumentException
are thrown by the underlying serialized, it is catched and an UnprocessableHttpEntityException
is thrown by the method itself.
try {
...
}
// These two serialization exception types mean there was a problem with
// the structure of the decoded data and it's not valid.
catch (UnexpectedValueException $e) {
throw new UnprocessableHttpEntityException($e->getMessage());
}
catch (InvalidArgumentException $e) {
throw new UnprocessableHttpEntityException($e->getMessage());
}
The issue here is that, per its signature, UnprocessableHttpEntityException
first argument should be an Exception
object, not a string
. When one of this situation occurs, a TypeError
is thrown by PHP, breaking the execution flow and leading to a 5xx error instead of a JSON:API managed 4xx error.
Comment | File | Size | Author |
---|---|---|---|
#16 | 3052954-16.patch | 3.32 KB | Wim Leers |
#16 | 3052954-16--tests-only-FAIL.patch | 1.58 KB | Wim Leers |
#14 | 3052954-14.patch | 3.28 KB | Wim Leers |
#14 | 3052954-14--tests-only-FAIL.patch | 1.54 KB | Wim Leers |
#14 | interdiff.txt | 1.86 KB | Wim Leers |
Comments
Comment #2
garphy CreditAttribution: garphy at ICI LA LUNE commentedThis patch provides a new test case that exhibit the issue : We expect a
UnprocessableHttpEntityException
but we get a PHP error instead.Comment #3
garphy CreditAttribution: garphy at ICI LA LUNE commentedFiring testbot.
Comment #4
garphy CreditAttribution: garphy at ICI LA LUNE commentedComment #6
gabesulliceThanks @garphy!
Comment #7
Wim LeersI'd swear I have a patch for this already…
Yep, found it, over at #3042124-23: [regression] Empty response body when user is an administrator and an exception is thrown; some traces cannot be encoded because of recursion detection.. I think that's the correct solution.
If we want to go with this solution, then we'll need to update the
@throws
entry on the\Drupal\jsonapi\Controller\EntityResource::deserialize()
docs.Comment #8
Wim LeersPer #7.
Comment #9
Wim LeersComment #10
gabesulliceThanks @Wim Leers!
LGTM.Nope, see below.Comment #11
gabesulliceI lied, we need to reroll the tests in #2. They should probably be moved to
JsonapiRegressionTest
too.Comment #12
gabesulliceThe actual fix in #8 looks good to me; back to @Wim Leers to review the reroll of @garphy's tests.
The test only patch is the interdiff.
Comment #14
Wim Leers🤓 Missing a verb. Fixed. ✅
🤔This is quite different compared to the other test methods in this class. Let's follow the same pattern. Done. ✅
All of that is just adding docs and reordering logic that @garphy wrote, so that doesn't preclude me from RTBC'ing :)
Comment #15
catchNeeds a re-roll. Not sure what happened to DrupalCI marking issues CNW.
Comment #16
Wim LeersRebased.
Comment #18
catchCommitted e0a8d33 and pushed to 8.8.x. Thanks!
Comment #20
catch