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] Add an option to skip null values #28661

Merged
merged 1 commit into from Oct 2, 2018

Conversation

@dunglas
Copy link
Member

commented Oct 1, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR symfony/symfony-docs#10427

Adds a new option to not serialize null values:

$dummy = new class {
    public $foo;
    public $bar = 'notNull';
};

$normalizer = new ObjectNormalizer();
$result = $normalizer->normalize($dummy, 'json', ['skip_null_values' => true]);
// ['bar' => 'notNull']

This feature is the only missing part to add JSON Merge Patch support in API Platform.
It will also help supporting this RFC in all other projects using the Symfony Serializer.

Copy link
Contributor

left a comment

Correct me if I am wrong, but utilizing $context for this most likely means you can't configure Serializer to enable this option by default? If somebody decides to go with this, they will want this to be enabled for whole instance by default. At least that happened in our case. App developer told us he does not want us to send null values and we could easily do that globally, since we used JMS serializer.

@nicolas-grekas nicolas-grekas added this to the next milestone Oct 1, 2018
@dunglas

This comment has been minimized.

Copy link
Member Author

commented Oct 1, 2018

@ostrolucky you're right. I'll also add a constructor option.

@dunglas

This comment has been minimized.

Copy link
Member Author

commented Oct 2, 2018

@ostrolucky done

I also widened the visibility of $maxDepthHandler to protected, for consistency.

/**
* @var callable|null
*/
private $maxDepthHandler;
protected $maxDepthHandler;

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Oct 2, 2018

Member

should be private

/**
* @var bool
*/
protected $skipNullValues = false;

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Oct 2, 2018

Member

shouldbe private also (and the typehint looks useless to me, isn't it?)

This comment has been minimized.

Copy link
@dunglas

dunglas Oct 2, 2018

Author Member

As it's an abstract class, making it private allows to reuse this property in child classes... and we need to do it in API Platform.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Oct 2, 2018

Member

that's still possible to interact with by decorating the skipNullValues method
protected properties are very hard to deprecate, that's why we really prefer not introducing new ones

This comment has been minimized.

Copy link
@dunglas

dunglas Oct 2, 2018

Author Member

We need to access to the value too... in the hot path.
But I agree with your point, and it's why we need to improve the general design of this component.

As a compromise, can we keep it protected but mark it @internal? We'll use it "as is" in API Platform, and if it breaks at some point... we'll upgrade!

@dunglas

This comment has been minimized.

Copy link
Member Author

commented Oct 2, 2018

Last commit has been reverted, after discussing with @nicolas-grekas we had a better idea: we'll introduce soon an option to be able to easily set a default context.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Oct 2, 2018

We just talked with @dunglas on Slack: he's going to make another PR that will provide a $contextDefaults argument to the constructor, instead of any added setter or extra argument.

@nicolas-grekas nicolas-grekas force-pushed the dunglas:serializer-skip-null-values branch from 0e158f7 to d3c5055 Oct 2, 2018
@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Oct 2, 2018

Thank you @dunglas.

@nicolas-grekas nicolas-grekas merged commit d3c5055 into symfony:master Oct 2, 2018
1 of 3 checks passed
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 Oct 2, 2018
This PR was squashed before being merged into the 4.2-dev branch (closes #28661).

Discussion
----------

[Serializer] Add an option to skip null values

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      |no
| New feature?  | yes <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | n/a   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | todo

Adds a new option to not serialize `null` values:

```php
$dummy = new class {
    public $foo;
    public $bar = 'notNull';
};

$normalizer = new ObjectNormalizer();
$result = $normalizer->normalize($dummy, 'json', ['skip_null_values' => true]);
// ['bar' => 'notNull']
```

This feature is the only missing part to add [JSON Merge Patch](https://tools.ietf.org/html/rfc7386) support in [API Platform](https://api-platform.com).
It will also help supporting this RFC in all other projects using the Symfony Serializer.

Commits
-------

d3c5055 [Serializer] Add an option to skip null values
@dunglas dunglas deleted the dunglas:serializer-skip-null-values branch Oct 2, 2018
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Oct 3, 2018
This PR was merged into the master branch.

Discussion
----------

[Serializer] Add an option to skip null values

symfony/symfony#28661

Commits
-------

6349bd3 [Serializer] Add an option to skip null values
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018
This was referenced Nov 3, 2018
sandergo90 pushed a commit to sandergo90/symfony that referenced this pull request Jul 4, 2019
…(dunglas)

This PR was squashed before being merged into the 4.2-dev branch (closes symfony#28661).

Discussion
----------

[Serializer] Add an option to skip null values

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      |no
| New feature?  | yes <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | n/a   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | todo

Adds a new option to not serialize `null` values:

```php
$dummy = new class {
    public $foo;
    public $bar = 'notNull';
};

$normalizer = new ObjectNormalizer();
$result = $normalizer->normalize($dummy, 'json', ['skip_null_values' => true]);
// ['bar' => 'notNull']
```

This feature is the only missing part to add [JSON Merge Patch](https://tools.ietf.org/html/rfc7386) support in [API Platform](https://api-platform.com).
It will also help supporting this RFC in all other projects using the Symfony Serializer.

Commits
-------

d3c5055 [Serializer] Add an option to skip null values
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.