Support conditional exclude for classes

[arneee]

Hi,

after further checking #1098, I've noticed that this functionality just isn't there yet. I've created a first draft PR for supporting condtional excludes on class level.

The PR includes the changes in class metadata and SerializationGraphNavigator as well as the annotation driver.

Still missing is:

• Evaluate condition in DeserializationGraphNavigator
• Other Metadata drivers
• Unit tests

While all existing tests are passing, I was not able to add the corresponding unit tests for this case. I'm also not sure if there are any unintended side effects by not completely excluding the class if there is an @exclude annotation like it is done at the moment, but keeping it and do the evaluation during serialization.

Thanks for your help!

Q	A
Bug fix?	no
New feature?	yes
Doc updated	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#1098
License	MIT

[goetas]

Very nice addition! Thanks a lot!
Will to a proper review in the next days.

[arneee]

Hi Asmir,

Cool, thanks! 👍

The PR definitely still needs some work, but since I only used this library yesterday for the first time, I'm not sure how to do the missing parts, especiallcy the unit tests.

Regards,

Arne

[goetas]

Hi, I found some time to review this.

The base-intuition is good and I think you are on the right direction.

The other drivers (xml and yaml) are still missing and tests.

For tests a good starting point can be:

• serializer/tests/Serializer/BaseSerializationTest.php

  Line 276 in 0e3bb12

  public function testExpressionExclusionConfiguredWithDisjunctStrategy()

• serializer/tests/Serializer/BaseSerializationTest.php

  Line 377 in 0e3bb12

  public function testExpressionExclusion($person, ExpressionFunction $function, $json)

You can probably add a property of PersonSecret that is an object and uses class based exclusion rules.

[arneee]

Hi @goetas ,

i was working on this again and run into a issue. While serialization works fine, there is one problem during deserialization.

/**
 * @Serializer\Exclude(if="object.expired != true")
 */
class PersonAccount {

    /**
     * @Serializer\Type("string")
     */
    public $name;

    /**
     * @Serializer\Type("boolean")
     */
    public $expired;

}

During deserialize() we come to the point where the exclude-If expression is being evaluated. Unfortunately the object is null, since the deserialization has not happened yet and therefore the expression fails.

I see two options.

1. Deserialize the object first (including all the properties, childs, etc.) and then evaluate the expression. That would also require to add the object in the DeserializationContext (currently it is only present in the SerializationContext)
2. Skip the exclude during deserialization

What is your opinion on this? Thanks!

[goetas]

This is a known issue, and @Serializer\Exclude(if="object && object.expired != true") should do the job. Do you think is enough?

[arneee]

That would be the easiest solution. For my use case that would be perfectly fine, since I'm interested in serialization only anyway. For the library I don't know, should deserialization offer the same functionality like serialization? If this is ok for you, it's fine for me.

[arneee]

Ok, hope I got this right and in sync now.

I've tried to get it working with the solution you have suggested before, but I wasn't able to completely skip the relevant objects during deserialization. If an object was skipped by a class level exclude-if, the array of the parent element still contained a null item.

For now I've removed the Exclude-If on deserialization, it only works during serialization.

[goetas]

This looks much better! We'll done.

Can you please add some tests for the Metadata in xml and yaml?

[arneee]

Can you please add some tests for the Metadata in xml and yaml?

Done (and now the drivers are actually working). 😃

[goetas]

Looks good! Can you please fix the code style issue (you can just run vendor/bin/phpcbf), after that I think it is ready to be merged!

[arneee]

Looks good! Can you please fix the code style issue (you can just run vendor/bin/phpcbf), after that I think it is ready to be merged!

It is coming from GraphNavigatorTest, which I didn't change. phpcbf does not find anything on my installation ("No fixable errors were found")

Tried to change this part manually, let's see what travis says... still strange.

[goetas]

Thanks a lot for your excellent work! very very nice! :)

[arneee]

Hi Asmir,
Thanks for the merge, unfortunately I've noticed a mistake on my side. Currently the condition is inverted, means Exclude(if="false") will actually exclude it. This was due to a copy mistake from Expose in the drivers as well as confusing test. I have simplified the test to make it more clear and fixed the drivers. I will create a new PR.