Skip to content

[Serializer] Feature deserialization of object with private constructor #19027

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

Conversation

l3l0
Copy link
Contributor

@l3l0 l3l0 commented Jun 11, 2016

Q A
Branch? "master"
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

See: #19025

@dunglas
Copy link
Member

dunglas commented Jun 12, 2016

Can you describe a bit more the use case?

{
$serializer = new Serializer(array(new GetSetMethodNormalizer()), array('json' => new JsonEncoder()));
$data = array('title' => 'foo', 'numbers' => array(5, 3));
$result = $serializer->deserialize(json_encode($data), '\Symfony\Component\Serializer\Tests\ModelWithPrivateConstructor', 'json');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use the ::class constant instead of the FQCN as string?

@GuilhemN
Copy link
Contributor

@dunglas this can be useful when you use a third party library allowing to instantiate its classes with static methods (even if this proposal looks risky to me and i would prefer to be able to customize the object factory).

@dunglas
Copy link
Member

dunglas commented Jun 13, 2016

IMO it would be easier to use a custom normalizer for that kind of usage.

@GuilhemN
Copy link
Contributor

@dunglas I agree even if i think we should allow to simply override the class constructor with something like:

protected function getConstructor(string $class, \ReflectionClass $reflectionClass) : \ReflectionMethod
{
    return $reflection->getConstructor();
}

protected function instantiateObject(/** ... */)
{
    // ...
    $constructor = $this->getConstructor($class, $reflectionClass);
    // ...
    if ($constructor->isConstructor()) {
        return $reflectionClass->newInstanceWithArgs($args);
    } else {
        return $constructor->invokeArgs(null, $args);
    }
}

WDYT ?

@l3l0 would also be able to fix his issue without introducing a hack in the core.

@dunglas
Copy link
Member

dunglas commented Jun 13, 2016

@Ener-Getick Good idea!

@l3l0
Copy link
Contributor Author

l3l0 commented Jun 13, 2016

@dunglas @Ener-Getick use case is quite simple... serialize and unserialize events with private constructor (cause events have many named constructor) and I do not want to break encapsulation and make constructor public only because serializer do not handle that :) This can be the problem with serializing other kind of Value Objects as well.
I resolved my issue by writing custom normalizer for now.

Extension point proposed by @Ener-Getick seems to make such changes easier and it seems like good idea (I would even thiking about delegating instantiateObject to separate object/class).

We should consider change default behavior of property normalizer as well, cause it seems strange to me that data are read/write by reflection and calling constructor is required (or at least it should be optional)

@GuilhemN
Copy link
Contributor

GuilhemN commented Jun 13, 2016

@l3l0 i don't think this is worth creating a new class. At worst, we can allow to pass a callable to the normalizer as what is done to manage circular references but i think the right way is just to extend the normalizer.

@fabpot
Copy link
Member

fabpot commented Jun 16, 2016

I like @Ener-Getick proposal more than the current implementation of this PR.

@GuilhemN
Copy link
Contributor

I opened #19137.

fabpot added a commit that referenced this pull request Jun 29, 2016
…Ener-Getick)

This PR was merged into the 3.2-dev branch.

Discussion
----------

[Serializer] Allow to use easily static constructors

| Q             | A
| ------------- | ---
| Branch?       | "master"
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #19027 (comment)
| License       | MIT
| Doc PR        | -

This PR allows to simply use static constructors to instantiate objects with the serializer by extending the default normalizers.

Commits
-------

9be6484 [Serializer] Allow to use easily static constructors
symfony-splitter pushed a commit to symfony/serializer that referenced this pull request Jun 29, 2016
…Ener-Getick)

This PR was merged into the 3.2-dev branch.

Discussion
----------

[Serializer] Allow to use easily static constructors

| Q             | A
| ------------- | ---
| Branch?       | "master"
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony/symfony#19027 (comment)
| License       | MIT
| Doc PR        | -

This PR allows to simply use static constructors to instantiate objects with the serializer by extending the default normalizers.

Commits
-------

9be6484 [Serializer] Allow to use easily static constructors
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.

5 participants