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

Small but useful. Entity class. #1176 #1186

Closed
wants to merge 13 commits into from

Conversation

nowackipawel
Copy link
Contributor

I've changed name which i proposed before (json-object -> json) cuz I use default parameter for json_decode($value, [$assoc = false]) in calling.

I've changed name which i proposed before (json-object -> json) cuz I use default parameter for json_decode($value, [$assoc = false]) in calling.
Copy link
Member

@lonnieezell lonnieezell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs docs added for these functions, as well as tests, please.

case 'json':
if (function_exists('json_decode') && is_string($value) && (strpos($value, '[') === 0 || strpos($value, '{') === 0))
{
$value = json_decode($value, false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should check the json_error_code here, also, to ensure it's valid json, or throw an exception otherwise, so the developer knows what went wrong.

Copy link
Contributor Author

@nowackipawel nowackipawel Aug 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Firstly I am not convinced about throwing exception when case 'array' won't throw anything when serialized array is formatted incorrectly.

Secondly if you think it is really required to throw exception with json (let me say more, I do not see any proper exception in CI that I could throw in this case; should I use "Config Exception" and a new Language/en/Entuty.php file dedicated to it?) then we have to clarify that
json_error_code() will return error if $value is null; what could be very likely case when data is retrieved from database. Do you reckon we should treat NULL value as exception or just ignore it? I bet: ignore it, as we do in the moment.

--
i will add docs and test in tmrw.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a fair point about the array - except that PHP itself will throw exceptions if the serialization fails, while I'm pretty sure json_encode doesn't throw an exception, it just returns false, leaving it to us to throw that ourself.

Yes - I do agree that we ignore nulls like we currently do, because that's a possible valid response from a database query.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exception + language (messages) files added and docs modified

system/Language/en/Cast.php Outdated Show resolved Hide resolved
tl;dr; small improvements according to experience with using json, json-array casting in  implementation of my cms
formatting
extract code  for JSON casting (which was almost identical for "json" and "json-array") to castAsJson 
support for "string" and numeric casting: 0x322,    0272,    0b1010101, 1234e0,
removing force casting to object as "json"  (what was too much according to actual manual)
@nowackipawel
Copy link
Contributor Author

@lonnieezell could you check if everything is ok?

@jim-parry jim-parry added this to the 4.0.1? milestone Sep 27, 2018
@lonnieezell lonnieezell mentioned this pull request Oct 3, 2018
@lonnieezell
Copy link
Member

Merged in PR #1283

@lonnieezell lonnieezell closed this Oct 3, 2018
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

Successfully merging this pull request may close these issues.

3 participants