[2.0] Add type hints #1846

caciobanu · 2018-08-06T20:21:38Z

Q	A
Type	improvement
BC Break	yes
Fixed issues	#1704

caciobanu · 2018-08-06T20:26:21Z

This is work in progress and only UnitOfWork & DocumentManager are done for now.

Some @param & @return docs are invalid so I think that a decision has to be taken which are supported and which not. Take a look at https://github.com/doctrine/mongodb-odm/pull/1846/files#diff-c5b64e39a058b30afece06cc7cd217b7R387 which must support https://github.com/doctrine/mongodb-odm/blob/master/lib/Doctrine/ODM/MongoDB/Query/Builder.php#L109

alcaeus

Early code review. As a general rule, don't rely on the null default value to make a type nullable, instead make it explicit.

It may also be beneficial to re-enable these two sniffs in our coding standard to help you with the transition: https://github.com/doctrine/mongodb-odm/blob/215e5e667474b230d41f40fac25a623106bacfed/phpcs.xml.dist#L27,,L28. Mainly, this will apply "easy" type hints when they are already clear-cut due to the @param typehint, potentially saving you tons of manual work.

Speaking of saving you from work, you can leave out any type hints from classes involved with Proxies: these will be replaced as part of #1813 so there's no point adding type hints now.

Last but not least, if you want to have separate commits for your changes, please make them descriptive. However, as it is now we'll most likely end up squashing this into one large commit.

alcaeus · 2018-08-07T06:02:13Z

lib/Doctrine/ODM/MongoDB/DocumentManager.php

@@ -524,25 +499,16 @@ public function merge($document)
     * @param int    $lockVersion
     * @throws \InvalidArgumentException
     */
-    public function lock($document, $lockMode, $lockVersion = null)
+    public function lock(object $document, int $lockMode, int $lockVersion = null): void


?int $lockVersion = null please.

alcaeus · 2018-08-07T06:02:42Z

lib/Doctrine/ODM/MongoDB/LockException.php

-     * @param object|null $document
-     */
-    public function __construct($msg, $document = null)
+    public function __construct(string $msg, object $document = null)


?object $document = null

alcaeus · 2018-08-07T06:04:44Z

lib/Doctrine/ODM/MongoDB/Repository/DocumentRepository.php

     */
-    public function findBy(array $criteria, ?array $sort = null, $limit = null, $skip = null)
+    public function findBy(array $criteria, ?array $orderBy = null, $limit = null, $offset = null): array


$orderBy and $offset are incorrect - they are called $sort and $skip respectively to reflect the naming in MongoDB: db.collection.find().sort(...).limit(...).skip(...).

alcaeus · 2018-08-07T06:05:19Z

lib/Doctrine/ODM/MongoDB/SchemaManager.php

     */
-    public function ensureIndexes($timeout = null)
+    public function ensureIndexes(int $timeout = null): void


?int $timeout = null

alcaeus · 2018-08-07T06:05:26Z

lib/Doctrine/ODM/MongoDB/SchemaManager.php

     */
-    public function updateIndexes($timeout = null)
+    public function updateIndexes(int $timeout = null): void


?int $timeout = null

alcaeus · 2018-08-07T06:06:43Z

lib/Doctrine/ODM/MongoDB/UnitOfWork.php

     */
-    public function getDocumentState($document, $assume = null)
+    public function getDocumentState(object $document, int $assume = null): int


?int $assume = null

alcaeus · 2018-08-07T06:07:07Z

lib/Doctrine/ODM/MongoDB/UnitOfWork.php

     * @throws InvalidArgumentException If the entity instance is NEW.
     * @throws LockException If the document uses optimistic locking through a
     *                       version attribute and the version check against the
     *                       managed copy fails.
     */
-    private function doMerge($document, array &$visited, $prevManagedCopy = null, $assoc = null)
+    private function doMerge(object $document, array &$visited, object $prevManagedCopy = null, array $assoc = null): object


?object $prevManagedCopy = null, ?array $assoc = null

alcaeus · 2018-08-07T06:07:23Z

lib/Doctrine/ODM/MongoDB/UnitOfWork.php

     * @throws LockException
     * @throws \InvalidArgumentException
     */
-    public function lock($document, $lockMode, $lockVersion = null)
+    public function lock(object $document, int $lockMode, int $lockVersion = null): void


?int $lockVersion = null

alcaeus · 2018-08-07T06:07:31Z

lib/Doctrine/ODM/MongoDB/UnitOfWork.php

     */
-    public function clear($documentName = null)
+    public function clear(string $documentName = null): void


?string $documentName = null

alcaeus · 2018-08-07T06:07:51Z

lib/Doctrine/ODM/MongoDB/UnitOfWork.php

     * @internal Highly performance-sensitive method.
     */
-    public function getOrCreateDocument($className, $data, &$hints = [], $document = null)
+    public function getOrCreateDocument(string $className, array $data, array &$hints = [], object $document = null): object


?object $document = null

caciobanu · 2018-08-07T06:48:42Z

Thanks for the feedback!

I'll do a rebase when I'll finish. I'm doing this step by step to be easier to fix failing tests or to update type hints. And yes, I know about Doctrine\Common\Proxy being replaced with ocramius/proxy-manager so I will ignore involved classes.

caciobanu · 2018-08-07T10:45:12Z

@alcaeus I'm not sure about the code in lib/Doctrine/ODM/MongoDB/Aggregation. Is it worth it to add type hints there ? For some classes I couldn't find any usages.

alcaeus · 2018-08-07T11:09:58Z

Most of the arguments to the methods in aggregation pipeline stages accept multiple arguments: a scalar value, an array containing a MongoDB expression or an aggregation Expr object - type hinting is currently impossible unless we introduce value objects which I wouldn't do on short notice.

Do you have any specific issues with aggregation classes?

caciobanu · 2018-08-07T12:03:06Z

@alcaeus Can you take a look at \Doctrine\ODM\MongoDB\Aggregation\Stage\GraphLookup\Match::getExpression ? I'm not sure that the method body is correct because \Doctrine\ODM\MongoDB\Aggregation\Stage::getExpression says that the return type should be an array.

caciobanu · 2018-08-07T12:08:41Z

I think I've understood why the method body is like that: search for restrictSearchWithMatch https://docs.mongodb.com/manual/reference/operator/aggregation/graphLookup/

caciobanu · 2018-08-07T13:53:17Z

@alcaeus finished adding as much type hints as possible. There are some tests failing but I have no ideea how to fix them.

alcaeus · 2018-08-07T14:59:18Z

Regarding Doctrine\ODM\MongoDB\Aggregation\Stage\GraphLookup\Match::getExpression, its typehint is wrong - it should be @return array|object. The problem is that we need to be specific about arrays vs. objects for the driver. For non-empty values this is relatively easy, but for empty values we need to explicitly convert them to arrays. This might be something that might remain non-strictly typed until we can deal with it properly.

As for tests I can take a look at the two failing tests and why they fail. Step-debugging should prove helpful here to see where they go wrong. Once we've done that and fixed issues found by our static analysers (phpcs and PHPStan) I'll give this another review.

caciobanu · 2018-08-07T15:19:40Z

I've made a minor change to Doctrine\ODM\MongoDB\Aggregation\Stage\GraphLookup\Match::getExpression so we can have only 1 return type: array. Take a look at https://github.com/doctrine/mongodb-odm/pull/1846/files#diff-149e2cae007283dbcfb76c37c91fe524R27 and https://github.com/doctrine/mongodb-odm/pull/1846/files#diff-40007aeae56972528d22a8a3f15acf17R175

caciobanu · 2018-08-07T16:57:06Z

