-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
+123
−5
Closed
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
5451c1a
Small but useful. Entity class. #1176
nowackipawel 31843fd
entity cast json / json-array docs
nowackipawel e3a52a8
Adding support for system/Exceptions/CastException
nowackipawel fc93a03
language file (Cast) for system/Exception/CastException.php
nowackipawel 5fcd663
Update Cast.php
nowackipawel 8496c6b
Update Cast.php
nowackipawel 7fd144f
Update Cast.php
nowackipawel 90ba7bb
Cast exceptions
nowackipawel 96a9d12
Update CastException.php
nowackipawel ad7c74f
Update Entity.php
nowackipawel 919b3bd
Update CastException.php
nowackipawel cdaffdd
Keys: Cast.stringName => stringName
nowackipawel 33395e5
improvments
nowackipawel File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
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.
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.
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.
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.
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.
exception + language (messages) files added and docs modified