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

Use empty keys instead of _empty_ in json decoding #1926

Merged
merged 1 commit into from
Jun 20, 2016

Conversation

bukka
Copy link
Member

@bukka bukka commented May 29, 2016

This follows #1836 and uses empty properties instead of empty. That improves symmetry of the json_encode and json_decode.

@staabm
Copy link
Contributor

staabm commented May 29, 2016

Should be mentioned as a BC break in the changelog/NEWS

@bukka
Copy link
Member Author

bukka commented May 29, 2016

@staabm I don't usually do that in the PR as there might be changes and I would have to sync it. So I almost always update news and upgrading after I merge the PR ;)

@@ -464,7 +464,7 @@ array(14) {
float(1.23456789E-13)
["E"]=>
float(1.23456789E+34)
["_empty_"]=>
[""]=>
Copy link
Member

Choose a reason for hiding this comment

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

what is the different here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really sure which one do you mean as github is showing this comment on two different places (github is being wierd though :) ). One in the commit view (File changed)

> -    ["_empty_"]=>
> +    [""]=>

which is to replace _empty_ and one in Conservation which shows

 -    ["e"]=>
+    ["e"]=>

which I'm not really sure. It might be some new line but if I look from the command line git diff, I don't see anything so might also be some github diff issue.

Anyway there isn't anything special that we should worry about :) It's just about not using _empty_ and make decoding array and object a bit more consistent!

Choose a reason for hiding this comment

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

If you check out the code the diffs is quite a bit different around these lines so github seems to be broken. I'd review in your favorite git tool instead.

@nikic
Copy link
Member

nikic commented Jun 15, 2016

What's the state here?

@bukka
Copy link
Member Author

bukka commented Jun 15, 2016

I will merge it this or next week

@php-pulls php-pulls merged commit f0d1cca into php:master Jun 20, 2016
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.

6 participants