dereuromark / cakephp-dto

CakePHP DTO plugin - quickly generate useful data transfer objects for your app (mutable/immutable)
MIT License
24 stars 6 forks source link

allow int as float type fix for immutables #59

Closed jorisvaesen closed 1 year ago

jorisvaesen commented 2 years ago

When using json_encode in PHP, we can preserve a float as float thanks to the JSON_PRESERVE_ZERO_FRACTION flag. But since not all programming languages have this implementation, passing a float as 1 instead of 1.0 makes filling/creating the dto from an array fail when using immutables. For immutables created from an array, variables are never set through their setters but rather directly. This validates the use of typechecking, but in this specific case, an int should be allowed to be set as float since this would also work for setters.

dereuromark commented 2 years ago

This would be the only exception on auto-"cast", right? Or are there other cases where casts seem needed?

What is the concrete case here that requires this?

I wonder if we instead should add a "cast" config with key=>value (int=>float) or alike to make it more agnostic to all kind of possible casting options?

jorisvaesen commented 2 years ago

This was the only place I encountered this problem, despite frequent use.

I went for this simple solution so as not to add too much extra complexity and to be able to write simple tests.

Should further cast issues arise, I favor a assertTypeMap along the lines of:

$assertTypeMap = [
 'float' => ['float', 'int'],
 ...
];
dereuromark commented 2 years ago

In your initial comment, can you state the reason why this is needed or useful? Maybe with some code examples to clarify?

I wonder if it wouldnt be better to cast manually before adding the data into the DTO

dereuromark commented 1 year ago

Is there any update on this or shall we close this for now?

jorisvaesen commented 1 year ago

I don't see any reason for casting the variable before setting it in this case since, even with strict types enabled, it is allowed to pass an int as float-parameter.

declare(strict_types = 1);

function run(float $a): float {
   return $a;
}

run(1);

I also don't see any other typecheck problems which would justify a larger adjustment in the code.

dereuromark commented 1 year ago

Usually I would say lets keep as much magic out of it as possible, especially if you are working with type safe env E.g. float vs int as param type is usually quite the difference, right? It would trigger an error with strict types on if you pass int to float?

public function foo(float $float)

But in general I would agree that we could for your use case allow this to be merged here now.

jorisvaesen commented 1 year ago

It does not trigger an error with strict types enabled, as my example in the previous comment works without errors thrown.

When using mutables, setters are used and there is no problem (since php accepts int as float type). https://github.com/dereuromark/cakephp-dto/blob/ebc57efcd7dd52757b47793a3bcb665cebd3a909/src/Dto/Dto.php#L314 When using immutables, there is a typecheck since using setters would clone the object (itself in this case) and the context would be lost. https://github.com/dereuromark/cakephp-dto/blob/ebc57efcd7dd52757b47793a3bcb665cebd3a909/src/Dto/Dto.php#L317

I really think this implementation is safe and to my knowledge, is the only exception needed in the typecheck.

dereuromark commented 1 year ago

Cool, ok, thanks for the clarification

dereuromark commented 1 year ago

You need to fix the build though (PHPStan is failing)

codecov-commenter commented 1 year ago

Codecov Report

Merging #59 (ebc57ef) into master (92f9711) will increase coverage by 0.07%. The diff coverage is 100.00%.

:exclamation: Current head ebc57ef differs from pull request most recent head 222899a. Consider uploading reports for the commit 222899a to get more accurate results

@@             Coverage Diff              @@
##             master      #59      +/-   ##
============================================
+ Coverage     81.73%   81.80%   +0.07%     
- Complexity      460      462       +2     
============================================
  Files            17       17              
  Lines          1040     1039       -1     
============================================
  Hits            850      850              
+ Misses          190      189       -1     
Impacted Files Coverage Δ
src/Dto/Dto.php 80.42% <100.00%> (+0.14%) :arrow_up:
src/Generator/Finder.php 100.00% <0.00%> (+7.14%) :arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more