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

fix: NullableTransformer should check if array key exists #7

Merged
merged 1 commit into from
Oct 3, 2023

Conversation

nikophil
Copy link
Collaborator

@nikophil nikophil commented Oct 3, 2023

when mapping from array to an object, if a property is nullable and is not present in the original object, there should be no error if the property defaults to null.

currently, this code:

readonly class SomeClass
{
    public function __construct(public int|null $foo = null)
    {
    }
}

$autoMapper->map([], SomeClass::class);

generates the following code:

// ...

            } else {
                $value_1 = null;
                if (isset($value['foo'])) {
                    $value_1 = $value['foo'];
                }
                $constructarg = $value_1;
            }

// ...

and results in an error where foo key does not exist, although we provided a default error.

At first, I wanted to add an isset() only if the property has a default value, but it appears that it is a little bit complex: NullableTransformerFactory would need to access to property info to get the write info of the property... I think this would be overkill for the case where the property is nullable and adding a isset() everywhere we have a array access is enough and not harmful.

@nikophil nikophil force-pushed the fix/handle-when-key-not-exists branch from df043be to f165f3d Compare October 3, 2023 11:01
Copy link
Member

@Korbeil Korbeil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fear it can break some user behavior but since there was no issues with current tests, let's merge this 👍

Thanks for the contribution @nikophil !

@Korbeil Korbeil merged commit 57dc1ce into jolicode:main Oct 3, 2023
2 checks passed
@nikophil
Copy link
Collaborator Author

nikophil commented Oct 3, 2023

I fear it can break some user behavior but since there was no issues with current tests, let's merge this 👍

I don't think so:

  • property is not nullable: this PR is not involved
  • property is nullable, and key is provided: no problem as it used to be
  • property is nullable but input is not an array: this PR is not involved

If property key is missing in the input array:

  • property is nullable and has default null: this is the specific use case I wanted to fix, the property will be set to null
  • property is nullable and has default to something else than null: the property will be set to null (see Default values in target are not handled #8)
  • property is nullable and has no default: in this case, it used to create an error, but now the property is defaulted to null. But I don't think how this could be a problem

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 this pull request may close these issues.

None yet

2 participants