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

Documentation on Exclusion Strategies has an error #122

Closed
AntonStoeckl opened this issue Jul 17, 2013 · 7 comments
Closed

Documentation on Exclusion Strategies has an error #122

AntonStoeckl opened this issue Jul 17, 2013 · 7 comments

Comments

@AntonStoeckl
Copy link

Johannes, I really like your library (+ the bundle).

Just one issue:

Under "Creating Different Views of Your Objects" it says:

$serializer->setGroups(array('list'));
$serializer->serialize(new BlogPost(), 'json');

There is no setGroups method on the serializer object, instead this needs to be done:

$context = SerializationContext::create()->setGroups(array('list'));
$serialize->serialize(new BlogPost(), 'json', $context);

It works that way, but for me it does not play well with the DI service. If I create a factory there which does the SerializationContext::create() and injects it to a class that consumes it I have one single instance there (in fact in ALL the classes that got it injected) so it's inevitable that setGroups is called multiple times on the same instance which raises an error that the context is already initialized.
There are workarounds, but I'm not really happy:

  1. Call ->create() on the injected instance, which works as it's no singleton
  2. Create a factory service that does the setGroups() ... after creating a new instance, this way it's also only ONE instance everywhere, but already with the right settings. This for sure makes it inpossible to use different context settings, or you'd need different versions in the DI.

I'm using 1) now.

By the way, the documentation is wrong in the same way about setVersion(), this function is also in the context object.

Best regards, Anton

@stof
Copy link
Contributor

stof commented Jul 17, 2013

the SerializationContext is a value object, not a service. creating a new one each time is expected. Injecting a SerializationContext as dependency into a service does not make much sense IMO.

@AntonStoeckl
Copy link
Author

Sure, but it's also a bad idea to use it static in consumer classes as this introduces hidden dependencies. So what we need is a factory service that can be injected. And this is ecactly what I'm doing now in 1) as the SerializationContext with it's create() method acts as a factory. I just don't like the fact that create IS a static method which seduces people to use it statically -> hidden dependency -> bummer. I'm thinking it might be better if create() were not a static method. And it would be great if the bundle would offer that jms_serializercontext_factory out of the box. As you point out that it's a value context it might even be better to have a different class that acts as a factory, instead of the value object beeing it's own factory. Which makes it a service, IMO. ;-)
You see the dilemma. The design of the SerializationContext class is not clear.

And by the way, my main point in this issue was to point out the wrong documentation. ;-)

@stof
Copy link
Contributor

stof commented Jul 18, 2013

the static method does nothing more than calling the constructor. It is here to allow inline building of the context on PHP 5.3 as the syntax (new SerializationContext())->setGroups(array('Foobar')) only works as of 5.4

@AntonStoeckl
Copy link
Author

I see what it's ment for. But still I don't see how someone could use it without generating a hidden dependency. Calling new SerializationContext in a consuming class does exactly that. What would you suggest to keep the code clean and SOLID then?

@schmittjoh
Copy link
Owner

You're right about the documentation, the code shown there is referring to an older version of the serializer.

Regarding the factory, I think that makes the general case too complicated. If you however would like to have a serialization context pre-populated in a certain way, then you should be able to implement a factory in your code quite easily. Apart from that the SerializationContext is not a service with behavior, but a mere option container (value object-like).

@schmittjoh
Copy link
Owner

I just checked the docs chapter. It seems like you were simply reading the wrong version of the docs. If you read the master docs, you should see the current way of setting versions/groups.

@AntonStoeckl
Copy link
Author

Hi Johannes, I was talking about this page:
http://jmsyst.com/libs/serializer/master/cookbook/exclusion_strategies
The URL suggests this is master ...

Also, I can't find any documentation page that talks about the SerializationContext class, maybe you can point me to the right place?

Regards, Anton

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants