Skip to content

Combine component and framework docs for Serializer #17783

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
Nov 23, 2024

Conversation

wouterj
Copy link
Member

@wouterj wouterj commented Jan 21, 2023

A very early PR, to avoid someone else working on this as well (especially @fabpot who is on fire with this type of PRs lately).

The serializer docs are the most unfortunate component - framework documentation situations I think, with most of the docs in the component docs instead of framework one. This PR is focused on combining all the information in the framework guide as a start.

closes #17814

@carsonbot carsonbot added this to the 5.4 milestone Jan 21, 2023
@wouterj wouterj marked this pull request as draft January 21, 2023 15:44
@javiereguiluz
Copy link
Member

Wouter, thank you so much for this 🙇 This has been easily the worst part of the Symfony Docs for a long time. Let's make it as good as the other parts of the docs!

@fabpot
Copy link
Member

fabpot commented Jan 21, 2023

I'm glad you're working on it :)

Copy link
Contributor

@94noni 94noni left a comment

Choose a reason for hiding this comment

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

small review coming from #17814 :)

// this creates a new Person instance based on the $jsonData
// (which contains e.g. `{"name": "Jane Doe", "age": 59, "sportsperson": true}`)

// ... do something with $person and return a response
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// ... do something with $person and return a response
// ... do something with $person and return a jsonresponse

? as you validate in first step it is a json request content type

@OskarStark
Copy link
Contributor

I added a "closes" to the PR header

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you for working on this.

Just a note: I would be nice to mention constructors. How are they being used when deserializing an object?

@javiereguiluz
Copy link
Member

@wouterj is this task something that you still want to complete?

If yes, take as long as you need 🙏

If not, let's close this as let's think of other ways to improve existing Serializer docs.

Thanks!

@wouterj
Copy link
Member Author

wouterj commented Sep 19, 2024

Small update: Lots of delays, but with fresh energy after my holiday, I'm very very close to finishing this rewrite locally (and we can improve with follow up PRs later).

@wouterj wouterj force-pushed the serializer branch 2 times, most recently from 45210b4 to 55c4ceb Compare October 20, 2024 23:00
@wouterj
Copy link
Member Author

wouterj commented Oct 20, 2024

So here we are... It is not perfect, but I believe it's complete enough to be merged. 🚀 We can iterate upon it after it's fully merged. Thank you very much for the patience this far! 🙏

The biggest thing missing at the moment imho is explaining how the serializer reads properties from the class, and how it determines the properties type. It was and is very focused on reading the type that is passed through the (de)serialize method, not so much on reading the property type/PHPdoc/getter return type/etc.
Also, the "Advanced Serialization" and "Advanced Deserialization" are a bit of a random collection of features. I've tried to order them by most-common to least-common. We might come up with a better way to organize the article in the future. There is a weird coupling of very low-level details and high level features (like what normalizer you must enable and configure in the context to get a specific feature), which made this an interesting challenge to organize in a beginner-friendly way.

I've not yet done a start to finish read of the new article (only scanning through it). It would be good if we good have a couple people reading it, and leaving feedback whenever they see something that requires attention 👀 .

Whenever the reviews are finished, I'll find some time to merge this PR and deal with the many merge conflicts when up-merging it to 7.2.

Comment on lines +1282 to +1435
:class:`Symfony\\Component\\Serializer\\Normalizer\\CustomNormalizer`
This normalizer calls a method on the PHP object when normalizing. The
PHP object must implement :class:`Symfony\\Component\\Serializer\\Normalizer\\NormalizableInterface`
and/or :class:`Symfony\\Component\\Serializer\\Normalizer\\DenormalizableInterface`.
Copy link
Member Author

Choose a reason for hiding this comment

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

I find it weird that this is the only built-in normalizer that is not registered by default. Is there a reason for this that we need to explain? (e.g. performance?)

