Update PHPStan #2231

franmomu · 2020-10-18T19:37:57Z

Q	A
Type	improvement
BC Break	no
Fixed issues

Summary

The PHPStan version was almost 2 year old. With the new version I had to lower the level to 5 because more than 900 errors appear in level 7.

I didn't know how to solve some of the errors so I ignored them, I'll be happy to solve them and remove them from the ignore list.

PR with the Github Action: franmomu#5

franmomu · 2020-10-18T19:41:01Z

lib/Doctrine/ODM/MongoDB/Query/ReferencePrimer.php

@@ -232,6 +226,8 @@ private function parseDotSyntaxForPrimer(string $fieldName, ClassMetadata $class

            return ['fieldName' => $fieldName, 'class' => $class, 'documents' => $documents, 'mapping' => $mapping];
        }
+
+        return ['fieldName' => $fieldName, 'class' => $class, 'documents' => $documents, 'mapping' => $mapping];


I have not enough knowledge about this, I've just added the same line than

mongodb-odm/lib/Doctrine/ODM/MongoDB/Query/ReferencePrimer.php

Line 171 in 6e37360

return ['fieldName' => $fieldName, 'class' => $class, 'documents' => $documents, 'mapping' => $mapping];

. But since I guess this line is never reached, maybe an Exception would be better.

You're right, an exception would be better if this should never be reached. This should read just fine:

Suggested change

return ['fieldName' => $fieldName, 'class' => $class, 'documents' => $documents, 'mapping' => $mapping];

throw new \LogicException('Unable to parse property path for ReferencePrimer. Please report an issue in Doctrine\'s MongoDB ODM.');

malarzm · 2020-10-24T11:55:53Z

@franmomu would it be possible for you to split this PR into two? First adding PHPStan (version currently used by us) as a GA and a second PR updating PHPStan, levels, and code to fix new faults? I'd definitely would like to merge PHPStan as a GA faster and solve new issues later :)

malarzm · 2020-11-03T21:15:11Z

All righty with #2236 merged we can now move this one forward. Please ping me when the branch is rebased, we'll be able to see results in GA now thanks to you :)

franmomu · 2020-11-04T07:39:19Z

hi @malarzm, can you please take a look whenever you can? I've also saw that PHPStan was not totally happy, I've created #2240 in the meanwhile if you want to have it green.

An exception is thrown in the line above.

malarzm

Left few remarks. Thanks @franmomu for all the work you do for ODM!

malarzm · 2020-11-04T19:28:43Z

lib/Doctrine/ODM/MongoDB/DocumentManager.php

@@ -310,8 +313,7 @@ public function getDocumentDatabase(string $className) : Database
            return $this->documentDatabases[$className];
        }

-        $metadata = $this->metadataFactory->getMetadataFor($className);
-        assert($metadata instanceof ClassMetadata);
+        $metadata = $this->getClassMetadata($className);


Not using getClassMetadata was an intentional micro-optimization to avoid an additional method call. I'm not sure it is still relevant in 2020 but better safe than sorry and I'd prefer keeping it

lib/Doctrine/ODM/MongoDB/PersistentCollection/PersistentCollectionTrait.php

malarzm · 2020-11-04T19:40:49Z

lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php

@@ -222,14 +222,12 @@ public function executeInserts(array $options = []) : void
            $inserts[] = $data;
        }

