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

[Serializer] Fix denormalization of object with variadic constructor typed argument #31438

Conversation

ajgarlag
Copy link
Contributor

@ajgarlag ajgarlag commented May 9, 2019

Q A
Branch? 3.4 up to 4.2
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #31436
License MIT

This PR adds a test to demonstrate the bug, and a fix to squash it.

@ajgarlag ajgarlag force-pushed the hotfix/denormalize-variadic-constructor-parameter branch from 1171ce3 to 7108f84 Compare May 9, 2019 07:23
@nicolas-grekas nicolas-grekas added this to the 3.4 milestone May 9, 2019
@nicolas-grekas nicolas-grekas changed the title Fix denormalization of object with variadic constructor typed argument [Serializer] Fix denormalization of object with variadic constructor typed argument May 9, 2019
@ajgarlag ajgarlag force-pushed the hotfix/denormalize-variadic-constructor-parameter branch from 36056a4 to 995d189 Compare May 9, 2019 08:38
@ajgarlag
Copy link
Contributor Author

ajgarlag commented May 9, 2019

PR ready to merge

);

$params = array_merge($params, $variadicParameters);
unset($data[$key]);
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

@ajgarlag ajgarlag May 10, 2019

Choose a reason for hiding this comment

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

Without this change, when using the PropertyNormalizer, the deserialized object is successfully instantiated, but the property is overwritten again with the original $data[$key] array value ignoring the type hint.

https://github.com/ajgarlag/symfony/blob/a736781cc2499a3ab829ddbf7898740e1638d128/src/Symfony/Component/Serializer/Tests/Normalizer/AbstractNormalizerTest.php#L147-L150

@nicolas-grekas nicolas-grekas force-pushed the hotfix/denormalize-variadic-constructor-parameter branch from a736781 to c8c3c56 Compare May 11, 2019 09:57
@nicolas-grekas
Copy link
Member

Thank you @ajgarlag.

@nicolas-grekas nicolas-grekas merged commit c8c3c56 into symfony:3.4 May 11, 2019
nicolas-grekas added a commit that referenced this pull request May 11, 2019
…onstructor typed argument (ajgarlag)

This PR was squashed before being merged into the 3.4 branch (closes #31438).

Discussion
----------

[Serializer] Fix denormalization of object with variadic constructor typed argument

| Q             | A
| ------------- | ---
| Branch?       | 3.4 up to 4.2
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #31436
| License       | MIT

This PR adds a test to demonstrate the bug, and a fix to squash it.

Commits
-------

c8c3c56 [Serializer] Fix denormalization of object with variadic constructor typed argument
@ajgarlag
Copy link
Contributor Author

@nicolas-grekas I'd like to have this issue fixed in 4.2 branch too. Can you tell me when will be merged into that branch? Thanks.

@nicolas-grekas
Copy link
Member

There is nothing to do on your side, we will merge 3.4 into 4.2 with the fix.

@WalentinG
Copy link

WalentinG commented May 20, 2019

@nicolas-grekas, the fix breaks current workaround
try the dummy object below in your test, with

$extractor = new PropertyInfoExtractor([], [new PhpDocExtractor(), new ReflectionExtractor()]);
$normalizer = new PropertyNormalizer(null, null, $extractor);
<?php

namespace Symfony\Component\Serializer\Tests\Fixtures;

final class VariadicConstructorAnnotationTypedArgsDummy
{
    /** @var Dummy[] */
    private $foo;
    
    public function __construct(...$foo)
    {
        $this->foo = $foo;
    }
    
    public function getFoo()
    {
        return $this->foo;
    }
}

This was referenced May 22, 2019
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.

None yet

5 participants