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

[make:serializer:normalizer] Inject a NormalizerInterface instead of an ObjectNormalizer #1273

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

mtarld
Copy link
Contributor

@mtarld mtarld commented Jan 19, 2023

Related to #1252

This PR aims to do not rely anymore on constructor argument injection and use NormalizerAwareInterface/NormalizerAwareTrait instead.

Moreover, this allows to not inject explicitly the ObjectNormalizer anymore which can be considered as a bad practice as it is a concrete implementation of the NormalizerInterface.

@mtarld
Copy link
Contributor Author

mtarld commented Jan 20, 2023

Actually, after talking with @chalasr, we went to the conclusion that it is better to inject the ObjectNormalizer through the constructor.

Therefore, this PR was updated to:

  • inject a NormalizerInterface in the normalizer constructor
  • update the service definition in order to inject explicitly the ObjectNormalizer

@mtarld mtarld changed the title [make:normalizer] Use NormalizerAwareInterface [make:normalizer] Inject a NormalizerInterface instead of an ObjectNormalizer Jan 20, 2023
Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

👍 NormalizerAwareTrait seems to not be much appreciated by the community, and I tend to agree - it feels complicated. Also using it implies to deal with some special context flag to workaround an infinite loop. Regular DI looks good!

Copy link
Collaborator

@jrushlow jrushlow left a comment

Choose a reason for hiding this comment

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

This looks good! Thank you @mtarld

@jrushlow jrushlow changed the title [make:normalizer] Inject a NormalizerInterface instead of an ObjectNormalizer [make:serializer:normalizer] Inject a NormalizerInterface instead of an ObjectNormalizer Feb 20, 2024
@jrushlow jrushlow merged commit c953816 into symfony:main Feb 20, 2024
9 checks passed
@jrushlow jrushlow mentioned this pull request Feb 20, 2024
@mtarld mtarld deleted the fix/normalizer branch February 20, 2024 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants