[Serializer] Fix normalizing objects with accessors having the same name as a property

[RafaelKr]

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

Notes

Although technically this is a bugfix and its pretty much an edge-case, this may affect existing projects and may alter their serialization results. So it could be a breaking change.

Edit: Also see comments below for further edge cases.

Before/After comparison

class MyClass {
  private string $owner = 'foo';
  private bool $isOwner = true;

  public function getOwner(): string {
    return $this->owner;
  }

  public function setOwner(string $owner): void {
    $this->owner = $owner;
  }

  public function isOwner(): bool {
    return $this->isOwner;
  }

  public function setIsOwner(bool $isOwner): void {
    $this->isOwner = $isOwner;
  }
}

$classMetadataFactory = new ClassMetadataFactory(new AttributeLoader());
$normalizer = new ObjectNormalizer($classMetadataFactory);

$object = new MyClass();
$normalized = $normalizer->normalize($object);

Before this PR

It was normalized as:

[
  'owner' => 'foo',
]

Removing the $owner property and getters would result in:

[
  'owner' => true,
]

After this PR

It will be normalized as:

[
  'owner' => 'foo',
  'isOwner'  => true,
]

[RafaelKr]

Another Edge-Case

After I just found #61023 I also tried having properties and getter methods called isFoo and canFoo (no getFoo at this time) in the ObjectWithBooleanPropertyAndIsserWithSameName class:

class ObjectWithBooleanPropertyAndIsserWithSameName
{
    private $foo = 'foo';

    private $isFoo = true;

    private $canFoo = false;

    public function isFoo()
    {
        return $this->isFoo;
    }

    public function canFoo()
    {
        return $this->canFoo;
    }
}

This results in

[
  'isFoo' => true,
  'foo' => true, // it's using the isFoo isser method!
]

Notice, that for the canFoo property it's using the isFoo method to get the value and prints foo as result name.

Reading through the linked PR I wonder if my PR also classifies as "new feature" instead of "bugfix"?
cc @nicolas-grekas

[RafaelKr]

Extended Edge-Case

class ObjectWithBooleanPropertyAndIsserWithSameName
{
    public $foo = 'foo';

    private $getFoo = 'getFoo';

    private $hasFoo = 'hasFoo';

    private $canFoo = 'canFoo';

    private $isFoo = 'isFoo';

    public function getFoo()
    {
        return $this->getFoo;
    }

    public function hasFoo()
    {
        return $this->hasFoo;
    }

    public function canFoo()
    {
        return $this->canFoo;
    }

    public function isFoo()
    {
        return $this->isFoo;
    }
}

Results in

[
  'foo' => 'getFoo',
  'isFoo' => 'isFoo'
]

So we may also want to add the $reflClass->hasProperty($name) check I added for issers for get, has and can?

That would result in:

[
    'getFoo' => 'getFoo',
    'hasFoo' => 'hasFoo',
    'canFoo' => 'canFoo',
    'isFoo' => 'isFoo',
    'foo' => 'getFoo'
]

[RafaelKr]

What do you think, how should we proceed here? Should we do the hasProperty check I added for issers for all getter-like methods? Then we would do the check for:

• getters
• hassers
• canners
• issers

IMO it would definitely make sense for has, can and is. For get it's debatable.

[nicolas-grekas]

👎 on my side, this looks extremely specific to one use case. I don't think the new behavior would be a generic expectation. This looks too much into private details.

[RafaelKr]

Hey @nicolas-grekas, thanks for chiming in. I just want to give a little summary, why I think the current state can lead to unnoticed bugs which may be hard to debug.

I had this little sidenote in the linked issue #61096:

Sidenote: When using the JetBrains IDE Generate Getter feature on a boolean property called like $isX, it will automatically create an isser method with the same name like the property. This is how I ran into this.

So imagine the following steps:

• Create a class with the property private bool $isOwner = true;
• Use the "Generate Getter" feature of JetBrains. By default this creates a getter with the signature public function isOwner(): bool
• Create a class instance and set this property to false
• Use the Symfony Serializer with the default configuration to serialize the value
• Deserialize the output back to an instance of that class: isOwner is never assigned. It will have its default value (true) instead of false.

The problem is, that de-/serialization doesn't behave deterministically in this case. The issue has a more detailed description why.

Although my implementation might not be the way to go, I really would like to have a solution for this case.

I get your point, that this looks into private details, but the current state (deciding on getters, canners, hassers and issers) also already does this. So if it's done in one way (serialization), it should also work in the opposite direction (deserialization). I don't really think, that this is extremely specific and it definitely would be my expectation, that it should work in both directions. What do you think?

[nicolas-grekas]

the current state (deciding on getters, canners, hassers and issers) also already does this

I forgot this aspect indeed. This voids my argument.

👍 (I updated the implementation to make it a bit more generic, doing the same for all accessors)

[RafaelKr]

Thank you very much, this is exactly what I would expect.

I first noticed it for issers, that's why I implemented it only there. Afterwards I saw it would make sense for all accessors, but wanted to get feedback by Symfony Core members. Thanks for understanding and adjusting the PR!

Edit: Also I love your adjustment of the match-statement. Very readable and clever to assign $i the way you did to get rid of the separate is-ifelse-branch.

[nicolas-grekas]

Thank you @RafaelKr.

[RafaelKr]

@nicolas-grekas Shouldn't there be also tests for all accessor methods?

[nicolas-grekas]

There could yes. Since everything is in the same codepath now, I didn't bother, but feel free to submit another PR of course!

[RafaelKr]

@nicolas-grekas please see #61660

While doing the changes I notices, that can is not classified as an accessor on the 6.4 branch here:

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

Line 131 in de58845

$accessorOrMutator = preg_match('/^(get|is|has|set)(.+)$/i', $method->name, $matches);

It was added to the 7.4 branch:
fb1da0b

Should this maybe be backported?

Also another place, where can is not accounted for:

symfony/src/Symfony/Component/Validator/Mapping/Loader/AttributeLoader.php

Line 90 in 1cdaa51

if (preg_match('/^(get|is|has)(.+)$/i', $method->name, $matches)) {

Should I create seperate issues for those?

[nicolas-grekas]

Support for canners has been added as a new feature, it cannot be backported (but it can be added to 7.4 for places where it's missing - if that makes sense)

[RafaelKr]

@nicolas-grekas, oh. This branch was merged into 6.4
https://github.com/symfony/symfony/blob/6.4/src/Symfony/Component/Serializer/Normalizer/ObjectNormalizer.php#L106

And it also already supported can before this PR (this is one commit on 6.4 before this PR was merged):

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

Line 106 in 6310c44

'c' => str_starts_with($name, 'can'),

Or do you only talk about the Validator AttributeLoader?

[nicolas-grekas]

I mean Validator AttributeLoader yes

[RafaelKr]

Okay, but the fb1da0b should still be backported, if Serializer already also supported can at 6.4, right?

[nicolas-grekas]

Well, that was discussed a bit in #61023
Feel free to open a PR if you want to reopen the discussion of course.

[RafaelKr]

I'm not sure if a new PR tackling only this one place is a good idea. For future consistency it may be a better idea to define all default accessor (and mutator?) methods in a place shared accross all Symfony components, if that's possible.

@stof I'm pinging you in reference to your comment at #61023 (comment). What do you think about this situation?

TLDR: We're talking about the discrepancy of the can accessor method, which is supported in the ObjectNormalizer since Symfony 6.1 (c3cb4f5) but not in the AttributeLoader (previously AnnotationLoader), where it was added for Symfony 7.4 in #61023

As one part of the Serializer (the Normalizer) already supports it, but another part (the AttributeLoader) doesn't, it may classify as a bugfix.