Skip to content

[Serializer] Allow AbstractNormalizer to use null for non-optional nullable constructor parameters without default value #40522

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

Merged
merged 1 commit into from
Mar 29, 2021

Conversation

pounard
Copy link
Contributor

@pounard pounard commented Mar 19, 2021

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #40511
License MIT

Serializer component AbstractNormalizer attemps to guess constructor parameters, and falls back using default values when possible. Yet, it misses one use case: nullable non-optional parameter with value not being present in incoming input, case in which null is a valid value, not the default one, yet still valid.

This PR introduce a two-line fix that forcefully set null as value for missing from input non-optional nullable constructor parameters values.

@pounard pounard requested a review from dunglas as a code owner March 19, 2021 16:45
@pounard pounard changed the title issue #40511 - Allow AbstractNormalizer to use null for nullable constructor parameters without default value issue #40511 - Allow AbstractNormalizer to use null for non-optional nullable constructor parameters without default value Mar 19, 2021
@pounard
Copy link
Contributor Author

pounard commented Mar 19, 2021

Build failure seems to be unrelated, am I misreading it ?

@nicolas-grekas nicolas-grekas changed the title issue #40511 - Allow AbstractNormalizer to use null for non-optional nullable constructor parameters without default value [Serializer] Allow AbstractNormalizer to use null for non-optional nullable constructor parameters without default value Mar 22, 2021
@nicolas-grekas nicolas-grekas added this to the 5.x milestone Mar 22, 2021
@pounard
Copy link
Contributor Author

pounard commented Mar 22, 2021

@ro0NL @nicolas-grekas thank you. I hope it will land in next 5.2 release, I have to maintain a patch in the meantime.

@nicolas-grekas nicolas-grekas modified the milestones: 5.x, 4.4 Mar 22, 2021
…constructor parameter denormalization when not present in input
@nicolas-grekas nicolas-grekas changed the base branch from 5.x to 4.4 March 22, 2021 16:48
@carsonbot carsonbot changed the title [Serializer] Allow AbstractNormalizer to use null for non-optional nullable constructor parameters without default value Allow AbstractNormalizer to use null for non-optional nullable constructor parameters without default value Mar 22, 2021
@carsonbot carsonbot changed the title Allow AbstractNormalizer to use null for non-optional nullable constructor parameters without default value [Serializer] Allow AbstractNormalizer to use null for non-optional nullable constructor parameters without default value Mar 22, 2021
@nicolas-grekas
Copy link
Member

I rebased on 4.4, where bugfixes should be merged.

@pounard
Copy link
Contributor Author

pounard commented Mar 22, 2021

I rebased on 4.4, where bugfixes should be merged.

Oh, nice ! Thank you very much.

@chalasr
Copy link
Member

chalasr commented Mar 29, 2021

Thank you @pounard.

@chalasr chalasr merged commit e16be83 into symfony:4.4 Mar 29, 2021
This was referenced May 1, 2021
nicolas-grekas added a commit that referenced this pull request May 5, 2023
…listed in the input (Christian Kolb)

This PR was merged into the 6.3 branch.

Discussion
----------

[Serializer] Add flag to require all properties to be listed in the input

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | Fix #49504
| License       | MIT
| Doc PR        | symfony/symfony-docs#17979

The PR #40522 introduced a fallback for nullable properties to be set to `null` when they aren't provided as parameters.

A drawback of that approach is that it easier for bugs to appear through typos or renamings of those properties. I think the current implementation makes perfect sense as a default. Therefore, this PR introduces a new context flag that prevents that fallback behaviour. This way nothing changes for existing systems, but for people wanting more control, it's possible to set a flag.

### Example

```php
final class Product
{
    public function __construct(
        public string $name,
        public ?int $costsInCent,
    ) {
    }
}

// This works and results in $costsInCent as null
$product = $this->serializer->deserialize(
    '{"name": "foo"}',
    Product::class,
    JsonEncoder::FORMAT,
);

// When using the flag, only the following JSON is valid
$product = $this->serializer->deserialize(
    '{"name": "foo", "costsInCent": null}',
    Product::class,
    JsonEncoder::FORMAT,
    [
        AbstractNormalizer::PREVENT_NULLABLE_FALLBACK => true,
    ],
);

// This would result in an error due to missing parameters
$product = $this->serializer->deserialize(
    '{"name": "foo"}',
    Product::class,
    JsonEncoder::FORMAT,
    [
        AbstractNormalizer::PREVENT_NULLABLE_FALLBACK => true,
    ],
);
```

Commits
-------

d62410a [Serializer] Add flag to require all properties to be listed in the input
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.

Allow AbstractNormalizer to use null for nullable constructor parameters without default value
6 participants