educoder / pest

A proper REST client for PHP.
356 stars 123 forks source link

Decode exception, errno as part of errormsg, throwDecodingExceptions (bool) #43

Closed ghost closed 11 years ago

ghost commented 11 years ago

Base commit 33843b2

1: Decode was throwing Encode exception, fixed 2: Added errno as part of error encode/decode exception message (handy in case of unknown in switch-case) 3: Added $throwDecodingExceptions (bool)

Bonus: commit c0799f0 - Added json_last_error_msg formatted error string when available >= PHP 5.5.0 Bonus commit a6ae6a6 - Added JSON_ERROR_UTF8 as JSON_ERROR_NONE (or do we have JSON_ERROR_IMPOSSIBLE hehe..) in case not defined (<= PHP 5.3.3) + corrected function_exist() to test boolean === false rather than !foo() for json_last_error_msg() added in c0799f0

Notes: Please consider adjusting the JSON exceptions defaults as true since the people who will use this library expect to get errors easily by default. Others probably never used PestJSON.php but instead used Pest.php directly - the ones that upgrade probably will understand to handle those exceptions - I understand backwards compatibility was a concern here? Pest.php throws exceptions by default, should follow that probably as well?

Also are we positive that JSON_ERROR_NONE is always set for json_error() with the versions of json library that PHP carries. Is there any reason not to check the $ret value for being false or null from encode/decode functions?

djsipe commented 11 years ago

Thanks for the update! :) Not to nit-pick, but now that we have everything nicely formatted... Can you check and see what you are using as indentation in your editor. I'd like to keep it consistent with 4 spaces as a tab. Could you make that quick change?

One other thing that was probably my fault for not naming the property clearly enough...I'm not sure people will really want to throw exceptions on decoding errors only and not encoding errors---though I get that the property name I had implied that it should only toggle the encoding errors. What if we renamed the property something like $throwJsonErrors and used it in both contexts?

ghost commented 11 years ago

PHP_NOTICE: PHP will use the constant as string value if not defined and throw PHP_NOTICE about it assuming. I've updated the constant to be defined as string value like it's assumed so that gets rid of the PHP_NOTICEs that are thrown now. The behaviour of this may change in future PHP versions as well so better be on safe side! e.g. in future it might raise critical compilation error... assumptions are bad.. better define the assumption :)

Encoding and decoding are two different beasts.. where as you can control your own encodings to some degree you may not be able to control decodings from other servers as you have nothing to do with the curl layer.. so it makes sense to control both separately.. though I see little reason for not to catch that exception in any case that leads me to following...

Can we please please leave these encoding/decoding errors as default true.. most people want them.. I see very rarely having the need of not using those precious exceptions... we should always care about exceptions and Pest.php was per default on... also newbies should be taught to use these exceptions... I see red when people don't handle the errors that APIs put out.. :)

And thanks for noticing the logic error.. grrr... oops!

Cheers

ghost commented 11 years ago

btw and as we have nicely formatted code we could start using more typing as well. e.g. boolean to be tested === false/true in PHP .. !foo() still works but testing for correct type could be good discipline to have as sometimes return can be a bit mixed... agree?

ghost commented 11 years ago

And if the code is interfacing with some crappy JSON server, they should use the last_response and manually json_decode it in that case.. all crappy JSON servers need to be fixed and not take away from the library for proper error handling :P

ghost commented 11 years ago

Bonus commit to turn both true as default. Debate above.

ghost commented 11 years ago

Can close this now