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

Improve the type descriptor for SimpleArrayType #438

Merged
merged 2 commits into from
Apr 6, 2023

Conversation

stof
Copy link
Contributor

@stof stof commented Mar 17, 2023

When converting database values to PHP, this type always produces a list<string>

@stof
Copy link
Contributor Author

stof commented Mar 22, 2023

@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.

@ondrejmirtes
Copy link
Member

This needs a test to understand what kind of error is this preventing. A new method in EntityColumnRuleTest should be fine.

@stof
Copy link
Contributor Author

stof commented Apr 5, 2023

@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.

@stof
Copy link
Contributor Author

stof commented Apr 5, 2023

And I found the fix for that issue.

@@ -160,13 +160,14 @@ public function processNode(Node $node, Scope $scope): array
return [];
}

$propertyTransformedType = TypeTraverser::map($propertyType, static function (Type $type, callable $traverse): Type {
// If the type descriptor does not precise the types inside the array, don't report errors if the field has a more precise type
Copy link
Member

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.

Copy link
Contributor Author

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)

Copy link
Contributor Author

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

Copy link
Member

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.

@ondrejmirtes
Copy link
Member

Please fix the build, otherwise good to go.

stof added 2 commits April 5, 2023 17:50
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.
@stof
Copy link
Contributor Author

stof commented Apr 5, 2023

@ondrejmirtes the build is now green

@ondrejmirtes ondrejmirtes merged commit 2c339ca into phpstan:1.3.x Apr 6, 2023
@ondrejmirtes
Copy link
Member

Thank you!

@stof stof deleted the patch-1 branch April 11, 2023 10:01
@stof
Copy link
Contributor Author

stof commented May 4, 2023

@ondrejmirtes do you have any idea when this will be released ?

@ondrejmirtes
Copy link
Member

I had to revert some blocking work that wasn't in ideal state, and I can now release this.

@ondrejmirtes
Copy link
Member

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;

@stof
Copy link
Contributor Author

stof commented May 4, 2023

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

@ondrejmirtes
Copy link
Member

Thank you!

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.

None yet

2 participants