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

feat: option to generate code with strict types #180

Closed MrMeshok closed 2 months ago

MrMeshok commented 2 months ago

PHP, by default, will coerce values of the wrong type into the expected scalar type declaration if possible. This leads to unexpected behavior. For example, losing decimal digits when casting float to int

readonly class IntDto
{
    public function __construct(
        public int $var,
    ) {}
}

$autoMapper = \AutoMapper\AutoMapper::create();
$result = $autoMapper->map(['var' => 1.1111], IntDto::class);
$result->var; // var is 1

This PR adds configuration to add declare(strict_types=1) to generated code when mapping, so instead of unexpected behavior, you get TypeError.

If everything is ok, I will update the docs, but I am not very fluent with english, so maybe someone else could write a more understandable description.

joelwurtz commented 2 months ago

:+1: for this thanks, can you add the possibility to configure this through the Mapper attribute also ?

Also wondering if declare strict should be true by default ? poke @Korbeil @nikophil ?

If you could update the doc and add a note in the changelog it would be nice, if you are not comfortable doing so we can do it.

Korbeil commented 2 months ago

I'm totally for this option and I think we should put it by default as @joelwurtz stated. Same as he said, we would require a configuration on the Mapper attribute :pray:

nikophil commented 2 months ago

won't we face some BC break if this becomes a default thing?

MrMeshok commented 2 months ago

Made it configurable from Mapper and enabled by default. I'll wait for the decision about the default value and force push if it needs to be changed. I will leave the changelog and docs for you

Korbeil commented 2 months ago

won't we face some BC break if this becomes a default thing?

This is right, we should put it disabled by default and we will put it enabled by default on next major ~ Could you make the change @MrMeshok ? :pray:

MrMeshok commented 2 months ago

@Korbeil Done👍

MrMeshok commented 2 months ago

Added note to CHANGELOG and combined everything in 1 commit

Korbeil commented 2 months ago

Thanks for the feature @MrMeshok ! :pray: