Add PHPStan check for iterable types #2437

franmomu · 2022-04-30T11:26:40Z

Q	A
Type	improvement
BC Break	no
Fixed issues

Summary

Last PR to add the PHPStan check for iterable types, the remaining ones where the ones in tests.

malarzm

I'm not saying "definitely no", but given few definitions are off and tests are green I'm not sure these changes provide any real value other than satisfying SA tools. Maybe we could stay with level 5 for tests and improve just the main codebase for higher levels? To add another pro some time ago while working on sanity checks I needed to ignore phpstan/psalm errors in tests because I wanted to pass wrong values to check if exceptions are raised properly.

tests/Doctrine/ODM/MongoDB/Tests/Aggregation/AggregationOperatorsProviderTrait.php

malarzm · 2022-05-04T22:43:01Z

tests/Doctrine/ODM/MongoDB/Tests/Aggregation/Stage/ProjectTest.php

@@ -64,6 +64,8 @@ public function provideAccumulators(): array
    }

    /**
+     * @param mixed[] $args


FWIW I'm not really a fan of adding mixed[] all over the place, IMO this is bringing little to no value

malarzm · 2022-05-04T22:46:05Z

tests/Doctrine/ODM/MongoDB/Tests/Events/LifecycleListenersTest.php

@@ -225,6 +225,9 @@ class MyEventListener
    /** @psalm-var array<string, list<class-string>> */
    public $called = [];

+    /**
+     * @param mixed[] $args


Can we somehow note that first element should be an instance of LifecycleEventArgs (and we don't care about the rest)?

malarzm · 2022-05-04T22:47:20Z

tests/Doctrine/ODM/MongoDB/Tests/Functional/CommitImprovementTest.php

@@ -162,6 +162,9 @@ public function getSubscribedEvents(): array
        ];
    }

+    /**
+     * @param mixed[] $args


Same question as before with LifecycleEventArgs

malarzm · 2022-05-04T22:50:52Z

tests/Doctrine/ODM/MongoDB/Tests/Functional/ReadPreferenceTest.php

@@ -43,6 +43,8 @@ public function testHintIsNotSetByDefault(): void
    }

    /**
+     * @param array<array<string, string>> $tags


Shall we introduce a ReadPreferenceTagsShape shape similar to SortShape?

malarzm · 2022-05-04T22:57:07Z

tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH560Test.php

@@ -114,6 +117,9 @@ public function getSubscribedEvents(): array
        return $this->events;
    }

+    /**
+     * @param mixed[] $args


Here goes LifecycleEventArgs question again

malarzm · 2022-05-04T22:58:07Z

tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/MODM83Test.php

@@ -63,6 +63,9 @@ class MODM83EventListener
    /** @var array<string, class-string[]> */
    public $called = [];