@alcaeus I've managed to track down the issue with the failing tests. As you can see in commit 6bdd9c6 when I changed the typehint for dm->clear() to string I couldn't provide the $user object as before so I provided the class but that made the uow to clear the objects used later in the ->contains checks. Providing an invalid string value mimics the behavior before the type hints where added.

caciobanu · 2018-08-07T16:59:02Z

@alcaeus this is ready for review 👍

alcaeus

Mighty impressive PR, thank you very much. I found some minor issues that need fixing. Also, as mentioned above, I believe it would be worth removing the exclusions in phpcs.xml I linked above to see if there are any other places where we might add some typehints.

alcaeus · 2018-08-07T17:47:27Z

lib/Doctrine/ODM/MongoDB/Aggregation/Stage/GraphLookup.php

    {
+        $restrictSearchWithMatch = $this->restrictSearchWithMatch->getExpression() ?: (object) [];


alcaeus · 2018-08-07T18:13:19Z

lib/Doctrine/ODM/MongoDB/LockException.php

    {
        return $this->document;
    }

-    public static function lockFailed($document)
+    public static function lockFailed($document): self


Shouldn't this have an ?object type for $document?

alcaeus · 2018-08-07T18:35:24Z

lib/Doctrine/ODM/MongoDB/Repository/AbstractRepositoryFactory.php

     * @return ObjectRepository|GridFSRepository
     */
-    protected function createRepository(DocumentManager $documentManager, $documentName)
+    protected function createRepository(DocumentManager $documentManager, string $documentName)


This should also have a common ObjectRepository return type.

alcaeus · 2018-08-07T18:39:10Z

lib/Doctrine/ODM/MongoDB/UnitOfWork.php

     */
-    public function containsId($id, $rootClassName)
+    public function containsId(string $id, string $rootClassName): bool


This may have been documented wrongly in the docblock - most other methods don't specify a type for the ID.

alcaeus · 2018-08-07T18:40:36Z

lib/Doctrine/ODM/MongoDB/UnitOfWork.php

     */
-    public function getClassNameForAssociation(array $mapping, $data)
+    public function getClassNameForAssociation(array $mapping, $data): string


?array $data

Seems like ?array is not enough. object is needed also.

alcaeus · 2018-08-07T18:43:25Z

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

@@ -61,7 +61,7 @@ public function testRemoveEmbedMany()
        $user->profileMany->removeElement($profile1);

        $this->dm->flush();
-        $this->dm->clear($user);
+        $this->dm->clear('invalid');


I know this fixes the test, but if the clear didn't do anything so we might as well drop it instead of passing invalid stuff

caciobanu · 2018-08-07T19:01:26Z

Updated after review & enabled the CS rules you mentioned above.

malarzm · 2018-08-07T19:22:27Z

Yaaay, another few k diff PR :D Thanks a lot @caciobanu for your work on this, I'll review tomorrow while in the office :)

caciobanu · 2018-08-07T19:34:23Z

The activated CS rules run on lib and tests. The output is huge.

Should I run phpcbf and then go from there ?

caciobanu · 2018-08-07T20:29:05Z

Unfortunately phpcs adds invalid parameter type hints on inherited methods from vendor packages, which lack type hints.

alcaeus

I was afraid that phpcs might find more than it should - we'll have to leave the sniffs excluded until we're completely done across the board, and even then might have to exclude some code pieces. Either way, I don't think it's feasible fixing those errors now - at least not at that scale.

My suggestion would be to bring this PR to a close, then focus on interfaces and abstract classes we're introducing here that don't have outside dependencies. Our first focus should be to get those cleaned up. In a second step, we should take care of other classes we're introducing, along with making them final unless we think they should be extended (which most likely, we don't). At the same time, we should also start the process of starting upstream updates in doctrine/persistence and so on, but that should wait until we got rid of doctrine/common.

alcaeus · 2018-08-08T04:59:36Z

