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] Added scalar denormalization #35235

Merged
merged 1 commit into from Jan 13, 2020

Conversation

@a-menshchikov
Copy link
Contributor

a-menshchikov commented Jan 6, 2020

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

Was added an ability to deserialize scalar data (single or array).

@a-menshchikov a-menshchikov requested a review from dunglas as a code owner Jan 6, 2020
@a-menshchikov a-menshchikov changed the title Added scalar denormalization in Serializer [Serializer] Added scalar denormalization Jan 6, 2020
@nicolas-grekas nicolas-grekas added this to the 4.3 milestone Jan 7, 2020
@a-menshchikov a-menshchikov requested a review from nicolas-grekas Jan 7, 2020
@a-menshchikov a-menshchikov requested a review from nicolas-grekas Jan 7, 2020
@a-menshchikov a-menshchikov requested a review from nicolas-grekas Jan 7, 2020
@dunglas
dunglas approved these changes Jan 7, 2020
Copy link
Member

nicolas-grekas left a comment

a last comment :)

src/Symfony/Component/Serializer/CHANGELOG.md Outdated Show resolved Hide resolved
src/Symfony/Component/Serializer/Serializer.php Outdated Show resolved Hide resolved
@ogizanagi

This comment has been minimized.

Copy link
Member

ogizanagi commented Jan 7, 2020

Just asking: does it really make sense for the Serializer and is it consistent to blindly cast the values?
AFAIK, the object normalizer:

@a-menshchikov

This comment has been minimized.

Copy link
Contributor Author

a-menshchikov commented Jan 7, 2020

@ogizanagi object normalizer doesn't work with scalar values. Data that passed into denormalize method should be an object.

@a-menshchikov a-menshchikov force-pushed the a-menshchikov:bugfix/33784 branch from 170461f to a89ced0 Jan 10, 2020
@a-menshchikov

This comment has been minimized.

Copy link
Contributor Author

a-menshchikov commented Jan 10, 2020

We briefly discussed with @xabbuh this question and he expressed his point of view that this is rather a feature than a bug.
But I have a bit different view. To explain it I added test for normalizing scalar values. As you can see, historically normalizing of scalars was worked and return strings are valid json values. But if we have trying to deserialize this value (in current 3.4) it leads to error. And I find here a serializer interface inconsistence, because in my mind any result of serialization should be able to be deserialized into origin value. From this point of view it seems like a bug.

@ogizanagi if you insist I'm ready to rebase PR to 5.0 as a new feature implementation.

@ogizanagi

This comment has been minimized.

Copy link
Member

ogizanagi commented Jan 10, 2020

As you can see, historically normalizing of scalars was worked and return strings are valid json values.

But scalar "normalization" in such case is simply "encoding". If you simply need decoding a scalar value, perhaps you should use decode instead?

Actually I'm not certain where we're going here: if the aim is to get a bijective behavior between serialize & deserialize, then we also need to handle null and arbitrary arrays, so such assertions would pass:

$this->assertSame([2, 'foo'], $serializer->deserialize('[2, "foo"]', 'array', 'json'));
$this->assertSame(null, $serializer->deserialize('null', 'null', 'json'));

right? (but that wouldn't even be enough to make it truly bijective)

In any case, I'd still argue this is a new usage of the Serializer, hence a feature for 5.1.

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Jan 11, 2020

By our standards, this is a new feature.

@a-menshchikov a-menshchikov force-pushed the a-menshchikov:bugfix/33784 branch from a89ced0 to bd53564 Jan 11, 2020
@a-menshchikov a-menshchikov requested review from lyrixx, sroze and xabbuh as code owners Jan 11, 2020
@a-menshchikov a-menshchikov changed the base branch from 3.4 to master Jan 11, 2020
@a-menshchikov a-menshchikov force-pushed the a-menshchikov:bugfix/33784 branch from bd53564 to 6536d6a Jan 11, 2020
@a-menshchikov a-menshchikov requested a review from nicolas-grekas Jan 11, 2020
@ogizanagi ogizanagi added Feature Serializer and removed Bug labels Jan 11, 2020
@a-menshchikov a-menshchikov force-pushed the a-menshchikov:bugfix/33784 branch 2 times, most recently from 1377d5e to 2f3eeb8 Jan 11, 2020
@a-menshchikov a-menshchikov requested a review from ogizanagi Jan 11, 2020
Copy link
Member

nicolas-grekas left a comment

With a minor cs change

src/Symfony/Component/Serializer/Serializer.php Outdated Show resolved Hide resolved
@a-menshchikov a-menshchikov force-pushed the a-menshchikov:bugfix/33784 branch from 2f3eeb8 to dad04d0 Jan 13, 2020
@ogizanagi

This comment has been minimized.

Copy link
Member

ogizanagi commented Jan 13, 2020

Thx for your patience @a-menshchikov

ogizanagi added a commit that referenced this pull request Jan 13, 2020
This PR was merged into the 5.1-dev branch.

Discussion
----------

[Serializer] Added scalar denormalization

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | Fix #33784
| License       | MIT

Was added an ability to deserialize scalar data (single or array).

Commits
-------

dad04d0 Added scalar denormalization in Serializer + added scalar normalization tests
@ogizanagi ogizanagi merged commit dad04d0 into symfony:master Jan 13, 2020
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
@a-menshchikov a-menshchikov deleted the a-menshchikov:bugfix/33784 branch Jan 13, 2020
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.