-
Notifications
You must be signed in to change notification settings - Fork 112
Improve the type descriptor for SimpleArrayType #438
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
Conversation
@ondrejmirtes any chance to merge this ? Even though it does not solve all issues related to this SimpleArrayType (due to #436), it improves the situation.
This needs a test to understand what kind of error is this preventing. A new method in EntityColumnRuleTest should be fine.
@ondrejmirtes I added more tests but they fail. It looks like the @var list<string> is transformed into array<int, mixed> somewhere when checking the EntityColumnRule.
And I found the fix for that issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree with this. If Doctrine can write any array to a property, it's wrong to expect a narrower type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is about explaining the existing logic that you added in e745610
The change I'm doing here is disabling this logic for some cases (when the type descriptor is precise), not enabling it, to avoid reporting errors that are wrong.
I tried to remove this transformation entirely, but this broke some existing tests (the ones added in that commit)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note however that the only type descriptors that return array as type (and so would trigger this logic) are json_array (deprecated in DBAL 2.10 and removed in DBAL 3.0) and array (deprecated in DBAL 3.4+).
So maybe we could drop this logic and return NeverType as done in the JsonType descriptor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I get it now, it's nice 👍
I'm not rushing to remove functionality, people use phpstan-doctrine with years-old versions of Doctrine.
Please fix the build, otherwise good to go.
When converting database values to PHP, this type always produces a `list<string>`
The EntityColumnRule was transforming the property type to avoid reporting errors for array types, to allow precising the types inside those arrays on the PHP side. However, this was breaking checks when the type descriptor was actually more precise.
@ondrejmirtes the build is now green
Thank you!
@ondrejmirtes do you have any idea when this will be released ?
I had to revert some blocking work that wasn't in ideal state, and I can now release this.
Hey, I've got some false positives because of this PR, can you please tell me what should be done?
------ ----------------------------------------------------------------------------------------------
Line app/services/PointOfInterest/PointOfInterest.php
------ ----------------------------------------------------------------------------------------------
109 Property Slevomat\PointOfInterest\PointOfInterest::$seasonMonths type mapping mismatch:
database can contain list<string> but property expects array<int, float|int|numeric-string>.
109 Property Slevomat\PointOfInterest\PointOfInterest::$seasonMonths type mapping mismatch:
property can contain array<int, float|int|string> but database expects array<string>.
------ ----------------------------------------------------------------------------------------------
------ ------------------------------------------------------------------------------------------------
Line app/services/Product/Cancel/ProductCancel.php
------ ------------------------------------------------------------------------------------------------
99 Property Slevomat\Product\Cancel\ProductCancel::$goodsStatuses type mapping mismatch: database
can contain array<int, string>|null but property expects array<int, int>|null.
99 Property Slevomat\Product\Cancel\ProductCancel::$goodsStatuses type mapping mismatch: property
can contain array<int, int>|null but database expects array<string>|null.
------ ------------------------------------------------------------------------------------------------
For these properties:
/** @var array<int, numeric> */ #[ORM\Column(type: 'simple_array')] private array $seasonMonths = []; /** @var array<int, int>|null */ #[ORM\Column(type: 'simple_array')] private ?array $goodsStatuses = null;
This reports an actual issue. When loaded from the DB, the simple_array does a explode(), so values in the array are strings, not integers. So there is indeed a type mismatch.
See https://github.com/doctrine/dbal/blob/47589fdf56929f1510fd9b394c328c31a7ce13fb/src/Types/SimpleArrayType.php#L53-L62 for the conversion code
Thank you!
When converting database values to PHP, this type always produces a
list<string>