NotificationsYou must be signed in to change notification settings
Fork9.6k
Star30.4k

[Serializer] Add PropertyValueNormalizer to AbstractObjectNormalizer for code reusability with composition#46662

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Open

NorthBlue333 wants to merge1 commit intosymfony:7.4

base:7.4

Choose a base branch

fromNorthBlue333:feature/allow-extending-abstract-object-normalizer

Open

[Serializer] Add PropertyValueNormalizer to AbstractObjectNormalizer for code reusability with composition#46662

NorthBlue333 wants to merge1 commit intosymfony:7.4fromNorthBlue333:feature/allow-extending-abstract-object-normalizer

+474 −221

Conversation

Copy link

NorthBlue333 commentedJun 13, 2022•
edited
Loading

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 enableAbstractObjetNormalizer 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 ApiPlatformhere andhere.

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

Please let me know if it needs some changes.

NorthBlue333 requested a review fromdunglas as acode owner

June 13, 2022 11:23

carsonbot added Status: Needs Review Feature labels

Jun 13, 2022

carsonbot added this to the6.2 milestone

Jun 13, 2022

NorthBlue333 changed the title~~Change AbstractObjectNormalizer methods visibility for code reusability~~[Serializer] Change AbstractObjectNormalizer methods visibility for code reusability

Jun 13, 2022

NorthBlue333 commented

Jun 13, 2022

View reviewed changes

src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php Outdated

		@@ -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

NorthBlue333Jun 13, 2022•
edited
Loading

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 force-pushed thefeature/allow-extending-abstract-object-normalizer branch fromcda47e5 to7bbaab9Compare

June 13, 2022 11:40

NorthBlue333 commented

Jun 13, 2022

View reviewed changes

src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php Outdated

		*
		* 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

NorthBlue333Jun 13, 2022

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?

Copy link

Member

stof commentedJun 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 ?

Copy link

Author

NorthBlue333 commentedJun 14, 2022

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

Copy link

Member

dunglas commentedJun 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 force-pushed thefeature/allow-extending-abstract-object-normalizer branch 2 times, most recently from12dfe3d to7ff98eaCompare

October 23, 2022 11:32

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

Copy link

Author

NorthBlue333 commentedOct 23, 2022•
edited
Loading

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