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

enclose return of getRawSchemaLessAttributes with is_array check #46

Closed
wants to merge 3 commits into from
Closed

enclose return of getRawSchemaLessAttributes with is_array check #46

wants to merge 3 commits into from

Conversation

pippinsmith
Copy link

Fixes #45

@freekmurze
Copy link
Member

There's a lot - too much - going on in that line. Could you split it up to make the intention more clear. Could you also add a test that illustrates why this change is necessary?

@pippinsmith
Copy link
Author

Thank you for the feedback, I have split the code to be more clear and follow correct if statement semantics (not committed yet) I will write the test later today.

@pippinsmith
Copy link
Author

I've split up the code and added a test with assertions that use the old code to prove it doesn't return an array, and an assertion that executes the modified function and asserts it returns an empty array.

I will agree on the fact that the original issue could only occur if the model property is manually set to a string instead of using the suggested helper methods for updating values, which is definitely a mistake on the implementation side.
That being said this PR fixes any errors on the getRawSchemaLessAttributes function if it's return type would not have been an array (in the case that the json_decode failed and didn't return an array but NULL).

@pippinsmith
Copy link
Author

Disclaimer, this is actually my first time writing a unit test, feel free to give me feedback on it, I kinda quickly looked through the other tests and wrote my test based on the knowledge gathered from those.
(I did run it to check if it actually worked before committing though 😉 )

@freekmurze
Copy link
Member

Going through the original issue again, I don't know if we should fix this. Shouldn't the users take care that there are regular NULLs in the db and not strings "null"?

@pippinsmith
Copy link
Author

I agree that this is indeed the user's responsibility, though on the official php documentation it clearly states that there are more possible return types than just object/array: source
Though if you decide that it is the users responsibility to make sure the values in the database are json decodable since the package provides all the necessary methods to do so, I will not try to reason with that decision.
I suppose at this point this PR is more of a personal test to see if I could actually fix this issue on the package side, and a venture into writing package tests. I suppose the one thing I failed in was recognizing if this issue was actually on the implementer's side and therefore should not be fixed on the package side
(although I'm not the one that added the label 'good first issue', not pointing fingers here, just mentioning that it was my incentive to start on this PR, maybe I falsely assumed that 'good first issue' means 'PR needed'?)

TL;DR: You are probably right about this and if you decide to close this PR I wouldn't disagree, though I wouldn't mind if you could find the time (I know, time is precious) if you could give any positive/negative/neutral feedback about my test case, as I'm always eager to improve my skills and this was a very new thing to me.

@freekmurze
Copy link
Member

Going to close this for now.

I took a look at the test, on first glance it seemed good. Thanks for wanting to contribute.

@freekmurze freekmurze closed this Apr 2, 2019
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.

2 participants