10000 [Serializer] Fix deserialization of object with private constructor by l3l0 · Pull Request #19025 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Serializer] Fix deserialization of object with private constructor #19025

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

[Serializer] Fix deserialization of object with private constructor #19025

wants to merge 3 commits into from

Conversation

l3l0
Copy link
Contributor
@l3l0 l3l0 commented Jun 10, 2016
Q A
Branch? 2.7
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

@l3l0 l3l0 changed the title Fix deserialization of object with private constructor [Serializer] Fix deserialization of object with private constructor Jun 10, 2016
@l3l0
Copy link
Contributor Author
l3l0 commented Jun 10, 2016

Ok I just noticed that 2.7 has in composer "php: >=5.3.9" and this patch will not work on php 5.3.
Should I open PR for master then? Maybe I should to find a way to make it work on php 5.3 as well...

@nicolas-grekas
Copy link
Member

It could be seen as a new feature, and be submitted on master yes.

@xabbuh
Copy link
Member
xabbuh commented Jun 10, 2016

To me this is a new feature. So 👍 for doing this on master.

@GuilhemN
Copy link
Contributor

This looks risky to me.
Wouldn't it be safer to allow to define a custom factory for each object ?

@l3l0
Copy link
Contributor Author
l3l0 commented Jun 10, 2016

@Ener-Getick Can you elaborate?

@GuilhemN
Copy link
Contributor
GuilhemN commented Jun 10, 2016 •
edited
Loading

@l3l0 the object's constructor may be necessary and a static method is often here to instantiate the object, so instead we could introduce a new annotation/metadata to choose which method to use to instantiate the object:

class Foo
{
    private function __construct()
    {
        // some instantiation
    }

    /**
     * @Constructor
     */
    public static function create()
    {
        return new self();
    }
}

@l3l0
Copy link
Contributor Author
l3l0 commented Jun 11, 2016 •
edited
Loading

@Ener-Getick right but arguments pass to constructor do not have to reflect internal state of the object - we can pass VO and convert them to basic types in constructor. We can have many named constructors as well. IMHO at last for PropertyNormalizer constructor should not be called. For now I will create new PR to master we can discuss that later in new PR.

@l3l0 l3l0 closed this Jun 11, 2016
@l3l0 l3l0 mentioned this pull request Jun 11, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Bug Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@l3l0 @nicolas-grekas @xabbuh @GuilhemN @carsonbot
0