jolicode / automapper

:rocket: Very FAST :rocket: PHP AutoMapper with on the fly code generation
https://automapper.jolicode.com/
MIT License
154 stars 15 forks source link

Handle array value casting #195

Open nlemoine opened 1 month ago

nlemoine commented 1 month ago

Hello,

This is more a question than a bug report.

Are array shapes supposed to be casted? To be more accurate:

<?php

class Dto
{
    public float $age;
}
$source = new Dto();
$target = $autoMapper->map(['age' => '10'], $source);

assert($target->age === 10.0); // ✅

class DtoArray
{
    /**
     * @var array<float>
     */
    public array $age;
}
$source = new DtoArray();
$target = $autoMapper->map(['age' => ['10']], $source);

assert($target->age === [10.0]); // ❌
// $target->age === ['10']

Would it be possible to handle array docblock types?

Korbeil commented 3 weeks ago

Hey @nlemoine and thanks for reaching us about your issue !

I added your case in my local test suite and reproduced it. I do think this is not even a behavior we do "on purpose", I think this is something we have because we don't add the declare(strict_types=1); on top of the generated mappers.

Also, at the moment we do not add it, but since https://github.com/jolicode/automapper/pull/180 you can have it in your mappers (it's a configuration that is false by default). When I added it onto your example I had the following result:

$ ./vendor/bin/phpunit --filter=testIssue195
PHPUnit 9.6.21 by Sebastian Bergmann and contributors.

E                                                                   1 / 1 (100%)

Time: 00:00.021, Memory: 14.00 MB

There was 1 error:

1) AutoMapper\Tests\AutoMapperTest::testIssue195
TypeError: Cannot assign string to property AutoMapper\Tests\Dto::$age of type float

/home/bleduc/Sites/korbeil/automapper/tests/cache/Mapper_array_AutoMapper_Tests_Dto.php:22
/home/bleduc/Sites/korbeil/automapper/src/AutoMapper.php:116
/home/bleduc/Sites/korbeil/automapper/tests/AutoMapperTest.php:1612

Since this is a configuration I would like to flip from false to true by default on next major release, your behavior won't even work anymore :confused:

I would say this is not something we want to support (array value casting) since we don't even support the "normal" casting and it's just a coincidence it is working at the moment. @joelwurtz @nikophil what do you think about this issue ?

Baptiste

MrMeshok commented 3 weeks ago

Hi everyone, I did have a thought about this issue earlier but didn't find the time to work on implementation. I think in theory we can extract array shape from doc block and create anonymous function to cast array values. It would look something like this:

$cast = static fn (float $value) => $value;
$values = [];
foreach ($value['age'] ?? [] as $key => $value_1) {
    $values[] = $cast($value_1);
}

And its works with either strictTypes setting, if we have it disabled value get casted, if it's enabled we get an error and we know something wrong with the source. Also example where it would behavior the same:

class DtoArray
{
    /**
     * @var array<float>
     */
    public array $age;
}

$source = ['age' => [10]];
$target = AutoMapper::create(new Configuration(strictTypes: true))->map($source, DtoArray::class);
assert($target->age === [10.0]); // It's allowed to cast int to float with declare(strict_types=1)
nlemoine commented 2 weeks ago

Salut @Korbeil !

Thanks for taking the time to provide such detailed feedback 🙂

Having the ability to cast weakly typed values would be a nice addition (maybe opt in, with a enableFlexibleCasting configuration setting, similar to Valinor). There are situations where you have to map poorly typed sources. However, I would totally understand if you think that's out of this library's scope.

Feel free to close the issue if this is an unwanted feature.