Skip to content

[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

Merged
merged 1 commit into from
Nov 9, 2024

Conversation

er1z
Copy link
Contributor

@er1z er1z commented May 21, 2024

Q A
Branch? 6.4
Bug fix? yes
New feature? no
Deprecations? no
Issue #58652
License MIT

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 produce failed queue message. And eventually, found out that attributes list of ObjectNormalizer contains a field cel 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.1 canners were introduced, alongside with issers and hassers. But can prefix is also applicable to a word canCel and provided PHP methods are case-insensitive, getting list of attributes caused accidental call of cancel method breaking business logic.

See attached unit test for better explanation.

@er1z er1z requested a review from dunglas as a code owner May 21, 2024 19:07
@carsonbot carsonbot added this to the 6.4 milestone May 21, 2024
@carsonbot
Copy link

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:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
  • Features and deprecations must be submitted against the 7.2 branch.

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!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@carsonbot

This comment has been minimized.

@er1z
Copy link
Contributor Author

er1z commented May 21, 2024

I can see that unit tests failures come from totally different area and the same failures repeat across different PRs. How should we proceed?

@carsonbot carsonbot changed the title fixed object normalizer for a class with cancel method [Serializer] fixed object normalizer for a class with cancel method May 22, 2024
@OskarStark
Copy link
Contributor

I can see that unit tests failures come from totally different area and the same failures repeat across different PRs. How should we proceed?

You can ignore them for now

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] ?? '')
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

@nicolas-grekas nicolas-grekas Jun 25, 2024

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

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

@nicolas-grekas
Copy link
Member

Please rebase, and please get rid of the merge commit meanwhile.
Note that even after rebasing, tests don't pass yet.
Can you also please add test cases for get_foo / get123 methods?

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]);

@er1z er1z force-pushed the object-normalizer-fix branch 2 times, most recently from 600a81d to ebfbf16 Compare September 21, 2024 17:56
@er1z
Copy link
Contributor Author

er1z commented Sep 21, 2024

Ok, rebased, caught with mainstream and extended some cases. The only thing I don't fully get is the one with get_ as it's done with name converter right?

@fabpot
Copy link
Member

fabpot commented Nov 9, 2024

Thank you @er1z.

@fabpot fabpot merged commit 8dabfd7 into symfony:6.4 Nov 9, 2024
10 checks passed
This was referenced Nov 13, 2024
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.

7 participants