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

[5.5] Cast to object before json_encoding #21608

Closed
wants to merge 1 commit into from

Conversation

alepeino
Copy link
Contributor

Both Support\MessageBag and Eloquent\Model hold their data as an associative array, and their JSON representation is the simple the "json_encode-ing" of that array. It's an implicit contract that a key-value structure is returned.

However, when that array is empty, the resulting JSON is [] instead of the expected {}. Thus, that contract is broken, from the point of view of the consumer of the json string.

Consider (related and following up to #21605 ) a Vue component to which I'm passing a model and the validation errors.

<user-form :model="{{ $user }}"
           :errors="{{ $errors }}"
></user-form>

While the component expects an object, if the error bag (or, less likely, the model) is empty, an empty array is effectively passed. This change ensures the resulting JSON represents an object.

@Dylan-DPC-zz
Copy link

This is a breaking change.

@sisve
Copy link
Contributor

sisve commented Oct 10, 2017

I'll ignore the fact that you decided to send a PR that may break existing 5.5 installations because you're unhappy with the data format in your specific use case, and instead ask you if forgot to write tests, or just forgot to add those test files to the commit?

@alepeino
Copy link
Contributor Author

@sisve Well, I tried to provide reasons as to why this could be considered the breaking of a contract (it's a different data type from the perspective of a JS client for instance), so it's not about "my specific use case", is it.

Speaking of breaking, I do realize this may be potentially breaking, so I will re submit it to a different branch.

@alepeino alepeino closed this Oct 10, 2017
@Dylan-DPC-zz
Copy link

@alepeino when you say it fails for a use case, it is better to provide a test that shows the use case.

@deleugpn
Copy link
Contributor

Send it to Master

@alepeino
Copy link
Contributor Author

@Dylan-DPC @deleugpn will do, thanks!

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.

4 participants