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

Allow multiple nested serializer calls #341

Closed
wants to merge 2 commits into from
Closed

Allow multiple nested serializer calls #341

wants to merge 2 commits into from

Conversation

xavierlacot
Copy link

In case of serializer recursion, the serializer send an exception: "Can't pop from an empty datastructure", as noted by @damienalexandre in #319

This PR allows to stack the navigator association to visitors, thus making it possible to nest serializer calls. It contains two commits:

  • the first one contains the required changes for the bug to be fixed;
  • the second one adds a test which shows up the bug resolution (tested against the fixed testsuite proposed in Fix test suite on travisci #292)

@ogizanagi
Copy link

Any news about this PR ?

@schmittjoh
Copy link
Owner

I'm not sure whether we need to support this use-case, i.e. nested calls. I think it might be better to create another serializer instance if you need this instead of using the same instance.

@ogizanagi
Copy link

Thanks for the quick answer 😃

This could be out of the scope of the current debate, but may I ask you the proper way to do that, as I need to register my custom handlers too, in the context of a Symfony 2 application ?
This basically means redoing the same work as in the jms/serializer-bundle, right ?

However, for my use case, using the SerializerBuilder directly, and registering the handlers I need seems sufficient.

@damienalexandre
Copy link

I find it disturbing that we need a fresh new Serializer to perform nested calls. A new SerializationContext should be enough. I use Serializer as a service most of the time, where one instance is supposed to fit all serialization needs.

Isn't it like saying Serializer is not stateless?

@digitalkaoz
Copy link

yeah ran into the same issue here...sadly the serializer is somewhat stateful and thats a bad design here in my eyes

@digitalkaoz
Copy link

@schmittjoh why are you blocking this valid use case?

@digitalkaoz
Copy link

if you have a more complex serializer construction (custom handlers, event subscribers, object constructor etcpp) this is a pain in the ass.

@bassrock
Copy link

👍

@bassrock
Copy link

@schmittjoh this helps especially when using this with Symfony so that you can reuse the injected serializer in different parts of the same code run.

@bassrock
Copy link

I created a new branch that mimics this but on the latest code base with a few additions. #453

@sfavot
Copy link

sfavot commented Aug 4, 2015

👍

@goetas
Copy link
Collaborator

goetas commented Aug 4, 2016

Do not know if this is still an issue, but #453 (comment) proposes an alternative implementation for this feature

@goetas
Copy link
Collaborator

goetas commented Oct 31, 2016

Nested serialization calls currently require big changes, for a relatively small benefit. Probably getting a new serializer instance can partially fix the issue.

Not planning to accept the feature for now.

@goetas goetas closed this Oct 31, 2016
@xavierlacot
Copy link
Author

So this makes the serializer stateful and unusable as a service, with all the performance and memory consumption downsides.

This is a blocker for "real life" production usage, as explained in the previous comments. This has proven to be blocking in at least 3 or 4 of our client projects, and should be clearly stated in the documentation: "jms-serializer is stateful and doesn't care of performance issues".

@patie
Copy link

patie commented Mar 14, 2017

👍

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.

9 participants