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 PropertyValueNormalizer to AbstractObjectNormalizer for code reusability with composition #46662

Open
wants to merge 1 commit into
base: 7.2
Choose a base branch
from

Conversation

NorthBlue333
Copy link

@NorthBlue333 NorthBlue333 commented Jun 13, 2022

Q A
Branch? 6.2
Bug fix? no
New feature? yes
Deprecations? no
Tickets Relates to #46649 (as a note)
License MIT
Doc PR -

As stated in #46649, I would like to enable AbstractObjetNormalizer code resuable to child without having to rewrite all the logic and maintain it. This would also avoid using trick to normalize differently some properties, as it is done by ApiPlatform here and here.

I have made comments on my PR to explain some changes.

Please let me know if it needs some changes.

@carsonbot carsonbot added this to the 6.2 milestone Jun 13, 2022
@NorthBlue333 NorthBlue333 changed the title Change AbstractObjectNormalizer methods visibility for code reusability [Serializer] Change AbstractObjectNormalizer methods visibility for code reusability Jun 13, 2022
@@ -441,135 +686,63 @@ abstract protected function setAttributeValue(object $object, string $attribute,
* @throws NotNormalizableValueException
* @throws LogicException
*/
private function validateAndDenormalize(array $types, string $currentClass, string $attribute, mixed $data, ?string $format, array $context): mixed
protected function validateAndDenormalize(array $types, string $currentClass, string $attribute, mixed $data, ?string $format, array $context): mixed
Copy link
Author

Choose a reason for hiding this comment

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

This method is made protected because it is used line 392. This allows overriding only the nested properties denormalization logic, and only part of it by using all previous methods provided.

@NorthBlue333 NorthBlue333 force-pushed the feature/allow-extending-abstract-object-normalizer branch from cda47e5 to 7bbaab9 Compare June 13, 2022 11:40
*
* This function is not meant to be overriden, only used. If you might want to override validateAndDenormalize.
*/
final protected function isNullDenormalization(Type $type, string $currentClass, string $attribute, mixed $data, ?string $format, array &$context): bool
Copy link
Author

Choose a reason for hiding this comment

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

These functions always receive the same set of arguments in order to stay coherent and help developping. But you might want not to?

@stof
Copy link
Member

stof commented Jun 13, 2022

Making things protected has a huge impact in term of maintenance, because inheritance-based extension points are the ones for which it is harder to write a BC layer.
Couldn't we provide extension points based on composition instead for that use case ?

@NorthBlue333
Copy link
Author

Sure! Will try to this is as soon as I can.

@dunglas
Copy link
Member

dunglas commented Jun 15, 2022

For the record there is an ongoing (yet slow) work to move from inheritance to composition in the Serializer component. So I agree with @stof, it would be better to move this work forward instead of opening more methods.

@NorthBlue333 NorthBlue333 force-pushed the feature/allow-extending-abstract-object-normalizer branch 2 times, most recently from 12dfe3d to 7ff98ea Compare October 23, 2022 11:32
@NorthBlue333 NorthBlue333 changed the title [Serializer] Change AbstractObjectNormalizer methods visibility for code reusability [Serializer] Add PropertyValueNormalizer to AbstractObjectNormalizer for code reusability with composition Oct 23, 2022
@NorthBlue333
Copy link
Author

NorthBlue333 commented Oct 23, 2022

Hi there, I have updated my PR according to @stof and @dunglas comments.
I am not so sure about the PropertyValueNormalizer file name, location, and instanciation, I will make any necessary changes to this PR.
Thanks for your time

@NorthBlue333 NorthBlue333 force-pushed the feature/allow-extending-abstract-object-normalizer branch 4 times, most recently from 23b3904 to df02d70 Compare October 24, 2022 12:50
@NorthBlue333 NorthBlue333 force-pushed the feature/allow-extending-abstract-object-normalizer branch from df02d70 to 816eddf Compare October 24, 2022 15:38
@nicolas-grekas nicolas-grekas modified the milestones: 6.2, 6.3 Nov 5, 2022
@nicolas-grekas nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023
@nicolas-grekas nicolas-grekas modified the milestones: 6.4, 7.1 Nov 15, 2023
@xabbuh xabbuh modified the milestones: 7.1, 7.2 May 15, 2024
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.

None yet

6 participants