-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Serializer] fixed object normalizer for a class with cancel
method
#56868
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
Conversation
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
This comment has been minimized.
This comment has been minimized.
I can see that unit tests failures come from totally different area and the same failures repeat across different PRs. How should we proceed? |
cancel
methodcancel
method
src/Symfony/Component/Serializer/Normalizer/ObjectNormalizer.php
Outdated
Show resolved
Hide resolved
You can ignore them for now |
src/Symfony/Component/Serializer/Tests/Normalizer/ObjectNormalizerTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Serializer/Normalizer/ObjectNormalizer.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Serializer/Tests/Normalizer/ObjectNormalizerTest.php
Show resolved
Hide resolved
if (str_starts_with($name, 'get') || str_starts_with($name, 'has') || str_starts_with($name, 'can')) { | ||
if ( | ||
(str_starts_with($name, 'get') || str_starts_with($name, 'has') || str_starts_with($name, 'can')) | ||
&& ctype_upper($name[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.
As you said in the description methods are case insensitive in PHP, so this looks too brittle 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 barely see any other option that won't imply having a list of exclusions. If you have any better idea, share please.
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.
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 think this would work for me. It'd just also allow _
next to the prefix so that we don't break snake cased methods.
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.
@nicolas-grekas isn't this a responsibility of NameConverter here?
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.
Actually it might be more robust to check with !ctype_lower
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.
@nicolas-grekas why?
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.
because it would account for methods named get_foo or get123, while the current logic doesn't
Please rebase, and please get rid of the merge commit meanwhile. I also think GetSetMethodNormalizer needs a similar change: --- a/src/Symfony/Component/Serializer/Normalizer/GetSetMethodNormalizer.php
+++ b/src/Symfony/Component/Serializer/Normalizer/GetSetMethodNormalizer.php
@@ -106,7 +106,7 @@ class GetSetMethodNormalizer extends AbstractObjectNormalizer
&& !($method->getAttributes(Ignore::class) || $method->getAttributes(LegacyIgnore::class))
&& !$method->getNumberOfRequiredParameters()
&& ((2 < ($methodLength = \strlen($method->name)) && str_starts_with($method->name, 'is'))
- || (3 < $methodLength && (str_starts_with($method->name, 'has') || str_starts_with($method->name, 'get')))
+ || (3 < $methodLength && (str_starts_with($method->name, 'has') || str_starts_with($method->name, 'get')) && !ctype_lower($method->name[3]))
);
}
@@ -118,7 +118,8 @@ class GetSetMethodNormalizer extends AbstractObjectNormalizer
return !$method->isStatic()
&& !$method->getAttributes(Ignore::class)
&& 0 < $method->getNumberOfParameters()
- && str_starts_with($method->name, 'set');
+ && str_starts_with($method->name, 'set')
+ && !ctype_lower($method->name[3]); |
600a81d
to
ebfbf16
Compare
Ok, rebased, caught with mainstream and extended some cases. The only thing I don't fully get is the one with |
50f2452
to
8ccc0e4
Compare
Thank you @er1z. |
During the debug of quite big application I found out that at unrelated edge cases a class got called
cancel
method. It turned out that as a part of outbox pattern, Serializer kicked in to producefailed
queue message. And eventually, found out that attributes list ofObjectNormalizer
contains a fieldcel
that didn't exist anywhere.Eventually, it turned out that for default
ObjectNormalizer
configuration, getters are also utilized to fetch list of attributes. But since Symfony 6.1canners
were introduced, alongside withissers
andhassers
. Butcan
prefix is also applicable to a wordcanCel
and provided PHP methods are case-insensitive, getting list of attributes caused accidental call ofcancel
method breaking business logic.See attached unit test for better explanation.