-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Conversation
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! |
I'm glad you're working on it :) |
There was a problem hiding this 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// ... 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
I added a "closes" to the PR header |
There was a problem hiding this 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?
@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! |
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). |
45210b4
to
55c4ceb
Compare
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. 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. |
: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`. |
There was a problem hiding this comment.
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
: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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just (same below):
The ``JsonEncoder`` | |
``JsonEncoder`` |
There was a problem hiding this 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
: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. |
There was a problem hiding this comment.
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.
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 ? :) |
@wouterj yes, please ... ship it whenever you can find some time for this. Thanks! |
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. |
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