Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Fixing Bug #249 #251

Merged
merged 8 commits into from
Jun 2, 2022
Merged

Fixing Bug #249 #251

merged 8 commits into from
Jun 2, 2022

Conversation

gerpo
Copy link
Contributor

@gerpo gerpo commented Oct 27, 2021

Default values work with MapFrom.

The issue with assign null to type, was that the default value was ignored if the mapping was not present.
Now it will fall back to the default if the mapped property is not present.

Adam Marciniak and others added 2 commits October 27, 2021 10:50
Default values work with MapFrom
@aidan-casey
Copy link
Collaborator

@gerpo - I think this is the right direction, especially by using the reflection property's getDefaultValue. A few quick notes:

  • getDefaultValue returns null if one is not set, so the hasDefaultValue check is unnecessary.
  • I don't believe the DataTransferObjectProperty class should set the value. Let's have a method called getDefaultValue return the default value, and use that in the DataTransferObject constructor instead of the current null.

Make sense?

Adam Marciniak added 2 commits October 27, 2021 12:55
Removing unnecessary check and making setting more explicit.
Removing unnecessary check and making setting more explicit.
@gerpo
Copy link
Contributor Author

gerpo commented Oct 27, 2021

@aidan-casey
Sure, the points make absolutely sense. Committed the changes.

@spatie-bot
Copy link

Dear contributor,

because this pull request seems to be inactive for quite some time now, I've automatically closed it. If you feel this pull request deserves some attention from my human colleagues feel free to reopen it.

@spatie-bot spatie-bot closed this Mar 23, 2022
@stevebauman
Copy link

This should be re-opened 👍

@freekmurze freekmurze reopened this May 30, 2022
@aidan-casey
Copy link
Collaborator

@gerpo - Can you modify this PR to use the default parameter for Arr::get() as opposed to the null coalescing operator? I'll release this patch once that is done.

@gerpo
Copy link
Contributor Author

gerpo commented Jun 2, 2022

@gerpo - Can you modify this PR to use the default parameter for Arr::get() as opposed to the null coalescing operator? I'll release this patch once that is done.

Done :)

@aidan-casey aidan-casey merged commit 83c04ce into spatie:main Jun 2, 2022
@aidan-casey
Copy link
Collaborator

Thank you, @gerpo. Apologies for the delay!

@joecampo
Copy link

joecampo commented Jun 2, 2022

This is awesome. I literally just ran into this today. Talk about good timing. Thanks @aidan-casey & @gerpo you're lifesavers. ❤️

@sebdesign
Copy link
Contributor

I think this is introducing a new bug, or if it's intended, that's a breaking change.

Before this PR, properties that had a default value, would be assigned the default value if the argument was null.
This was because of the null coalescing operator.
If Arr::get($args, $property->name) was null, then $this->{$property->name} would return the default value.

Now with Arr::get($args, $property->name, $property->getDefaultValue()), If the $args array has a key named $property->name, it will return its value whether it's null or not. The $property->getDefaultValue() will only be returned when the $args array does NOT have this key.

The following test was green until this PR.

$data = [
    'title' => 'Hello world',
    'description' => null,
];

$dto = new class ($data) extends DataTransferObject {
    public string $title;

    public string $description = 'Test Text';
};

$this->assertEquals('Hello world', $dto->title);
$this->assertEquals('Test Text', $dto->description);

Now this test throws an error:

TypeError: Cannot assign null to property Spatie\DataTransferObject\DataTransferObject@anonymous::$description of type string

/vendor/spatie/data-transfer-object/src/Reflection/DataTransferObjectProperty.php:48
/vendor/spatie/data-transfer-object/src/DataTransferObject.php:29

@aidan-casey
Copy link
Collaborator

aidan-casey commented Jun 22, 2022

@sebdesign - Please see #282. The behavior you are describing was never intended, documented, or a supported feature. It is not a breaking change worthy of a major release because it was a bug that was fixed in this PR.

@pr4xx
Copy link

pr4xx commented Sep 7, 2022

For me this introduced a new bug in my application. I have a base class which extends DataTransferObject. In the constructor, just before calling parent::__construct();, I set some default values which I cannot set just using the class properties (for example object instantiation). Prior to this update this worked:

class BaseDTO extends DataTransferObject
{
    public MyObject $myObject;
    
    public function __construct(...$args)
    {
        // set default
        $this->myObject = new MyObject();
        // call parent
        parent::__construct(...$args);
    }
}

Now this fails because when the parent constructor runs, it tries to set $myObject to null because it has no default value. Prior to this update it would have used the existing instance created from me.
What would be the correct way to assign complex default values prior to assigning data from json/array?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
8 participants