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

feat: option to generate code with strict types #180

Merged
merged 2 commits into from
Sep 3, 2024

Conversation

MrMeshok
Copy link
Contributor

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
Copy link
Member

👍 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
Copy link
Member

Korbeil commented Aug 30, 2024

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 🙏

@nikophil
Copy link
Collaborator

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

@MrMeshok
Copy link
Contributor Author

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

@@ -24,7 +24,8 @@ public function __construct(
public readonly array $propertiesMetadata,
public readonly bool $checkAttributes = true,
public readonly ConstructorStrategy $constructorStrategy = ConstructorStrategy::AUTO,
public bool $allowReadOnlyTargetToPopulate = false,
public readonly bool $allowReadOnlyTargetToPopulate = false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like readonly was missed here

@Korbeil
Copy link
Member

Korbeil commented Sep 3, 2024

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 ? 🙏

@MrMeshok
Copy link
Contributor Author

MrMeshok commented Sep 3, 2024

@Korbeil Done👍

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.

Looks good to me 👌

Could you add a note about your feature into the CHANGELOG please ?

@MrMeshok
Copy link
Contributor Author

MrMeshok commented Sep 3, 2024

Added note to CHANGELOG and combined everything in 1 commit

@Korbeil Korbeil merged commit 8fa3035 into jolicode:main Sep 3, 2024
5 checks passed
@Korbeil
Copy link
Member

Korbeil commented Sep 3, 2024

Thanks for the feature @MrMeshok ! 🙏

@MrMeshok MrMeshok deleted the strict-types branch September 3, 2024 12:59
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.

4 participants