Add some property typehints #2362

franmomu · 2021-09-02T06:58:51Z

Q	A
Type	improvement
BC Break	no
Fixed issues

Summary

This add some property type hints to tests directory, I've changed a couple of things that I would point out in the code. Since there are a lot missing, I'll try to make manageable PRs.

franmomu · 2021-09-02T07:03:19Z

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

+     *
+     * @var string|null
+     */
+    public $name = '';


name is always initialized:

mongodb-odm/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH1294Test.php

Lines 15 to 21 in 1577336

$user1 = new GH1294User();

$user1->id = 'aaa111aaa';

$user1->name = 'Steven';

$user2 = new GH1294User();

$user2->id = 'bbb111bbb';

$user2->name = 'Jeff';

franmomu · 2021-09-02T07:05:47Z

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

@@ -76,7 +76,7 @@ public function testFindWithOrOnCollectionWithDiscriminatorMap(): void

        $sameCollection1->id   = $ids[0];
        $sameCollection1->name = 'First entry in SameCollection1';
-        $sameCollection1->test = 1;
+        $sameCollection1->test = 'test';


It is used as string:

mongodb-odm/tests/Doctrine/ODM/MongoDB/Tests/Persisters/PersistenceBuilderTest.php

Lines 55 to 63 in 1577336

->field('test')->set('OK! TEST')

->field('id')->equals($id);

$query = $qb->getQuery();

$query->execute();

$this->dm->refresh($testCollection);

$this->assertEquals('OK! TEST', $testCollection->test);

malarzm

I'm gonna trust you and all our tests with this :)

malarzm · 2021-09-02T07:42:03Z

lib/Doctrine/ODM/MongoDB/Configuration.php

    public function setClassMetadataFactoryName(string $cmfName): void
    {
        $this->attributes['classMetadataFactoryName'] = $cmfName;
    }

+    /**
+     * @psalm-return class-string


Let's be consistent with multi-line vs single-lined PHPDocs :)

done, I've used multiline for PHPDoc in methods (I've found a way to force this from phpcs), the single-lined one is forced by phpcs when only has one single line content.

IonBazan · 2021-09-02T07:45:15Z

lib/Doctrine/ODM/MongoDB/Configuration.php

@@ -87,8 +87,11 @@ class Configuration
     * @psalm-var array{
     *      autoGenerateHydratorClasses?: self::AUTOGENERATE_*,
     *      autoGeneratePersistentCollectionClasses?: self::AUTOGENERATE_*,
+     *      classMetadataFactoryName?: class-string,


Let's try to limit allowed classes here. It appears that it should only accept \Doctrine\ODM\MongoDB\Mapping\ClassMetadataFactory. In fact, the class is marked as final so I can't imagine anyone extending it but you can still create your own implementation that adds setDocumentManager and getDocumentManager methods (there is no common interface for that).

sure, I've used class-string<ClassMetadataFactory> and modified also defaultGridFSRepositoryClassName and classMetadataFactoryName, thanks!

Can you also apply that to the getters and setters where applicable?

yeah, should be better now, thanks.

IonBazan · 2021-09-02T07:45:52Z

lib/Doctrine/ODM/MongoDB/Configuration.php

     *      defaultCommitOptions?: CommitOptions,
     *      defaultDocumentRepositoryClassName?: string,
+     *      defaultGridFSRepositoryClassName?: class-string,


This should implement \Doctrine\ODM\MongoDB\Repository\GridFSRepository

IonBazan · 2021-09-02T07:47:22Z

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

@@ -68,6 +72,7 @@ public function testLifecycleListeners(): void
        $this->listener->called = [];

        $document = $dm->find(TestDocument::class, $test->id);
+        assert($document->embedded instanceof PersistentCollectionInterface);


Can we use self::assertInstanceOf() together with phpstan-phpunit plugin instead?

franmomu added the Task label Sep 2, 2021

franmomu commented Sep 2, 2021

View reviewed changes

malarzm reviewed Sep 2, 2021

View reviewed changes

malarzm approved these changes Sep 2, 2021

View reviewed changes

IonBazan reviewed Sep 2, 2021

View reviewed changes

franmomu force-pushed the add_types_1 branch from 1577336 to 62d22a2 Compare September 2, 2021 10:28

franmomu added 2 commits September 2, 2021 13:11

Add missing array keys

1587ece

Add missing property type hints

1c89d70

franmomu force-pushed the add_types_1 branch from 62d22a2 to 1c89d70 Compare September 2, 2021 11:12

IonBazan approved these changes Sep 2, 2021

View reviewed changes

franmomu merged commit 0f64911 into doctrine:2.3.x Sep 2, 2021

franmomu deleted the add_types_1 branch September 2, 2021 11:59

franmomu mentioned this pull request Sep 2, 2021

Add more missing property type hints #2363

Merged

malarzm added this to the 2.3.0 milestone Sep 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add some property typehints #2362

Add some property typehints #2362

franmomu commented Sep 2, 2021

franmomu Sep 2, 2021

franmomu Sep 2, 2021

malarzm left a comment

malarzm Sep 2, 2021

franmomu Sep 2, 2021

IonBazan Sep 2, 2021

franmomu Sep 2, 2021

IonBazan Sep 2, 2021

franmomu Sep 2, 2021

IonBazan Sep 2, 2021

IonBazan Sep 2, 2021

franmomu Sep 2, 2021

	$user1 = new GH1294User();
	$user1->id = 'aaa111aaa';
	$user1->name = 'Steven';

	$user2 = new GH1294User();
	$user2->id = 'bbb111bbb';
	$user2->name = 'Jeff';

	->field('test')->set('OK! TEST')
	->field('id')->equals($id);

	$query = $qb->getQuery();
	$query->execute();

	$this->dm->refresh($testCollection);

	$this->assertEquals('OK! TEST', $testCollection->test);

Add some property typehints #2362

Add some property typehints #2362

Conversation

franmomu commented Sep 2, 2021

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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