Skip to content
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

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

Closed
annervisser opened this issue Aug 9, 2022 · 7 comments
Closed

Comments

@annervisser
Copy link

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';
    }
}
  1. 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
Copy link
Contributor

Hi @annervisser ,

Thank for the report.

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

Thanks!

@annervisser
Copy link
Author

Yes, that PR should fix the issue 👍
Thanks for the effort of putting my test case in as actual test 😄

@eerison
Copy link

eerison commented Mar 1, 2023

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

@annervisser
Copy link
Author

annervisser commented Mar 1, 2023

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

@eerison
Copy link

eerison commented Apr 3, 2023

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
Copy link
Author

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
Copy link

eerison commented Apr 4, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants