Skip to content

[Serializer] Convert names using denormalize during denormalization #50042

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

Closed
wants to merge 1 commit into from

Conversation

wouterj
Copy link
Member

@wouterj wouterj commented Apr 17, 2023

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Might be related to #49808
License MIT
Doc PR -

This code is called during denormalization, but it was wrongly calling the normalize function of the name converter. This bug exists since the start of name converters (e14854f). I believe we never caught this bug as the ObjectNormalizer would simply set the property using the name converter in the correct direction after initialization.

However, with the recent readonly feature this does not work as the constructor sets it to a value and thus the fallback of ObjectNormalizer errors with "Cannot modify readonly property".

It might be a bit risky to change this direction in an LTS release, but that is only because people were relying on buggy behavior.

@nicolas-grekas
Copy link
Member

Any way to test this?

@wouterj wouterj force-pushed the issue-49808/name-converter-bug branch from 1ebca9a to 044caa7 Compare April 18, 2023 20:14
@wouterj
Copy link
Member Author

wouterj commented Apr 18, 2023

I've added a test (which is more like an integration test than a unit test, but that seems "normal" in the AbstractNormalizerTest).

This proves that I was wrong and the logic is correct. So this is no longer a bug, but we do not support inputs that already have the correct name format as input to normalization.

@wouterj wouterj removed the Bug label Apr 18, 2023
@wouterj
Copy link
Member Author

wouterj commented Apr 18, 2023

I'll close for now, as I have no time to investigate how to support this case further. If someone has, you can take my commit as a starting point.

@wouterj wouterj closed this Apr 18, 2023
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.

4 participants