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

Add default SerializerContext to serializer, update builder #541

Closed

Conversation

alcalyn
Copy link
Contributor

@alcalyn alcalyn commented Dec 23, 2015

This PR adds a default SerializerContext factory and SerializerContext factory in Serializer class to avoid to pass a context every time we call serialize, deserialize, toArray or fromArray.

It also update the serializer builder by adding setDefaultSerializerContextFactory and setDefaultDeserializerContextFactory methods, allowing to pass a callable which create a context.

Now we can set a default serializer context factory by doing this:

use JMS\Serializer\SerializationContext;

$serializer = \JMS\Serializer\SerializerBuilder::create()
    ->setDefaultSerializationContextFactory(function () {
        return SerializationContext::create()
            ->setSerializeNull(false)
        ;
    })
    ->build()
;

@alcalyn
Copy link
Contributor Author

alcalyn commented Dec 23, 2015

For now I made it as simple as possible by using php Closure, then we can use the builder like that:

$serializer = \JMS\Serializer\SerializerBuilder::create()
    ->setDefaultSerializationContextFactory(function () {
        return \JMS\Serializer\SerializationContext::create()
            ->setSerializeNull(true)
        ;
    })
    ->build()
;

@nuxwin
Copy link

nuxwin commented Dec 23, 2015

@alcalyn

Please, don't enforce closure for the factory. Eg, in zf2 service manager, it is recommended to use factory classes which is much better. You could maybe enforce callable. This would allow invokables.

@alcalyn
Copy link
Contributor Author

alcalyn commented Dec 23, 2015

Yep I started to create interfaces SerializationContextFactoryInterface and DeserializationContextFactoryInterface, but I find it too verbose at first

@nuxwin
Copy link

nuxwin commented Dec 23, 2015

At first... Don't worry about verbosity. It is much better to rely on interface ;)

Thanks for your work anyway. I need this.

@alcalyn
Copy link
Contributor Author

alcalyn commented Dec 23, 2015

Now we can set a default serializer context factory by doing this:

use JMS\Serializer\ContextFactory\SerializationContextFactoryInterface;
use JMS\Serializer\SerializationContext;

class MySerializationContextFactory implements SerializationContextFactoryInterface
{
    public function createSerializationContext()
    {
        return SerializationContext::create()
            ->setSerializeNull(true)
        ;
    }
}

Then:

$serializer = \JMS\Serializer\SerializerBuilder::create()
    ->setDefaultSerializationContextFactory(new MySerializationContextFactory())
    ->build()
;

@alcalyn
Copy link
Contributor Author

alcalyn commented Jan 4, 2016

Last commit allows to use a callable in Serializer builder for a shorter syntax. Example:

$serializer = \JMS\Serializer\SerializerBuilder::create()
    ->setDefaultSerializationContextFactory(function () {
        return SerializationContext::create()
            ->setSerializeNull(false)
        ;
    })
    ->build()
;

@alcalyn alcalyn closed this May 5, 2016
@alcalyn alcalyn reopened this May 22, 2016
@alcalyn alcalyn force-pushed the feature-default_serializer_context branch from 57bb4f4 to f60cbac Compare May 23, 2016 09:56
@alcalyn
Copy link
Contributor Author

alcalyn commented Aug 2, 2016

What about this PR ? @schmittjoh

@goetas
Copy link
Collaborator

goetas commented Aug 2, 2016

To me looks a bit too much 500 lines of code, just to have a default implementation of the context... that anyway changes for each usecase

@alcalyn
Copy link
Contributor Author

alcalyn commented Aug 2, 2016

Mmh in theses 500 lines, its about 200 lines for testing, 120 for license, a bunch of one-method interfaces...

And I just needed it this morning because I don't want to create a wrapper around serializer service to only create a new context with setSerializeNull(true) on each serialize.

Maybe another solution is to set serializeNull in config instead of context, but also setGroups and enableMaxDepthChecks could be in config, because as a new user, I don't know why a part of configuration is in builder, and another part in context...

So that's why I think a default context factory in builder is not déconnant :D

@nuxwin
Copy link

nuxwin commented Aug 2, 2016

@goetas

For our project, we have interest too in that PR. Please, merge it ;)

@alcalyn ---> Déconnant ???

@alcalyn
Copy link
Contributor Author

alcalyn commented Aug 2, 2016

is not irrevelant* :P

@goetas
Copy link
Collaborator

goetas commented Aug 2, 2016

Personally I can agree with having a context factories, but what is the use case for the callable context factories?

They are already part of a specific implementation, so probably they should stay in your project, while the default factories should stay here... is it?

@alcalyn
Copy link
Contributor Author

alcalyn commented Aug 2, 2016

the callable context factory is to avoid to create a class which just call setSerializeNull(true), and provides a more "builder" way to add a default context (see #541 (comment)).

@goetas goetas added the To Merge label Aug 9, 2016
@goetas goetas added this to the v1.4 milestone Aug 9, 2016
@goetas
Copy link
Collaborator

goetas commented Sep 9, 2016

What do you think about renaming some variables as in #645 ?

(i have just removed the "default" prefix that was looking redundant to me)

@alcalyn
Copy link
Contributor Author

alcalyn commented Sep 9, 2016

Okey, its shorter

@alcalyn
Copy link
Contributor Author

alcalyn commented Sep 10, 2016

I close it in favor to #645

@alcalyn alcalyn closed this Sep 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants