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

Rename object to data in NormalizerInterface::normalize #54880

Open
wants to merge 1 commit into
base: 7.2
Choose a base branch
from

Conversation

maxbeckers
Copy link
Contributor

@maxbeckers maxbeckers commented May 10, 2024

Q A
Branch? 7.2
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #54774
License MIT

Rename sttribure of Serializer::normalize to the naming of the interface to make the usage of named attributes possible.

From the discussion of this PR I changed it to rename the first parameter of NormalizerInterface::normalize from object to data because it can't handle only objects, it's type mixed.

@maxbeckers maxbeckers requested a review from dunglas as a code owner May 10, 2024 09:46
@maxbeckers maxbeckers force-pushed the patch-54774_correct_naming_in_serializer branch from 8440c4a to b2e70e5 Compare May 15, 2024 07:12
@nicolas-grekas
Copy link
Member

nicolas-grekas commented May 15, 2024

I think we'd better not do this: named arguments are out of the BC promise, so using named arguments is a terrible argument for this change :) Also, this interface accepts mixed, not only objects, so $data is a more appropriate name. We might want to change the interface instead.

@russelomua
Copy link

russelomua commented May 15, 2024

I was anticipating a discussion, so I was surprised by the sudden appearance of this PR. I agree, @nicolas-grekas, but would modifying the interface be a BC?

@maxbeckers
Copy link
Contributor Author

Yeah i just opened this PR to push the discussion about it because it took just a few seconds to prepare this PR. I'm open to close this PR with "Won't Fix" or even take the topic to change the interface as Nicolas mentioned.
I can take it with me and change the interface and all related classes to bring them in sync.

And when we want to change it, is it really a bugix or should we do this in 7.2?

@scruwi
Copy link
Contributor

scruwi commented May 16, 2024

IMO those interface changes are rather BC than changes of implemantation.

@nicolas-grekas
Copy link
Member

Named arguments out of our BC promise. Also, changing argument names on interfaces cannot break existing code since PHP maps to arguments found in concrete implementations, not in abstract ones.

@nicolas-grekas
Copy link
Member

That'd be for 7.2, it's not a bug fix but just a tweak.

@maxbeckers maxbeckers force-pushed the patch-54774_correct_naming_in_serializer branch from b2e70e5 to 7a35fc4 Compare May 16, 2024 11:51
@carsonbot carsonbot changed the title [Serializer] Add correct signature to normalize to allow named attribures Add correct signature to normalize to allow named attribures May 16, 2024
@maxbeckers maxbeckers changed the base branch from 5.4 to 7.1 May 16, 2024 11:51
@maxbeckers maxbeckers changed the title Add correct signature to normalize to allow named attribures Add correct signature to normalize for type mixed May 16, 2024
@maxbeckers maxbeckers force-pushed the patch-54774_correct_naming_in_serializer branch 2 times, most recently from c669d62 to 6e486bc Compare May 16, 2024 13:13
@nicolas-grekas
Copy link
Member

I'd suggest changing only the interface

@maxbeckers
Copy link
Contributor Author

Don't we want to keep the other classes in sync with the interface?

@xabbuh xabbuh added this to the 7.2 milestone May 16, 2024
@maxbeckers maxbeckers force-pushed the patch-54774_correct_naming_in_serializer branch from 6e486bc to ca8f75c Compare May 16, 2024 14:03
@stof
Copy link
Member

stof commented May 16, 2024

Renaming all implementations will increase the risk of conflicts when merging between branches.
And the argument name in our implementations does not really matter for users of Symfony (as we don't support named parameters in our BC policy anyway, and covering them with BC would have prevented the renaming entirely).

@maxbeckers maxbeckers force-pushed the patch-54774_correct_naming_in_serializer branch from ca8f75c to a5c4dba Compare May 17, 2024 05:15
@maxbeckers maxbeckers changed the title Add correct signature to normalize for type mixed Rename object to data in NormalizerInterface::normalize May 17, 2024
…malize`` from object to data because of type mixed
@maxbeckers maxbeckers force-pushed the patch-54774_correct_naming_in_serializer branch from a5c4dba to 26983eb Compare May 17, 2024 05:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants