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

Rebase of serialization nesting onto the latest #453

Closed
wants to merge 2 commits into from

Conversation

bassrock
Copy link

This is a rebase of #341 on the latest code base. It also adds some accessor methods to remove properties via a callback after it has been serialized.

@digitalkaoz
Copy link

+1

@crisu83
Copy link

crisu83 commented Aug 13, 2015

+1

This is important because we use the same serializer instance across our Lumen application through DI. My temporary fix for this was to wrap SerializationBuilder in a static helper classes that registers the required metadata directories and returns the serialized object.

In other words we use a separate instance for serializing nested objects. While this works, it's not as dynamic as using the same instance.

@bassrock
Copy link
Author

bassrock commented Feb 3, 2016

@schmittjoh Any chance of merging this in? Just rebased with the latest from master again.

@bassrock
Copy link
Author

bassrock commented Aug 3, 2016

@goetas Since you look to be helping now. Any chance of merging this in?

@goetas
Copy link
Collaborator

goetas commented Aug 4, 2016

I see some problems with this PR that makes it "not reday to be merged". First, you have removed tests. Second, this approach complicates all the visitors adding code to handle this specific usecase.

Have you considered an approach of recreating only the visitor instance for each seralization call? It should give that same result without changing visitors code...

@bassrock
Copy link
Author

bassrock commented Aug 4, 2016

@goetas Im not sure what you mean when you say I removed tests. I haven't removed any tests.

How would you recreate the visitor instance for each serialization call?

@goetas
Copy link
Collaborator

goetas commented Aug 4, 2016

Im not sure what you mean when you say I removed tests. I haven't removed any tests.

as you can see on https://github.com/schmittjoh/serializer/pull/453/files there are no tests

How would you recreate the visitor instance for each serialization call?

https://github.com/schmittjoh/serializer/blob/master/src/JMS/Serializer/Serializer.php#L68 contains the list of visitors.

Currently visitors are pre-instantiated in the serialization builder. Doing some kind of lazy-instantiation/callback will allow to instantiate the visitor right when needed.

It is a bit more complex feature, but is a general solution to the problem exposed here

@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
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.

4 participants