lib/Doctrine/ODM/MongoDB/Aggregation/Stage/Bucket/BucketAutoOutput.php

     */
-    public function granularity($granularity)
+    public function granularity(string $granularity): \Doctrine\ODM\MongoDB\Aggregation\Stage\BucketAuto


This can be shortened as with the method above

Yes. Will do.

caciobanu · 2018-08-08T06:36:45Z

Disabled the CS rules for type hinting.

malarzm

Sorry for late and long review, but I really wanted to read this through :)

malarzm · 2018-08-08T18:32:26Z

lib/Doctrine/ODM/MongoDB/Aggregation/Stage.php

     */
-    public function limit($limit)
+    public function limit(int $limit)


Missing return type

Can't added it because of \Doctrine\ODM\MongoDB\Aggregation\Stage\GeoNear::limit which overrides it and has different returned type.

I will add back in the comment @return Stage\Limit

malarzm · 2018-08-08T18:34:34Z

lib/Doctrine/ODM/MongoDB/Aggregation/Stage/GeoNear.php

     * @return $this
     */
-    public function limit($limit)
+    public function limit(int $limit)


Missing return type

See the comment above.

malarzm · 2018-08-08T18:35:15Z

lib/Doctrine/ODM/MongoDB/Aggregation/Stage/GeoNear.php

     * @return $this
     */
-    public function maxDistance($maxDistance)
+    public function maxDistance(float $maxDistance)


Here as well

If I add to this method self I cannot add type hint to \Doctrine\ODM\MongoDB\Aggregation\Stage\Match::maxDistance. If I add to \Doctrine\ODM\MongoDB\Aggregation\Stage\Match::maxDistance then the method above has to return \Doctrine\ODM\MongoDB\Aggregation\Stage\Match

malarzm · 2018-08-08T18:35:24Z

lib/Doctrine/ODM/MongoDB/Aggregation/Stage/GeoNear.php

     * @return $this
     *
     */
-    public function minDistance($minDistance)
+    public function minDistance(float $minDistance)


and here :)

The same as above applies here as well.

I guess I'll need to say it at least once more, but sorry for the noise then :)

malarzm · 2018-08-08T18:37:29Z

lib/Doctrine/ODM/MongoDB/Aggregation/Stage/GraphLookup.php

    {
        return $this->dm->getUnitOfWork()->getDocumentPersister($class->name);
    }

-    private function getReferencedFieldName($fieldName, array $mapping)
+    private function getReferencedFieldName(string $fieldName, array $mapping): ?string


Why is return type nullable? It seems like it's either string or an exception is thrown

malarzm · 2018-08-08T19:51:37Z

lib/Doctrine/ODM/MongoDB/Tools/Console/MetadataFilter.php

     * @param array|string $filter
-     * @return array


Same for @return

malarzm · 2018-08-08T19:52:56Z

lib/Doctrine/ODM/MongoDB/Tools/Console/MetadataFilter.php

@@ -39,10 +37,7 @@ public function __construct(\ArrayIterator $metadata, $filter)
        parent::__construct($metadata);
    }


Can't comment on actual place, but can you set $this->_filter to be string[] and hint in ctor that $filter is string|string[]?

malarzm · 2018-08-08T19:55:10Z

lib/Doctrine/ODM/MongoDB/UnitOfWork.php

