[Serializer] Add NormalizedValueInterface that can be returned by NormalizerInterface::normalize to enable receiving a value object for normalized values.
#43498
Conversation
…NormalizerInterface::normalize` to enable receiving a value object for normalized values.
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
src/Symfony/Component/Serializer/Normalizer/NormalizedValueInterface.php
Outdated
Show resolved
Hide resolved
…me the method `getNormalization` to `getValue`
|
The errors by psalm are about missing return type on some of the serializer classes. I was thinking they shouldn't by default return a NormalizedValue since that would be a BC break. This should be an kind of opt-in feature imo. I wonder what your thoughts are on that @dunglas :) |
| @@ -161,6 +162,10 @@ public function normalize(mixed $data, string $format = null, array $context = [ | |||
| return $normalizer->normalize($data, $format, $context); | |||
| } | |||
|
|
|||
| if ($data instanceof NormalizedValueInterface) { | |||
There was a problem hiding this comment.
In my opinion the serializer class should return the wrapper object too.
We have a use case in API Platform: we'll be able to return an object containing the serialized data and some additional metadata. We need to be able to access the metadata from the outside.
There was a problem hiding this comment.
Awesome to hear there is an reason to have this in api platform. I will have a look later today too see how that might work.
I didn't initially since i thought that would be a bigger bc break and if you want to access the data you could just have your own serializer.
I do see your point though
|
If this is a new feature, this PR must target 5.4. |
|
I'm really scratching my head over this. I mainly fail to understand why caching would be the concern of a serialization layer. What we're basically doing here is fundamentally changing the contract of normalizers. Be aware that the return value of a normalizer is not only consumed by the serializer but also by other normalizers that currently do not expect this new type of return type. |
Sorry for this long explanation below. Maybe it's an indication we're overthinking things :P Consider an object representing a bank account. It has a balance property represented by a Balance object. Only the bank tellers and customers are supposed to see the balance value and customer support staff should not. When that account is serialized into a representation to be transferred to the user, the balance should be omitted from one version of the serialization and included in another. Drupal's object model is quite complex and its serializations are costly to compute. Therefore, we cache them (much like one would cache a bit of rendered HTML). To safely cache them, we need to know under what contexts the serialization is valid for a particular user (i.e. something like the user's role: teller, customer, or support) so that we can compute a cache key (e.g. Which object should be concerned with access to the balance value? We decided it should be the Balance object's responsibility. If the Balance object implements We could have some sort of "pre-normalization" step in which we compute a tree of value objects that carry cache information before passing that tree into the Symfony serializer to be normalized to keep the serialization abstraction pure, but it seems a little silly since the Symfony serializer is already traversing a tree to begin with. |
|
@gabesullice to me, a solution to do that in the current architecture of the Serializer component is to make the Btw, to me, this PR cannot be complete as is. One of the consumers of the normalized data are encoders, that are left unchanged in that PR and so don't handle your NormalizedValue. |
@stof Would you mean by context, just passing context by reference perhaps and keeping track there?
Yes, this is true, but the discussion on this is very valuable. I wouldn't mind going through the whole motion of getting this PR all done, but only if we can agree on this is probably "a good idea" (TM) :) @dunglas you mention that API platform would benifit from this in an earlier comment. Perhaps you could ellaborate a little further on that? |
|
For instance, we need to keep track of the IRIs of all serialized resources. Currently we indeed use a hack involving a context in passed by reference. This works but its a bit hacky. |
Wouldn't using a state object in the context work instead? |
I've built this PR to see if we can make the migration to Symfony 6 for Drupal a little less painfull. This also enables some new handy feature for the serializer where you could for example create a Cache dependency tree while serializing a tree of objects.
This PR is a result of the discussion in #43021.
Drupal serialization
Drupal uses de serializer component for a few things. One of these things is serializing our entity objects to something that can be returned as json. The entity objects contain fields which can have one or more values, so we start high up and normalize all the way to the bottom of the stack, the field values.
Creating sensible cache metadata using a value object
In order to get the combined cache tag information from every level of serialization we've chosen to track that in the normalizer. It knows what it did and we track this in a normalized value object (called CachableNormalization). This means we can consolidate the proper caching dependency tree information of the object without having to do some weird gymnastics to track this information somewhere else.
It would help us loads if we would have some sort of
NormalizedValueInterfacethat can be returned by the normalize method. This would enable us to get things working as intented and keep a less complex cache-tracking system in place. An interface like this would work for us quite handily.