Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[JsonResponse] Silent only JSON errors#11418
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
GromNaN commentedJul 18, 2014
| 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
are you sure this will cover the case properly ? If the error happens early in the error handling, thejson_last_error might be related to a previous json operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sure, that's the tricky part.
I can runjson_encode(null) before to clearjson_last_error().
But it's still possible to executejson_encode orjson_decode inside aJsonSerializable object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The most reliable way to know if the error has been triggered by this call tojson_encode is to check the$errfile and$errline.
fabpot commentedSep 15, 2014
@GromNaN What's the status of this PR? Is it ready to be merged? |
e5f2d42 toa4a8535CompareGromNaN commentedSep 15, 2014
That's very hard to test and to catch all cases. |
fabpot commentedSep 16, 2014
@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. 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The warning is converted to an exception by PHPUnit.
fabpot commentedSep 22, 2014
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/aCommits-------6d6a3af Clear json_last_erroref91f71 Fix JsonSerializable namespaced952f90 Catch exceptions to restore the error handlerddf95c7 [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] VarDumperRelated HHVM issues:-facebook/hhvm#5563Commits-------8a78255 [2.6] Towards 100% HHVM compat