serializer.rst Outdated
Comment on lines 1287 to 1455
:class:`Symfony\\Component\\Serializer\\Normalizer\\GetSetMethodNormalizer`
This normalizer is an alternative to the default ``ObjectNormalizer``.
It reads the content of the class by calling the "getters" (public
methods starting with ``get``, ``is`` or ``has``). It will denormalize
data by calling the constructor and the "setters" (public methods
starting with ``set``).

Objects are normalized to a map of property names and values (names are
generated by removing the ``get`` prefix from the method name and transforming
the first letter to lowercase; e.g. ``getFirstName()`` -> ``firstName``).

:class:`Symfony\\Component\\Serializer\\Normalizer\\PropertyNormalizer`
This is yet another alternative to the ``ObjectNormalizer``. This
normalizer directly reads and writes public properties as well as
**private and protected** properties (from both the class and all of
its parent classes) by using `PHP reflection`_. It supports calling the
constructor during the denormalization process.

Objects are normalized to a map of property names to property values.
Copy link
Member Author

Choose a reason for hiding this comment

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

So we have 2 older iterations of the ObjectNormalizer still hanging around in the component. Is there any situation where I would not want to use the ObjectNormalizer and instead register one of these 2? If not, should we deprecate them (or one of them)? Should we stop documenting them, and hope their usages will slowly extinguish?

I really dislike having to say "yet another alternative" without any concrete example of why you should want to use it.

Copy link
Member

Choose a reason for hiding this comment

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

Not an expert in this component, but what you say makes sense. Let's keep the good one and deprecate/remove the others.

All these encoders are enabled by default when using the Serializer component
in a Symfony application.

The ``JsonEncoder``
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just (same below):

Suggested change
The ``JsonEncoder``
``JsonEncoder``

Copy link
Member

@javiereguiluz javiereguiluz left a comment

Choose a reason for hiding this comment

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

Thanks Wouter for a superb effort refactoring the entire Serializer docs 🙇🙇🙇

serializer.rst Outdated
Comment on lines 1287 to 1455
:class:`Symfony\\Component\\Serializer\\Normalizer\\GetSetMethodNormalizer`
This normalizer is an alternative to the default ``ObjectNormalizer``.
It reads the content of the class by calling the "getters" (public
methods starting with ``get``, ``is`` or ``has``). It will denormalize
data by calling the constructor and the "setters" (public methods
starting with ``set``).

Objects are normalized to a map of property names and values (names are
generated by removing the ``get`` prefix from the method name and transforming
the first letter to lowercase; e.g. ``getFirstName()`` -> ``firstName``).

:class:`Symfony\\Component\\Serializer\\Normalizer\\PropertyNormalizer`
This is yet another alternative to the ``ObjectNormalizer``. This
normalizer directly reads and writes public properties as well as
**private and protected** properties (from both the class and all of
its parent classes) by using `PHP reflection`_. It supports calling the
constructor during the denormalization process.

Objects are normalized to a map of property names to property values.
Copy link
Member

Choose a reason for hiding this comment

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

Not an expert in this component, but what you say makes sense. Let's keep the good one and deprecate/remove the others.

@wouterj
Copy link
Member Author

wouterj commented Nov 16, 2024

I feel bad for pinging a PR that I've stalled for a year, but are you OK with me merging this one in the current state, @symfony/docs ? :)

@javiereguiluz
Copy link
Member

@wouterj yes, please ... ship it whenever you can find some time for this. Thanks!

@wouterj wouterj changed the base branch from 5.4 to 6.4 November 23, 2024 13:54
@wouterj
Copy link
Member Author

wouterj commented Nov 23, 2024

The sharp eye of @xabbuh revealed that this PR was a combination of 5.4 and 6.4 features. I've decided to completely rebase this PR to 6.4, given 5.4 is EOL this month and searching which 6.4 features unintentionally made it into this PR would be a huge task. Added a couple new sections about 6.4 features that were documented already.

When build is green, I'll merge this into 6.4 & 7.x.

@wouterj wouterj merged commit 2685f38 into symfony:6.4 Nov 23, 2024
2 of 3 checks passed
@wouterj wouterj deleted the serializer branch November 23, 2024 16:57
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.

8 participants