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 default values not working #27

Closed
pqt-edannenberg opened this issue Sep 18, 2023 · 8 comments · Fixed by #31
Closed

Property default values not working #27

pqt-edannenberg opened this issue Sep 18, 2023 · 8 comments · Fixed by #31

Comments

@pqt-edannenberg
Copy link

Detailed description

Not sure if I'm missing something obvious or if this is a regression in Serde, but I can't seem to get property default values for deserialization working at all:

use Crell\Serde\Attributes\Field;
use Crell\Serde\SerdeCommon;

class Foo
{
    public function __construct(
        #[Field(default: null)]
        public ?int  $id,
        public string $name
    ) {
        //
    }        
}

$serde = new SerdeCommon();
$newFoo = $serde->deserialize(['name' => 'foobar'], 'array', Foo::class);
dump($newFoo);
= Foo {#6214
    +name: "foobar",
  }

I have tried a couple different ways, even the php default values in the constructor are not working.

Your environment

$ composer show -t crell/serde
crell/serde 0.6.0 A general purpose serialization and deserialization library
├──crell/attributeutils ~0.8.2
│  ├──crell/fp ~0.4.0
│  │  └──php ~8.1
│  └──php ~8.1
├──crell/fp >= 0.3.3
│  └──php ~8.1
└──php ~8.1
@oprypkhantc
Copy link

Just stumbled upon this myself. They work with native default values IF they come after ones without default values.

@pqt-edannenberg
Copy link
Author

Sprinkled some debugs into the Serde source and for the test case above the shouldUseDefault property currently always evaluates to false. After forcing it to true and using a default value other than null it worked as expected.

@Crell
Copy link
Owner

Crell commented Sep 20, 2023

The tests already cover a whole bunch of default values. Is this limited to when the default value being set is null, perhaps?

@Crell
Copy link
Owner

Crell commented Sep 20, 2023

Also, try on the master branch. There's been a lot of improvements around null handling recently that may have already resolved this.

@pqt-edannenberg
Copy link
Author

The tests already cover a whole bunch of default values. Is this limited to when the default value being set is null, perhaps?

No luck. Tested with current master:

class Foo
{
    public function __construct(
        #[Field(default: "42")]
        public string $bar,
        public string $name
    ) {
        //
    }
}

$serde = new SerdeCommon();
dump($serde->deserialize(['name' => 'foobar'], 'array', Foo::class));

Foo {#6350
  +name: "foobar"
}
$ composer show -t crell/serde
crell/serde dev-master A general purpose serialization and deserialization library
├──crell/attributeutils ~0.8.2
│  ├──crell/fp ~0.4.0
│  │  └──php ~8.1
│  └──php ~8.1
├──crell/fp >= 0.3.3
│  └──php ~8.1
└──php ~8.1

@Crell
Copy link
Owner

Crell commented Sep 22, 2023

Resolved in #31. I'm shocked that bug managed to survive so long, honestly. I could have sworn there was a test for that case. But it's been resolved, and supports null as a default value, too. (Which required some fun enum dancing.)

@pqt-edannenberg
Copy link
Author

Thanks for the swift fix! One more thing I noticed while working with defaults, would it make sense to also add a default arg to SequenceField and DictionaryField? It felt a bit inconsistent to me.

@Crell
Copy link
Owner

Crell commented Sep 22, 2023

The Field default should work for any field type. SequenceField et al are TypeFields, which are type-specific sub-attributes. You absolutely can and should use both Field and SequenceField on the same property.

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

Successfully merging a pull request may close this issue.

3 participants