[BUG] serializer.post_serialize listeners are never notified if there's a custom handler for given custom data type

[yyaremenko]

JMS\Serializer\GraphNavigator\SerializationGraphNavigator::accept() line 204:

If there's a handler given for the custom type, e.g. JMS\Serializer\Handler\FormErrorHandler to handle form errors, SerializationGraphNavigator::afterVisitingObject() is never called, so listeners of serializer.post_serialize are never notified.

Is it by design or is it a bug? If it is by design, I would find it rather confusing, as listeners of serializer.pre_serialize event are always notified for custom data type, no matter if a custom handler is given or not.

[goetas]

If you decide to handle the serialization process (by having a custom handler), the serializer does not interfere with that process to avid side effects. if you need to customize the serialization process in that case, you have to do it in the handler (or to override the handler if is not in your control).

[yyaremenko]

My point is that, in case of custom handling, there should be either two events, or none (because if it's custom handler's business to dispatch a post_serialize event, then it also should be its business to dispatch pre_serialize event). Either a serializer does not interfere into dispatching events if a custom handler is given (all responsibility regarding pre- and post- events is given to the custom handler), to avoid side effects, or a serializer dispatches both events, no matter if the handler is a custom one or not. In the current situation, the serializer only does not interfere into post serialization, but interferes into pre serialization.

[goetas]

Ah, sorry, i missed that. then is probably a bug.

Looking at the 1.x code, there were both events... probably that got lost in the 2.x major release.

At this point it seem to me that the safest thing to do is to add back the post_serialize event.

Do you agree? Are you willing to send a patch for this?

[yyaremenko]

well, I am not the bundle developer, so I can't really know what is the proper way to fix the thing, so I prefer to believe you here and I think you are right

[yyaremenko]

yep, I can do the fix; so, I should make sure the post_serialize event is dispatched no matter if custom handler is given or not. did I understand you correctly?

[goetas]

:heart_eyes:

correct. you can have a look on how it was done in 1.x ( see https://github.com/schmittjoh/serializer/blob/ba908d278fff27ec01fb4349f372634ffcd697c0/src/JMS/Serializer/GraphNavigator.php#L204)

[yyaremenko]

hey @goetas looks like the previous version actually worked in the very same way; prior to fixing the bug, I've made a failing test,

https://github.com/yyaremenko/serializer/commit/5f673bf10414df09b06def61cd4e991a1170bd72

but I am not sure now if I'm moving in the right direction; should we maybe invite @schmittjoh to make things clear?

[goetas]

I'm fine with adding that event, i think that will not hurt

[yyaremenko]

okay, I've made a fix, my test passes, but now the other tests fail

https://github.com/yyaremenko/serializer/commit/6a6a08dad8b9c479ab6bebdcaf6c19f1bf986c69

@goetas could you please probably have a look and give a hint on what to do? the failing tests complain on non existing classes:

JMS\Serializer\Tests\Serializer\Doctrine\IntegrationTest::testDiscriminatorIsInferredFromDoctrine ReflectionException: Class ArrayCollection does not exist

JMS\Serializer\Tests\Serializer\JsonSerializationTest::testBlogPost ReflectionException: Class ArrayCollection does not exist

JMS\Serializer\Tests\Serializer\JsonSerializationTest::testDeserializingNull ReflectionException: Class ArrayCollection does not exist

JMS\Serializer\Tests\Serializer\JsonSerializationTest::testLog ReflectionException: Class AuthorList does not exist

JMS\Serializer\Tests\Serializer\JsonSerializationTest::testCircularReference ReflectionException: Class ArrayCollection does not exist

... and so on

if I move

$metadata = $this->metadataFactory->getMetadataForClass($type['name']);

back to its original place, the errors do not happen anymore; but I need metadata for the post-serialize event

[goetas]

Hmm..... that could be the reason for not having that event.... as it is an edgecase situation... :disappointed_relieved:

[yyaremenko]

@goetas may I have your update on this one please?