+    /**
+     * @param mixed[] $args


I'm just marking all places with LifecycleEventArgs I see in case there'd be a place to improve (next 2 classes could use this as well)

malarzm · 2022-05-04T23:01:06Z

tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/MODM95Test.php

@@ -78,7 +78,7 @@ public function __construct()
     * by mapping array indexes (size URL's are required, cropMetadata is not).
     * Any invalid elements will be ignored.
     *
-     * @param array|Traversable $embeddedDocuments
+     * @param array<MODM95TestEmbeddedDocument>|Traversable<MODM95TestEmbeddedDocument> $embeddedDocuments


Both @param and code could be improved to use iterable<MODM95TestEmbeddedDocument> which wasn't available at the time of writing the test

malarzm · 2022-05-04T23:03:24Z

tests/Doctrine/ODM/MongoDB/Tests/Persisters/PersistenceBuilderTest.php

@@ -229,6 +231,10 @@ public function getDocumentsAndExpectedData(): array
        ];
    }

+    /**
+     * @param array<string, mixed>      $expectedData
+     * @param array<string, mixed>|null $preparedData


I doubt that null should be allowed for $preparedData as the very first thing we do is iterate over it :)

malarzm · 2022-05-04T23:07:10Z

tests/Doctrine/ODM/MongoDB/Tests/UnitOfWorkTest.php

@@ -249,6 +249,9 @@ public function testComputingChangesetForFileWithoutMetadataThrowsNoError(): voi
    }

    /**
+     * @param array<string, string>|null $origData
+     * @param array<string, string>|null $updateData


There are currently also ints and bools in the array

franmomu · 2022-11-03T23:48:28Z

I'm not saying "definitely no", but given few definitions are off and tests are green I'm not sure these changes provide any real value other than satisfying SA tools. Maybe we could stay with level 5 for tests and improve just the main codebase for higher levels? To add another pro some time ago while working on sanity checks I needed to ignore phpstan/psalm errors in tests because I wanted to pass wrong values to check if exceptions are raised properly.

Thanks @malarzm for the review! I've finally found some time to take a look at this again. I've just updated the code with the comments and make it green improving a bit some of the mixed[] types.

I'll try to add another commit removing some of the phpdoc that don't add value and adding them to the baseline.

malarzm · 2022-11-04T22:43:01Z

Awesome! Let me know if you'd need anything :)

They don't add value

franmomu · 2022-11-05T13:08:35Z

I think it's ready to review, I understand the mixed[] vs array concern, to me when I see mixed[] it's like the type has been reviewed, but maybe list<mixed> would be better to express that it is list of mixed types and the key doesn't matter.

malarzm

Thank you very much @franmomu for your hard work on this 🍻

malarzm · 2022-11-06T19:56:22Z

tests/Doctrine/ODM/MongoDB/Tests/Events/LifecycleListenersTest.php

@@ -224,6 +225,9 @@ class MyEventListener
    /** @psalm-var array<string, list<class-string>> */
    public array $called = [];

+    /**
+     * @param array{LifecycleEventArgs} $args


Cool this was possible :)

franmomu added the Enhancement label Apr 30, 2022

franmomu added this to the 2.4.0 milestone Apr 30, 2022

malarzm requested changes May 4, 2022

View reviewed changes

malarzm modified the milestones: 2.4.0, 2.5.0 May 8, 2022

Add PHPStan check for iterable types

b1f7bfd

franmomu force-pushed the specify_iterator_types_tests branch from c532cf0 to d6a5fa1 Compare November 3, 2022 23:31

franmomu changed the base branch from 2.4.x to 2.5.x November 3, 2022 23:33

franmomu closed this Nov 3, 2022

franmomu reopened this Nov 3, 2022

franmomu force-pushed the specify_iterator_types_tests branch 2 times, most recently from 7763fc5 to a035a32 Compare November 3, 2022 23:41

franmomu force-pushed the specify_iterator_types_tests branch 2 times, most recently from d4679c9 to 4c337ea Compare November 4, 2022 00:14

franmomu added 4 commits November 5, 2022 13:44

Add iterable types

b2a9348

Remove unexisting ArraySubset class

5295286

Specify IndexOptions keys

fee2938

Remove some iterable types

babf193

They don't add value

franmomu force-pushed the specify_iterator_types_tests branch from 4c337ea to babf193 Compare November 5, 2022 12:44

franmomu requested a review from malarzm November 5, 2022 13:08

malarzm approved these changes Nov 6, 2022

View reviewed changes

malarzm merged commit 0981d3d into doctrine:2.5.x Nov 6, 2022

franmomu deleted the specify_iterator_types_tests branch November 7, 2022 06:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PHPStan check for iterable types #2437

Add PHPStan check for iterable types #2437

franmomu commented Apr 30, 2022

malarzm left a comment

malarzm May 4, 2022

malarzm May 4, 2022

malarzm May 4, 2022

malarzm May 4, 2022

malarzm May 4, 2022

malarzm May 4, 2022

malarzm May 4, 2022

malarzm May 4, 2022

malarzm May 4, 2022

franmomu commented Nov 3, 2022 •

edited

Loading

malarzm commented Nov 4, 2022

franmomu commented Nov 5, 2022

malarzm left a comment

malarzm Nov 6, 2022

Add PHPStan check for iterable types #2437

Add PHPStan check for iterable types #2437

Conversation

franmomu commented Apr 30, 2022

Summary

malarzm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franmomu commented Nov 3, 2022 • edited Loading

malarzm commented Nov 4, 2022

franmomu commented Nov 5, 2022

malarzm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franmomu commented Nov 3, 2022 •

edited

Loading