[JsonResponse] Silent only JSON errors #11418
Conversation
| Q | A |
|---|---|
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | maybe |
| Fixed tickets | #11415 |
| License | MIT |
| Doc PR | n/a |
| $this->data = @json_encode($data, $this->encodingOptions); | ||
| $errorHandler = null; | ||
| $errorHandler = set_error_handler(function () use (&$errorHandler) { | ||
| if (JSON_ERROR_NONE !== json_last_error()) { |
There was a problem hiding this comment.
are you sure this will cover the case properly ? If the error happens early in the error handling, the json_last_error might be related to a previous json operation.
There was a problem hiding this comment.
Sure, that's the tricky part.
I can run json_encode(null) before to clear json_last_error().
But it's still possible to execute json_encode or json_decode inside a JsonSerializable object.
There was a problem hiding this comment.
The most reliable way to know if the error has been triggered by this call to json_encode is to check the $errfile and $errline.
|
@GromNaN What's the status of this PR? Is it ready to be merged? |
e5f2d42 to
a4a8535
Compare
|
That's very hard to test and to catch all cases. |
|
@GromNaN Thanks for your feedback. As your PR is an improvement over the current situation, I think it's safe to merge it as is. Perfection can be achieved later on... is possible at all. 👍 |
| @@ -201,4 +201,32 @@ public function testSetContent() | |||
| { | |||
| JsonResponse::create("\xB1\x31"); | |||
| } | |||
|
|
|||
| /** | |||
| * @expectedException Exception | |||
There was a problem hiding this comment.
The warning is converted to an exception by PHPUnit.
|
Thank you @GromNaN. |
This PR was submitted for the master branch but it was merged into the 2.5 branch instead (closes #11418). Discussion ---------- [JsonResponse] Silent only JSON errors | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | maybe | Fixed tickets | #11415 | License | MIT | Doc PR | n/a Commits ------- 6d6a3af Clear json_last_error ef91f71 Fix JsonSerializable namespace d952f90 Catch exceptions to restore the error handler ddf95c7 [HttpFoundation] Silent only JSON errors
This PR was merged into the 2.6 branch. Discussion ---------- [2.6] Towards 100% HHVM compat | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #9917, #11418 | License | MIT | Doc PR | - Failing components: - [x] HttpFoundation - [x] HttpKernel - [x] VarDumper Related HHVM issues: - facebook/hhvm#5563 Commits ------- 8a78255 [2.6] Towards 100% HHVM compat