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

Fix serializing nulls in arrays #626

Closed
wants to merge 1 commit into from

Conversation

megazoll
Copy link
Contributor

nulls in the arrays should not be ignored when serializeNull is true.

@megazoll megazoll force-pushed the serialize-null-array branch from 5604cd8 to 0cef8ba Compare August 19, 2016 15:31
@megazoll
Copy link
Contributor Author

Also fixed yaml serialization for integrity and added some tests for serializing nulls

@goetas
Copy link
Collaborator

goetas commented Sep 9, 2016

Merging this as it is is not possible. As it is, is a BC break since the array serialization behavior changes between versions.

What about adding an extra shouldSerializeInArrayNull ? this will make it BC compatible.

Other suggestions?

Otherwise will have to postpone it to 2.0

@scaytrase
Copy link
Contributor

Not sure that fixing bug should be considered a BC break

@goetas
Copy link
Collaborator

goetas commented Sep 9, 2016

Not sure that fixing bug should be considered a BC break

valid point....

@goetas
Copy link
Collaborator

goetas commented Sep 9, 2016

But there will be a lot of broken code around if this gets merged as it is

@scaytrase
Copy link
Contributor

scaytrase commented Sep 9, 2016

The behaviour is inconsistent now. We have

$array = [0, null, 'a'];
$context = SerializationContext::create()->setSerializeNull(true);
Expressionvar_dump result
$array
array(3) {
  [0] =>
  int(0)
  [1] =>
  NULL
  [2] =>
  string(1) "a"
}
$serializer->toArray($array)
array(2) {
  [0] =>
  int(0)
  [1] =>
  string(1) "a"
}
json_encode($array)
string(12) "[0,null,"a"]"
serializer->serialize($array, 'json', $context)
string(15) "{"0":0,"2":"a"}"
Yaml::dump($array)
string(15) "- 0
- null
- a
"
serializer->serialize($array, 'yml', clone $context)
string(8) "- 0
- a
"

@goetas
Copy link
Collaborator

goetas commented Sep 9, 2016

@megazoll what do you think?

@goetas
Copy link
Collaborator

goetas commented Sep 9, 2016

@scaytrase I agree with you, but Im just worried for all the users that upgrading from 1.3 to 1.4 get their code to be broken.

@scaytrase
Copy link
Contributor

@goetas updated the table. Currently json makes the array to be an object, yaml does not (simply ignores null as toArray).

I think we should ask the community, i.e maintainers of some dependent libraries (FOS Rest, Sonata Admin)

https://www.versioneye.com/php/jms:serializer/references
https://www.versioneye.com/php/jms:serializer-bundle/references

@scaytrase
Copy link
Contributor

IMO
It's not a BC break to have a major version. But we can aggregate more changes in a separate branch, leaving this one stable behind. Focusing on a separate major (and not upgrading this one) release we can introduce more usefull changes alongside the improving consistency.

@goetas goetas modified the milestones: v1.5, v1.4 Oct 23, 2016
@goetas
Copy link
Collaborator

goetas commented Oct 23, 2016

At the moment moving it to 1.5

Anybody interested in implementing something as #626 (comment) ?

@megazoll
Copy link
Contributor Author

@goetas updated. Please review my changes.

@goetas goetas closed this in #660 Oct 28, 2016
@goetas
Copy link
Collaborator

goetas commented Oct 28, 2016

@megazoll Thanks for your effort, I have realized that this was an old standing bug (in XML was totally not working) and decided to fix it with #660 (as said is almost identical to the first version of your work).

Thanks again for the contribution and sorry if it took so much time and uncertainty.

@megazoll megazoll deleted the serialize-null-array branch October 28, 2016 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants