schmittjoh / serializer

Library for (de-)serializing data of any complexity (supports JSON, and XML)
http://jmsyst.com/libs/serializer
MIT License
2.32k stars 585 forks source link

Property Accessor called with default value of property since 3.18.0 #1421

Closed annervisser closed 1 year ago

annervisser commented 2 years ago
Q A
Bug report? yes
Feature request? no
BC Break report? yes
RFC? no

Properties that have an Accessor setter, are called with the default property value whenever that class is deserialized

This is a BC break since 3.17.1. It happens since 3.18.0, specifically PR #1417

Steps required to reproduce the problem

  1. Take an example class with both traditional and promoted properties, some with accessors:

    class DefaultValuesAndAccessors
    {
    public string $traditional = 'default';
    #[Accessor(setter: 'setTraditionalWithSetter')]
    public string $traditionalWithSetter = 'default';
    
    public function __construct(
        public string $promoted = 'default',
        #[Accessor(setter: 'setPromotedWithSetter')]
        public string $promotedWithSetter = 'default',
    ) {
    }
    
    public function setTraditionalWithSetter(string $value): void
    {
        $this->traditionalWithSetter = $value . '_fromsetter';
    }
    
    public function setPromotedWithSetter(string $value): void
    {
        $this->promotedWithSetter = $value . '_fromsetter';
    }
    }
  2. Deserialize some data into the class: $serializer->fromArray([], DefaultValuesAndAccessors::class)

Repo with reproduction: https://github.com/annervisser/jms-serializer-accessor-default-value-repro

Expected Result

{
    "traditional": "default",
    "traditionalWithSetter": "default",
    "promoted": "default",
    "promotedWithSetter": "default"
}

Actual Result

output in 3.17.1:

{
    "traditional": "default",
    "traditionalWithSetter": "default"
}

output in 3.18.0:

{
    "traditional": "default",
    "traditionalWithSetter": "default_fromsetter",
    "promoted": "default",
    "promotedWithSetter": "default_fromsetter"
}
joaojacome commented 2 years ago

Hi @annervisser ,

Thank for the report.

Would you be able to check if the pull request fixes your issue?

Thanks!

annervisser commented 2 years ago

Yes, that PR should fix the issue :+1: Thanks for the effort of putting my test case in as actual test :smile:

eerison commented 1 year ago

as I can see the PR was already merged, is it missing something else here?

annervisser commented 1 year ago

This should be fixed now in version https://github.com/schmittjoh/serializer/releases/tag/3.23.0 .

eerison commented 1 year ago

I'm still facing the same issue in release 3.23.0 :/

is it working for you on that version @annervisser ?

@annervisser do you mind to reopen this issue, I'm facing the same issue in 3.23.0

Edit: Sorry, I was just looking for the new fields that as added

{
    "traditional": "default",
    "traditionalWithSetter": "default",
+    "promoted": "default",
+    "promotedWithSetter": "default"
}

But I just realised that, the issue is because there is the suffix _fromsetter. and Yeah, in the last version it's as expected

{
    "traditional": "default",
    "traditionalWithSetter": "default",
    "promoted": "default",
    "promotedWithSetter": "default"
}

Then, it's working ✅ :)

annervisser commented 1 year ago

We were still facing an issue with 3.23.0, so we're still running 3.17.1. Something to do with deserializationHandlers, we'll research this later & submit a proper issue.

But glad to hear it's fixed for you @eerison

eerison commented 1 year ago

We were still facing an issue with 3.23.0, so we're still running 3.17.1. Something to do with deserializationHandlers, we'll research this later & submit a proper issue.

But glad to hear it's fixed for you @eerison

Yep, This issue was "fixed" (But we have a different output.)

But I'm also facing others issues