852 - setSerialiseNull(true) + exclusion strategies still include data

[fsevestre]

Q	A
Bug fix?	yes
New feature?	no
Doc updated	no
BC breaks?	no
Deprecations?	no
Tests pass?	wip
Fixed tickets	#852 #821
License	Apache-2.0

First commit: the failing test.
Second commit: a fix.

Given an array containing an object,
when I use an exclusion strategy to exclude this object,
I shouln't get any data about this object after serialization
even if I define null values to be serialize.

[fsevestre]

Just updated the first (better example and same set of tests for setSerializeNull(true) and setSerializeNull(false) and second commits.

The tests pass now, but I'm not sure that the way I check if the exclusion rule has skipped the class is good.

Any help with this will be well appreciated.

[goetas]

Thanks a lot for the PR. This was a long-standing feature request and I'm interested in merging it.

Im just afraid this could be considered a BC break. Is it? Should we have a "flag" to disable the serialization of null values inside a collection?

[fsevestre]

Personally, I consider this as a bug fix since the BC break was added in 1.4 and it broke my code when I tried to upgrade an old app.
But the bug was introduced in October 2016 so it may indeed break newer applications (I don't know in which case but it can be possible I think).

If you think it's safer to add a way to enable this change I will add it.

[goetas]

can you explain me why (is_object($v) && null !== $context->getExclusionStrategy() && $context->getMetadataStack()->isEmpty()) is necessary? isnt (null === $v && $context->shouldSerializeNull() !== true) enough?

[fsevestre]

In fact, it's the $context->shouldSerializeNull() !== true condition the problem because I want null values to be serialized (->setSerializeNull(true)) but not objects serialized to null using an exclusion strategy.

[fsevestre]

But as I said in my comment on the PR, I'm not sure that is_object($v) && null !== $context->getExclusionStrategy() && $context->getMetadataStack()->isEmpty() is the right check to do. I came to this condition because I went in the GraphNavigator class and saw this https://github.com/schmittjoh/serializer/blob/master/src/JMS/Serializer/GraphNavigator.php#L222-L228
But in case of some refactoring, the condition might be wrong :/

[goetas]

To me makes much more sense to not serialize nulls, independently from the reason why they are null (simple nulls or exclusion strategies).

What do you think

[fsevestre]

In my case it will be complicated because it will break my clients' fronts because they don't check for the key's existence but only the value :/

--

Maybe it's more a "returned value" issue, which shouldn't be considered as the same null value ?

I may be wrong (I don't work a lot with serializers) but in my mind array(new Object(toSkip), new Object(doNotSkip)) (with an exclusion strategy removing the first object) and array(new Object(doNotSkip)) should return the same result. FYI it's not totally the case anyway because of the array key: in the first example, the second object will have the key 1 and in the second the key 0 in the json/yaml/... result.

--

Just to add my use case, we generate a big tree (no limit of level, depends on the configuration) of objects, do lots of things on it, and update a display (bool) property which is used in an exclusion strategy to skip the class (shouldSkipClass) with display at false and remove the property (shouldSkipProperty).
All the objects are very important during the process (even the ones that are supposed to be hidden) but we don't want to return some of them (display false) through the API because it's a business requirement.

[goetas]

hmmm, will have to think about it 😟

[goetas]

Fixed in 94c319d