Crell / Serde

Robust Serde (serialization/deserialization) library for PHP 8.
Other
296 stars 14 forks source link

Property default values not working #27

Closed pqt-edannenberg closed 1 year ago

pqt-edannenberg commented 1 year ago

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 commented 1 year ago

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

pqt-edannenberg commented 1 year ago

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 commented 1 year ago

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

Crell commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.