-        if ($inserts) {


Out of curiosity: PHPStan detected this if as redundant as its value is basing on queuedInserts which is already checked at the beginning?

Looks like it! https://phpstan.org/r/07684e38-4e79-4aa7-b6ba-500c27da21c3

malarzm · 2020-11-04T19:48:01Z

lib/Doctrine/ODM/MongoDB/Query/ReferencePrimer.php

@@ -232,6 +226,8 @@ private function parseDotSyntaxForPrimer(string $fieldName, ClassMetadata $class

            return ['fieldName' => $fieldName, 'class' => $class, 'documents' => $documents, 'mapping' => $mapping];
        }
+
+        return ['fieldName' => $fieldName, 'class' => $class, 'documents' => $documents, 'mapping' => $mapping];


You're right, an exception would be better if this should never be reached. This should read just fine:

Suggested change

return ['fieldName' => $fieldName, 'class' => $class, 'documents' => $documents, 'mapping' => $mapping];

throw new \LogicException('Unable to parse property path for ReferencePrimer. Please report an issue in Doctrine\'s MongoDB ODM.');

The current return types did not comply with PersistentCollectionInterface.

The phpdoc did not match the current return and arguments types.

The check removed was impossible to be true since the argument has a type declaration.

The ReferencePrimer::parseDotSyntaxForPrimer had a case which there was no return and the function has to return an array or Traversable.

At the beginning of the method, "$this->queuedInserts" is checked if it is empty, if it is not, "$inserts" variable will always have some value and the "if" removed is always true.

malarzm

Thanks for the fixes!

franmomu · 2020-11-04T21:23:36Z

You've very welcome @malarzm! Happy to help

franmomu commented Oct 18, 2020

View reviewed changes

franmomu mentioned this pull request Oct 27, 2020

Migrate PHPStan job to GA #2236

Merged

franmomu force-pushed the update_phpstan branch from 6e37360 to 3e6c5e1 Compare November 4, 2020 02:26

franmomu changed the title ~~Move PHPStan check to Github actions~~ Update PHPStan Nov 4, 2020

franmomu force-pushed the update_phpstan branch from 3e6c5e1 to a2de76e Compare November 4, 2020 02:28

Remove unreachable code

dda477a

An exception is thrown in the line above.

franmomu force-pushed the update_phpstan branch from a2de76e to 165cb9e Compare November 4, 2020 08:08

malarzm reviewed Nov 4, 2020

View reviewed changes

franmomu added 6 commits November 4, 2020 21:59

Fix return types in PersistentCollectionInterface

99e2786

The current return types did not comply with PersistentCollectionInterface.

Fix PHPDoc in DocumentPersister

201517e

The phpdoc did not match the current return and arguments types.

Add assertions for ClassMetadata

1002737

Remove unreachable code

39e88ca

The check removed was impossible to be true since the argument has a type declaration.

Add a missing return

5c64e38

The ReferencePrimer::parseDotSyntaxForPrimer had a case which there was no return and the function has to return an array or Traversable.

Remove always true condition

e718fcc

At the beginning of the method, "$this->queuedInserts" is checked if it is empty, if it is not, "$inserts" variable will always have some value and the "if" removed is always true.

franmomu force-pushed the update_phpstan branch from 165cb9e to e718fcc Compare November 4, 2020 21:10

malarzm approved these changes Nov 4, 2020

View reviewed changes

malarzm added the Task label Nov 4, 2020

malarzm added this to the 2.1.3 milestone Nov 4, 2020

malarzm merged commit 519403e into doctrine:2.1.x Nov 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update PHPStan #2231

Update PHPStan #2231

franmomu commented Oct 18, 2020

franmomu Oct 18, 2020 •

edited

Loading

malarzm Nov 4, 2020

malarzm commented Oct 24, 2020

malarzm commented Nov 3, 2020

franmomu commented Nov 4, 2020

malarzm left a comment

malarzm Nov 4, 2020

malarzm Nov 4, 2020

franmomu Nov 4, 2020

malarzm Nov 4, 2020

malarzm Nov 4, 2020

malarzm left a comment

franmomu commented Nov 4, 2020

	return ['fieldName' => $fieldName, 'class' => $class, 'documents' => $documents, 'mapping' => $mapping];
	throw new \LogicException('Unable to parse property path for ReferencePrimer. Please report an issue in Doctrine\'s MongoDB ODM.');

Update PHPStan #2231

Update PHPStan #2231

Conversation

franmomu commented Oct 18, 2020

Summary

franmomu Oct 18, 2020 • edited Loading

Choose a reason for hiding this comment

malarzm Nov 4, 2020

Choose a reason for hiding this comment

malarzm commented Oct 24, 2020

malarzm commented Nov 3, 2020

franmomu commented Nov 4, 2020

malarzm left a comment

Choose a reason for hiding this comment

malarzm Nov 4, 2020

Choose a reason for hiding this comment

malarzm Nov 4, 2020

Choose a reason for hiding this comment

franmomu Nov 4, 2020

Choose a reason for hiding this comment

malarzm Nov 4, 2020

Choose a reason for hiding this comment

malarzm Nov 4, 2020

Choose a reason for hiding this comment

malarzm left a comment

Choose a reason for hiding this comment

franmomu commented Nov 4, 2020

franmomu Oct 18, 2020 •

edited

Loading