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

Projects
None yet
5 participants
@ajgarlag
Copy link
Contributor

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 ajgarlag:hotfix/denormalize-variadic-constructor-parameter branch from 1171ce3 to 7108f84 May 9, 2019

@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 ajgarlag:hotfix/denormalize-variadic-constructor-parameter branch from 36056a4 to 995d189 May 9, 2019

@ajgarlag

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

PR ready to merge

Show resolved Hide resolved ...Symfony/Component/Serializer/Tests/Normalizer/AbstractNormalizerTest.php Outdated
Show resolved Hide resolved src/Symfony/Component/Serializer/Normalizer/AbstractNormalizer.php Outdated
);
$params = array_merge($params, $variadicParameters);
unset($data[$key]);

This comment has been minimized.

Copy link
@dunglas

dunglas May 10, 2019

Member

Why this change?

This comment has been minimized.

Copy link
@ajgarlag

ajgarlag May 10, 2019

Author Contributor

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 ajgarlag:hotfix/denormalize-variadic-constructor-parameter branch from a736781 to c8c3c56 May 11, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented May 11, 2019

Thank you @ajgarlag.

@nicolas-grekas nicolas-grekas merged commit c8c3c56 into symfony:3.4 May 11, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request May 11, 2019

bug #31438 [Serializer] Fix denormalization of object with variadic c…
…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

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

@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

This comment has been minimized.

Copy link
Member

commented May 16, 2019

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

@WalentinG

This comment has been minimized.

Copy link

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;
    }
}

@fabpot fabpot referenced this pull request May 22, 2019

Merged

Release v4.3.0-BETA2 #31577

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.