Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG]: Different behavior json_decode and \Phalcon\Helper\Json::decode() #14936

Closed
sinbadxiii opened this issue Mar 31, 2020 · 7 comments · Fixed by #15025
Closed

[BUG]: Different behavior json_decode and \Phalcon\Helper\Json::decode() #14936

sinbadxiii opened this issue Mar 31, 2020 · 7 comments · Fixed by #15025
Labels
bug A bug report

Comments

@sinbadxiii
Copy link
Contributor

  • Phalcon version: 4.0.5
  • PHP Version: 7.4.4
  • Operating System: Linux
  • Installation type: Compiling from source
  • Zephir version (if any): 0.12.17-6724dbf
  • Server: Nginx

if you pass an empty value in the json_decode(), it will return null

 var_dump(json_decode(""));
//null

but the \Phalcon\Helper\Json::decode("") throws out Exception:

var_dump(\Phalcon\Helper\Json::decode(''))
//InvalidArgumentException: json_decode error: Syntax error in BaseController.php:34 Stack trace: #0 BaseController.php(34): Phalcon\Helper\Json::decode() # ...

as a result, early compatibility breaks $ this->request->getJsonRawBody()

What behavior will be correct?)

Thx

@sinbadxiii sinbadxiii added bug A bug report status: unverified Unverified labels Mar 31, 2020
@TimurFlush
Copy link

This is because the Phalcon uses the JSON_THROW_ON_ERROR constant in json_decode options
I don't know how good or bad it is.

But in my code I always use this constant

@sinbadxiii
Copy link
Contributor Author

sinbadxiii commented Apr 6, 2020

JSON_THROW_ON_ERROR

JSON_THROW_ON_ERROR in the json_decode() is known.

But in the phalcon, there is a problem with getJsonRawBody(), there we can not specify the json decode options.

Nonetheless , by default, I think, the behavior of the json_decode and the \Phalcon\Helper\Json::decode() should be the same.

@TimurFlush
Copy link

JSON_THROW_ON_ERROR

JSON_THROW_ON_ERROR in the json_decode() is known.

But in the phalcon, there is a problem with getJsonRawBody(), there we can not specify the json decode options.

Nonetheless , by default, I think, the behavior of the json_decode and the \Phalcon\Helper\Json::decode() should be the same.

As temporarily fix, you can override this method

@sergeyklay
Copy link
Contributor

sergeyklay commented May 5, 2020

Well, even in plain PHP it is error:

var_dump(json_decode(""));
echo json_last_error_msg();
Syntax error

@sinbadxiii
Copy link
Contributor Author

sinbadxiii commented May 6, 2020

Well, even in plain PHP it is error:

var_dump(json_decode(""));
echo json_last_error_msg();
Syntax error

but the result of json_decode("") will still be null if you do not call json_last_error_msg()

Let me tell you what caused me to write Issue.

In the project on Phalcon 3, there is a class that accepts $this->request->getJsonRawBody(), and with an empty request it worked out normally, it might be worthwhile to do validation on the input data, but for now let's say that it doesn’t matter.

So with the upgrade to Phalcon 4, the behavior has changed, now an error is issued.
i.e now just calling $this->request->getJsonRawBody() causes an error.

I temporarily replaced $this->request->getJsonRawBody() with  I am using json_decode($this->request->getRawBody())

and it works :) but in essence - it should work the same

P.S. I seem not the only one https://forum.phalcon.io/discussion/20587/getjsonrawbody-strange-behavior

@niden niden added 4.0.6 and removed status: unverified Unverified labels May 6, 2020
@niden niden mentioned this issue May 6, 2020
5 tasks
@niden niden linked a pull request May 6, 2020 that will close this issue
5 tasks
@sergeyklay
Copy link
Contributor

Fixed in 4.0.x branch. Thank you for the bug report, and for helping us make Phalcon better.

@sinbadxiii
Copy link
Contributor Author

Thanks guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug report
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants