bug symfony#56868 [Serializer] fixed object normalizer for a class with cancel method (er1z)

This PR was merged into the 6.4 branch.

Discussion
----------

[Serializer] fixed object normalizer for a class with `cancel` method

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issue | symfony#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.

Commits
-------

8ccc0e4 [Serializer] fixed object normalizer for a class with `cancel` method

Author: fabpot

src/Symfony/Component/Serializer/Mapping/Loader/AttributeLoader.php

@@ -129,7 +129,7 @@ public function loadClassMetadata(ClassMetadataInterface $classMetadata): bool
             }
 
             $accessorOrMutator = preg_match('/^(get|is|has|set)(.+)$/i', $method->name, $matches);
-            if ($accessorOrMutator) {
+            if ($accessorOrMutator && !ctype_lower($matches[2][0])) {
                 $attributeName = lcfirst($matches[2]);
 
                 if (isset($attributesMetadata[$attributeName])) {

src/Symfony/Component/Serializer/Normalizer/GetSetMethodNormalizer.php

@@ -105,8 +105,8 @@ private function isGetMethod(\ReflectionMethod $method): bool
         return !$method->isStatic()
             && !($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')))
+            && ((2 < ($methodLength = \strlen($method->name)) && str_starts_with($method->name, 'is') && !ctype_lower($method->name[2]))
+                || (3 < $methodLength && (str_starts_with($method->name, 'has') || str_starts_with($method->name, 'get')) && !ctype_lower($method->name[3]))
             );
     }
 
@@ -118,7 +118,9 @@ private function isSetMethod(\ReflectionMethod $method): bool
         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])
+            ;
     }
 
     protected function extractAttributes(object $object, ?string $format = null, array $context = []): array

src/Symfony/Component/Serializer/Normalizer/ObjectNormalizer.php

@@ -100,7 +100,8 @@ protected function extractAttributes(object $object, ?string $format = null, arr
             $name = $reflMethod->name;
             $attributeName = null;
 
-            if (3 < \strlen($name) && match ($name[0]) {
+            // ctype_lower check to find out if method looks like accessor but actually is not, e.g. hash, cancel
+            if (3 < \strlen($name) && !ctype_lower($name[3]) && match ($name[0]) {
                 'g' => str_starts_with($name, 'get'),
                 'h' => str_starts_with($name, 'has'),
                 'c' => str_starts_with($name, 'can'),
@@ -112,7 +113,7 @@ protected function extractAttributes(object $object, ?string $format = null, arr
                 if (!$reflClass->hasProperty($attributeName)) {
                     $attributeName = lcfirst($attributeName);
                 }
-            } elseif ('is' !== $name && str_starts_with($name, 'is')) {
+            } elseif ('is' !== $name && str_starts_with($name, 'is') && !ctype_lower($name[2])) {
                 // issers
                 $attributeName = substr($name, 2);
 

src/Symfony/Component/Serializer/Tests/Fixtures/Attributes/AccessorishGetters.php

@@ -0,0 +1,39 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <[email protected]>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Serializer\Tests\Fixtures\Attributes;
+
+class AccessorishGetters
+{
+    public function hash(): void
+    {
+    }
+
+    public function cancel()
+    {
+    }
+
+    public function getField1()
+    {
+    }
+
+    public function isField2()
+    {
+    }
+
+    public function hasField3()
+    {
+    }
+
+    public function setField4()
+    {
+    }
+}

src/Symfony/Component/Serializer/Tests/Mapping/Loader/AttributeLoaderTestCase.php

@@ -21,6 +21,7 @@
 use Symfony\Component\Serializer\Mapping\Loader\AnnotationLoader;
 use Symfony\Component\Serializer\Mapping\Loader\AttributeLoader;
 use Symfony\Component\Serializer\Mapping\Loader\LoaderInterface;
+use Symfony\Component\Serializer\Tests\Fixtures\Attributes\AccessorishGetters;
 use Symfony\Component\Serializer\Tests\Mapping\Loader\Features\ContextMappingTestTrait;
 use Symfony\Component\Serializer\Tests\Mapping\TestClassMetadataFactory;
 
@@ -212,6 +213,22 @@ public function testLoadGroupsOnClass()
         self::assertSame(['a'], $attributesMetadata['baz']->getGroups());
     }
 
+    public function testIgnoresAccessorishGetters()
+    {
+        $classMetadata = new ClassMetadata(AccessorishGetters::class);
+        $this->loader->loadClassMetadata($classMetadata);
+
+        $attributesMetadata = $classMetadata->getAttributesMetadata();
+
+        self::assertCount(4, $classMetadata->getAttributesMetadata());
+
+        self::assertArrayHasKey('field1', $attributesMetadata);
+        self::assertArrayHasKey('field2', $attributesMetadata);
+        self::assertArrayHasKey('field3', $attributesMetadata);
+        self::assertArrayHasKey('field4', $attributesMetadata);
+        self::assertArrayNotHasKey('h', $attributesMetadata);
+    }
+
     /**
      * @group legacy
      */

src/Symfony/Component/Serializer/Tests/Normalizer/GetSetMethodNormalizerTest.php

@@ -515,6 +515,14 @@ public function testNormalizeWithDiscriminator()
         $this->assertSame(['type' => 'one', 'url' => 'URL_ONE'], $normalizer->normalize(new GetSetMethodDiscriminatedDummyOne()));
     }
 
+    public function testNormalizeWithMethodNamesSimilarToAccessors()
+    {
+        $classMetadataFactory = new ClassMetadataFactory(new AttributeLoader());
+        $normalizer = new GetSetMethodNormalizer($classMetadataFactory);
+
+        $this->assertSame(['class' => 'class', 123 => 123], $normalizer->normalize(new GetSetWithAccessorishMethod()));
+    }
+
     public function testDenormalizeWithDiscriminator()
     {
         $classMetadataFactory = new ClassMetadataFactory(new AttributeLoader());
@@ -902,3 +910,46 @@ public function setBar($bar = null, $other = true)
         $this->bar = $bar;
     }
 }
+
+class GetSetWithAccessorishMethod
+{
+    public function cancel()
+    {
+        return 'cancel';
+    }
+
+    public function hash()
+    {
+        return 'hash';
+    }
+
+    public function getClass()
+    {
+        return 'class';
+    }
+
+    public function setClass()
+    {
+    }
+
+    public function get123()
+    {
+        return 123;
+    }
+
+    public function set123()
+    {
+    }
+
+    public function gettings()
+    {
+    }
+
+    public function settings()
+    {
+    }
+
+    public function isolate()
+    {
+    }
+}

src/Symfony/Component/Serializer/Tests/Normalizer/ObjectNormalizerTest.php

@@ -937,6 +937,24 @@ public function testObjectNormalizerWithAttributeLoaderAndObjectHasStaticPropert
         $normalizer = new ObjectNormalizer(new ClassMetadataFactory(new AttributeLoader()));
         $this->assertSame([], $normalizer->normalize($class));
     }
+
+    public function testNormalizeWithMethodNamesSimilarToAccessors()
+    {
+        $classMetadataFactory = new ClassMetadataFactory(new AttributeLoader());
+        $normalizer = new ObjectNormalizer($classMetadataFactory);
+
+        $object = new ObjectWithAccessorishMethods();
+        $normalized = $normalizer->normalize($object);
+
+        $this->assertFalse($object->isAccessorishCalled());
+        $this->assertSame([
+            'accessorishCalled' => false,
+            'tell' => true,
+            'class' => true,
+            'responsibility' => true,
+            123 => 321
+        ], $normalized);
+    }
 }
 
 class ProxyObjectDummy extends ObjectDummy
@@ -1219,3 +1237,63 @@ class ObjectDummyWithIgnoreAttributeAndPrivateProperty
 
     private $private = 'private';
 }
+
+class ObjectWithAccessorishMethods
+{
+    private $accessorishCalled = false;
+
+    public function isAccessorishCalled()
+    {
+        return $this->accessorishCalled;
+    }
+
+    public function cancel()
+    {
+        $this->accessorishCalled = true;
+    }
+
+    public function hash()
+    {
+        $this->accessorishCalled = true;
+    }
+
+    public function canTell()
+    {
+        return true;
+    }
+
+    public function getClass()
+    {
+        return true;
+    }
+
+    public function hasResponsibility()
+    {
+        return true;
+    }
+
+    public function get_foo()
+    {
+        return 'bar';
+    }
+
+    public function get123()
+    {
+        return 321;
+    }
+
+    public function gettings()
+    {
+        $this->accessorishCalled = true;
+    }
+
+    public function settings()
+    {
+        $this->accessorishCalled = true;
+    }
+
+    public function isolate()
+    {
+        $this->accessorishCalled = true;
+    }
+}