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

CodeIgniter\Format\JSONFormatter needs a look. #544

Closed
ivantcholakov opened this issue Jun 11, 2017 · 3 comments
Closed

CodeIgniter\Format\JSONFormatter needs a look. #544

ivantcholakov opened this issue Jun 11, 2017 · 3 comments

Comments

@ivantcholakov
Copy link

ivantcholakov commented Jun 11, 2017

Current code: https://github.com/bcit-ci/CodeIgniter4/blob/26cee0f2d3f6327825ebbe9f84f50e10ef4bf339/system/Format/JSONFormatter.php

1
The property $errors (the array) is not needed, because there is the PHP function json_last_error_msg() that does the job. http://php.net/manual/en/function.json-last-error-msg.php

2
$result = json_encode($data, 512, $options);
Argument placement is wrong, it should be:
$result = json_encode($data, $options, 512); http://php.net/manual/en/function.json-encode.php

3
"If result is NULL, then an error happened." - this is not true. FALSE is returned on failure. But since FALSE could be a valid result, it would be better json_last_error() !== JSON_ERROR_NONE to be the condition about failure detection.

4
return utf8_encode($result);
Why utf8_encode() usage is needed here? I think, it should be removed.

@lonnieezell
Copy link
Member

Good calls. I must have thrown that one together pretty fast at one point.

  1. You're right about the json_last_error_message() function. I had forgotten about that one.
  2. true.
  3. again, true.
  4. That was a solution to help ensure unicode characters (such as in the cyrillic alphabet) are not converted to hexadecimal entities, but kept as their original characters. I'm not sure why I opted for that method instead of JSON_UNESCAPED_UNICODE passed in $options, though.

I'll get a patch up for this tonight.

@ivantcholakov
Copy link
Author

I haven't notice before one more thing that is to be removed: JSON_NUMERIC_CHECK

IMO this is data interpretation that does not belong here, because it depends on meaning which is unknown in this context. The application code should decide whether to convert to numbers or not. Here is a plausible demo what could get broken on active JSON_NUMERIC_CHECK option:

var_dump(json_encode(['phone' => '088222333'], JSON_NUMERIC_CHECK)); // Would give a wrong phone number
var_dump(json_encode(['phone' => '088222333'])); // Would give a correct result (just a phone number string with the leading zero preserved)

@lonnieezell lonnieezell reopened this Jun 14, 2017
@lonnieezell
Copy link
Member

Thank you for the example. I kept going back and forth on whether to do that, but couldn't think of an example where it would cause an issue. That's a very good one. I'll remove those.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants