Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Serializer] fix default context in Serializer#59552
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
[Serializer] fix default context in Serializer#59552
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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 make the method non-internal or even keep it as is on 7.3, instead of making it a constructor parameter as proposed in#58889.
| * | ||
| * @param array<string, mixed> $defaultContext | ||
| */ | ||
| public function setDefaultContext(array $defaultContext): void |
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.
this should be added as a constructor argument to me
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 prefer the approach in#58889.
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.
It looks cleaner for me too. Should I add more details to say that it's a fix up to 7.3?
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.
Shall we merge this as a bugfix for branches up to 7.3?
Closing in favor of#58889 (which should target 6.4 now) |
Bug fix approach for#58889