@@ -273,11 +273,8 @@ public function __construct(DocumentManager $dm, EventManager $evm, HydratorFact

    /**
     * Factory for returning new PersistenceBuilder instances used for preparing data into
-     * queries for insert persistence.


This shouldn't be removed ;)

malarzm · 2018-08-08T19:59:15Z

lib/Doctrine/ODM/MongoDB/UnitOfWork.php

@@ -2478,11 +2329,8 @@ public function getVisitedCollections($document)
    /**
     * INTERNAL:
     * Gets PersistentCollections that are scheduled to update and related to $document
-     *
-     * @param object $document
-     * @return array


Could you keep this with PersistentCollectionInterface[]?

malarzm · 2018-08-08T20:01:01Z

lib/Doctrine/ODM/MongoDB/UnitOfWork.php

     */
-    public function registerManaged($document, $id, $data)
+    public function registerManaged(object $document, $id, $data): void


Could you add array for $data?

malarzm

Actually even if something is still off a bit, let's get this merged once Travis is green!

caciobanu · 2018-08-09T07:38:16Z

Before merging I can do a rebase to get rid of all those horrid commit messages.

alcaeus · 2018-08-09T08:07:32Z

Yeah, go ahead and give this a rebase - once the build is stable again I'd like to get this merged 🎉

caciobanu · 2018-08-09T08:43:18Z

Done.

alcaeus · 2018-08-09T08:45:15Z

Thanks @caciobanu! 🎖

caciobanu · 2018-08-09T09:13:58Z

Glad to help!

alcaeus added Feature BC Break Needs 1.3 Compat Layer labels Aug 7, 2018

alcaeus added this to the 2.0.0 milestone Aug 7, 2018

alcaeus reviewed Aug 7, 2018

View reviewed changes

alcaeus changed the title ~~[2.0] Add type hints~~ [2.0] [WIP] Add type hints Aug 7, 2018

caciobanu changed the title ~~[2.0] [WIP] Add type hints~~ [2.0] Add type hints Aug 7, 2018

alcaeus self-requested a review August 7, 2018 17:58

alcaeus reviewed Aug 7, 2018

View reviewed changes

alcaeus requested a review from malarzm August 7, 2018 19:03

alcaeus reviewed Aug 8, 2018

View reviewed changes

malarzm requested changes Aug 8, 2018

View reviewed changes

malarzm approved these changes Aug 9, 2018

View reviewed changes

[2.0] Added parameters type hints & return types

724c044

caciobanu force-pushed the add-type-hints branch from c88da82 to 724c044 Compare August 9, 2018 08:13

alcaeus merged commit 5e5e1fc into doctrine:master Aug 9, 2018

caciobanu deleted the add-type-hints branch August 9, 2018 09:34

romainnorberg mentioned this pull request Oct 15, 2019

Support doctrine/mongodb-odm ^2.x & doctrine/mongodb-odm-bundle ^4.x sonata-project/SonataDoctrineMongoDBAdminBundle#321

Merged

		{
		$restrictSearchWithMatch = $this->restrictSearchWithMatch->getExpression() ?: (object) [];

		@@ -39,10 +37,7 @@ public function __construct(\ArrayIterator $metadata, $filter)
		parent::__construct($metadata);
		}

[2.0] Add type hints #1846

[2.0] Add type hints #1846

Conversation

caciobanu commented Aug 6, 2018

caciobanu commented Aug 6, 2018

alcaeus 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

Choose a reason for hiding this comment

caciobanu commented Aug 7, 2018

caciobanu commented Aug 7, 2018

alcaeus commented Aug 7, 2018

caciobanu commented Aug 7, 2018

caciobanu commented Aug 7, 2018

caciobanu commented Aug 7, 2018

alcaeus commented Aug 7, 2018

caciobanu commented Aug 7, 2018

caciobanu commented Aug 7, 2018

caciobanu commented Aug 7, 2018

alcaeus 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

caciobanu commented Aug 7, 2018

malarzm commented Aug 7, 2018

caciobanu commented Aug 7, 2018 • edited Loading

caciobanu commented Aug 7, 2018

alcaeus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caciobanu commented Aug 8, 2018

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

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

malarzm left a comment

Choose a reason for hiding this comment

caciobanu commented Aug 9, 2018

alcaeus commented Aug 9, 2018

caciobanu commented Aug 9, 2018

alcaeus commented Aug 9, 2018

caciobanu commented Aug 9, 2018

caciobanu commented Aug 7, 2018 •

edited

Loading