Skip to content

[Serializer] ObjectNormalizer throws exception normalizing classes with the method get #58012

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.

Sign up for GitHub

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

Jump to bottom
Closed
mihai-stancu opened this issue Aug 14, 2024 · 2 comments
Closed

[Serializer] ObjectNormalizer throws exception normalizing classes with the method get #58012

mihai-stancu opened this issue Aug 14, 2024 · 2 comments
Labels
Bug Serializer Status: Needs Review

Comments

@mihai-stancu
Copy link
Contributor

mihai-stancu commented Aug 14, 2024 •
edited
Loading

Symfony version(s) affected

>= 6.4.x

Description

If a class has a method called get that method will get confused for an accessor and it will go through logic relating to property-access for the property named substr('get', 3).

Eventually a new PropertyPath instance will be created with an empty path which will trigger an exception.

How to reproduce

$serializer->serialize(new class { 
    public function get() {} 
}, 'json');

Possible Solution

A naive solution ObjectNormalizer::isAllowedAttribute() at line 193

-                self::$isReadableCache[$class.$attribute] = (\is_object($classOrObject) && $this->propertyAccessor->isReadable($classOrObject, $attribute)) || $this->propertyInfoExtractor->isReadable($class, $attribute) || $this->hasAttributeAccessorMethod($class, $attribute);
+                self::$isReadableCache[$class.$attribute] = (\is_object($classOrObject) && !empty($attribute) && $this->propertyAccessor->isReadable($classOrObject, $attribute)) || $this->propertyInfoExtractor->isReadable($class, $attribute) || $this->hasAttributeAccessorMethod($class, $attribute);

But most likely this object would need to be skipped earlier in the normalization process, perhaps here:

-            if (str_starts_with($name, 'get') || str_starts_with($name, 'has') || str_starts_with($name, 'can')) {
+            if ($name !== 'get' && str_starts_with($name, 'get')
+            || $name !== 'has' && str_starts_with($name, 'has')
+            || $name !== 'can' && str_starts_with($name, 'can')) {
                // getters, hassers and canners
                $attributeName = substr($name, 3);

                if (!$reflClass->hasProperty($attributeName)) {
                    $attributeName = lcfirst($attributeName);
                }
-            } elseif (str_starts_with($name, 'is')) {
+            } elseif ($name !== 'is' && str_starts_with($name, 'is')) {
                // issers
                $attributeName = substr($name, 2);

                if (!$reflClass->hasProperty($attributeName)) {
                    $attributeName = lcfirst($attributeName);
                }
            }

Additional Context

Exception is triggered around here $propertyAccessor->isReadable() instantiates a PropertyPath.

// ObjectNormalizer::isAllowedAttribute() line 193
        if ($context['_read_attributes'] ?? true) {
            if (!isset(self::$isReadableCache[$class.$attribute])) {
                self::$isReadableCache[$class.$attribute] = (\is_object($classOrObject) && $this->propertyAccessor->isReadable($classOrObject, $attribute)) || $this->propertyInfoExtractor->isReadable($class, $attribute) || $this->hasAttributeAccessorMethod($class, $attribute);
            }

            return self::$isReadableCache[$class.$attribute];
        }
@vidarl vidarl mentioned this issue Aug 20, 2024
Closed
@mihai-stancu mihai-stancu changed the title ObjectNormalizer throws exception normalizing classes with the method get [Serializer] ObjectNormalizer throws exception normalizing classes with the method get Aug 24, 2024
@mihai-stancu
Copy link
Contributor Author

Is the diff I provided of interest? I can write-up a PR for it.

@mtarld
Copy link
Contributor

mtarld commented Sep 9, 2024

Hey @mihai-stancu

Indeed, it'd be great to fix that, we'd love a PR for that, thanks!

@mihai-stancu mihai-stancu mentioned this issue Sep 10, 2024
Merged
nicolas-grekas added a commit that referenced this issue Sep 16, 2024
@nicolas-grekas
bug #58226 [Serializer] Fix for method named get() (mihai-stancu)
c49d6db 
This PR was squashed before being merged into the 6.4 branch.

Discussion
----------

[Serializer] Fix for method named `get()`

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #58012
| License       | MIT

During normalization, if a class has a method called `get` that method will get confused for an accessor and it will go through logic relating to property-access for the property named `substr('get', 3)`.

Eventually a new PropertyPath instance will be created with an empty path which will trigger an exception.
```php
$serializer->serialize(new class {
    public function get() {}
}, 'json');
```

Commits
-------

a0d6e26 [Serializer] Fix for method named `get()`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Serializer Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@nicolas-grekas @mihai-stancu @xabbuh @mtarld @carsonbot