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] Deprecate AdvancedNameConverterInterface #20273

Open
wants to merge 3 commits into
base: 7.2
Choose a base branch
from

Conversation

mdoutreluingne
Copy link
Contributor

Fix #20266

Here, I don't know if I should modify or delete the .. note:: block, what do you think?

@mtarld
Copy link
Contributor

mtarld commented Sep 30, 2024

IMO yes, you should delete that note block.
And I think you should update the OrgPrefixNameConverter example as well

…ncedNameConverterInterface`` deprecated block
@mdoutreluingne
Copy link
Contributor Author

mdoutreluingne commented Oct 1, 2024

I've also deleted the deprecated block that I'd added because AdvancedNameConverterInterface no longer refers to it in the documentation, so I don't think it's relevant to leave it.

Why do I need to update the example OrgPrefixNameConverter? AdvancedNameConverterInterface does not refer in this example

I may have not understood the changes that could occur with your PR, could you explain?

@mtarld
Copy link
Contributor

mtarld commented Oct 2, 2024

It is because the NameConverterInterface signature has changed as well and triggers deprecations if not updated:

    public function normalize(string $propertyName/* , ?string $class = null, ?string $format = null, array $context = [] */): string;

It now must be used as the AdvancedNameConverterInterface was (same for the denormalization)

@mdoutreluingne
Copy link
Contributor Author

Thank you for taking the time to explain

I've updated the OrgPrefixNameConverter example. do you approve?

Copy link
Contributor

@mtarld mtarld left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @mdoutreluingne 🙂

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