Raise Psalm level to 3 and add generics to ClassMetadata #159

franmomu · 2021-02-14T16:39:05Z

The first commits of this PR are changes in order to raise psalm level to 3. I've used 2.2.x as base thinking about adding the ClassMetadata generics as feature, but maybe I should split the PR in two targeting 2.1.x with the raising psalm level part.

lib/Doctrine/Persistence/Reflection/TypedNoDefaultReflectionProperty.php

greg0ire · 2021-02-14T19:40:40Z

I don't have a strong opinion about the target branch. Do what you feel is best.

greg0ire · 2021-04-03T13:43:34Z

Please run phpcbf

franmomu · 2021-04-03T19:13:51Z

Please run phpcbf

oops, didn't see that, done.

greg0ire · 2021-04-03T19:51:58Z

@orklah please review

orklah

Great job :)

orklah · 2021-04-05T18:51:46Z

lib/Doctrine/Persistence/Mapping/AbstractClassMetadataFactory.php

@@ -218,6 +221,7 @@ public function getMetadataFor($className)

            $realClassName = $this->getFqcnFromAlias($namespaceAlias, $simpleClassName);
        } else {
+            /** @psalm-var class-string $className */


This should be specified in parameters instead of overrided with @var

Thank you for reviewing! I remember struggling with this one:

// Check for namespace alias if (strpos($className, ':') !== false) { [$namespaceAlias, $simpleClassName] = explode(':', $className, 2); $realClassName = $this->getFqcnFromAlias($namespaceAlias, $simpleClassName); } else { /** @psalm-var class-string $className */ $realClassName = $this->getRealClass($className); }

$className can be a class-string or a class name string alias (App:Entity), I tried to move it to parameters as class-string|string but then psalm doesn't know that which one is receiving.

nevermind then, I thought $className would always be a className. How naive :D

maybe rename $className to $classNameOrClassNameAlias?
This will ease reading the code then

orklah · 2021-04-05T18:53:42Z

lib/Doctrine/Persistence/Mapping/AbstractClassMetadataFactory.php

@@ -483,6 +491,8 @@ private function getRealClass(string $class): string
            $this->createDefaultProxyClassNameResolver();
        }

+        assert($this->proxyClassNameResolver instanceof ProxyClassNameResolver);


asserting the variable is !== null would be semantically more meaningful I think

orklah · 2021-04-05T18:54:46Z

lib/Doctrine/Persistence/Mapping/Driver/AnnotationDriver.php

@@ -66,7 +66,7 @@ abstract class AnnotationDriver implements MappingDriver
    /**
     * Name of the entity annotations as keys.
     *
-     * @var string[]
+     * @var array<class-string, mixed>


was string[] plain wrong? If not, it should become array<class-string, string>

Looks like it was wrong, I used mixed because looking at its use:

doctrine/orm the values are int: https://github.com/doctrine/orm/blob/f8fa0fe0694ee661bbcc18406475fdb1749b0be1/lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php#L52-L55

doctrine/couchdb-odm are bool : https://github.com/doctrine/couchdb-odm/blob/9bf3d8ed9a649d91b6a66aa439b05a8a1f1ef7fd/lib/Doctrine/ODM/CouchDB/Mapping/Driver/AnnotationDriver.php#L24-L28

And here in tests we're using bool, I'll change it to array<class-string, int|bool>.

orklah · 2021-04-05T18:56:00Z

lib/Doctrine/Persistence/Mapping/ReflectionService.php

@@ -21,6 +21,9 @@ interface ReflectionService
     * @return string[]
     *
     * @throws MappingException
+     *
+     * @psalm-param class-string $class
+     * @psalm-return class-string[]


maybe use the extensive syntax if offset type is known?

The only implementation I've seen (apart from the ones returning empty array) is

persistence/lib/Doctrine/Persistence/Mapping/RuntimeReflectionService.php

Lines 34 to 41 in d8de172

public function getParentClasses($class)

{

if (! class_exists($class)) {

throw MappingException::nonExistingClass($class);

}

return class_parents($class);

}

So I don't know if that can be improved.

orklah · 2021-04-05T18:57:06Z

psalm.xml

@@ -39,5 +39,21 @@
                <file name="tests/Doctrine/Tests/Persistence/Mapping/SymfonyFileLocatorTest.php"/>
            </errorLevel>
        </NullArgument>
+        <PossiblyNullReference>


Maybe leave a comment to explain why those couldn't be fixed and had to be ignored? It will be helpful for future contributions

orklah · 2021-04-05T18:57:28Z

tests/Doctrine/Tests/Persistence/Mapping/AnnotationDriverTest.php

@@ -24,7 +24,7 @@ public function testGetAllClassNames(): void

 class SimpleAnnotationDriver extends AnnotationDriver
 {
-    /** @var bool[] */
+    /** @var array<class-string, mixed> */


should probably be array<class-string, bool>

orklah · 2021-04-05T18:58:29Z

tests/Doctrine/Tests/Persistence/Mapping/DriverChainTest.php

+        $companyDriver = $this->createMock(MappingDriver::class);
+        $defaultDriver = $this->createMock(MappingDriver::class);
+        /** @psalm-var class-string */
+        $entityClassName = 'Doctrine\Tests\ORM\Mapping\DriverChainEntity';


I think ::class should be used too (and eventually suppressed because the dependancy is not there) instead of being overrided

orklah · 2021-04-05T18:59:46Z

tests/Doctrine/Tests/Persistence/Mapping/TestClassMetadataFactory.php

@@ -91,6 +92,8 @@ protected function onNotFoundMetadata($className)

    /**
     * {@inheritDoc}
+     *
+     * @param string $class


I think I remember @inheritdoc have weird behaviour when a part is overrided. Maybe ditch it completely?

orklah · 2021-04-05T19:00:41Z

tests/Doctrine/Tests/Persistence/ObjectManagerDecoratorTest.php

@@ -22,7 +22,7 @@ public function __construct(ObjectManager $wrapped)

 class ObjectManagerDecoratorTest extends TestCase
 {
-    /** @var MockObject|ObjectManager */
+    /** @var MockObject&ObjectManager */


Not sure how IDE will react to that. Maybe in an @psalm-var?

Interesting, I've been using it for a while because PHPStorm understands it, don't know others.

IIRC, PHPStorm basically treat that the same way as "|" now. There was an issue on their bug tracker where they talked about that and they were not sure of what to do with that

orklah · 2021-04-05T19:02:43Z

lib/Doctrine/Persistence/AbstractManagerRegistry.php

+        }
+
+        /** @psalm-var class-string */
+        return $className;


Maybe an assertion for class_exists could be used in the future instead of overriding? In a future version to avoid BC breaks maybe?

Some method can receive a string which could be a class-string or an alias of a class-string.

orklah · 2021-04-06T07:41:24Z

tests/Doctrine/Tests/Persistence/Mapping/DriverChainTest.php

@@ -118,7 +118,7 @@ public function testDefaultDriver(): void
        self::assertNull($chain->getDefaultDriver());

        $chain->setDefaultDriver($defaultDriver);
-        $chain->addDriver($companyDriver, 'Doctrine\Tests\Models\Company');
+        $chain->addDriver($companyDriver, 'Doctrine\Tests\Persistence\Mapping\Fixtures\Manager');


This should be a ::class instead

In this case, this argument is a namespace.

orklah · 2021-04-06T07:43:05Z

psalm.xml

@@ -41,17 +41,21 @@
        </NullArgument>
        <PossiblyNullReference>
            <errorLevel type="suppress">
+                <!-- ReflectionProperty::getType() can return null, but there is ReflectionProperty::hasType()


This has been fixed in Psalm recently:
vimeo/psalm#5258
Maybe Psalm can be upgraded?

Here is ReflectionProperty (instead of ReflectionParameter) I've created vimeo/psalm#5584 based on vimeo/psalm#5258, but after reading it again, IIUC, it won't work because ReflectionProperty is not immutable.

Oh, my mistake then :)

greg0ire · 2021-04-24T08:03:40Z

This can be closed, right?

greg0ire reviewed Feb 14, 2021

View reviewed changes

lib/Doctrine/Persistence/Reflection/TypedNoDefaultReflectionProperty.php Outdated Show resolved Hide resolved

franmomu force-pushed the metadata_generics branch from 40f5e50 to 3a240d7 Compare February 14, 2021 22:42

greg0ire approved these changes Feb 15, 2021

View reviewed changes

franmomu force-pushed the metadata_generics branch from 3a240d7 to a3fc681 Compare April 2, 2021 16:58

franmomu force-pushed the metadata_generics branch from 1da1086 to 0489cb2 Compare April 3, 2021 19:12

greg0ire added the Improvement label Apr 3, 2021

franmomu force-pushed the metadata_generics branch from 0489cb2 to d892fda Compare April 5, 2021 13:20

orklah reviewed Apr 5, 2021

View reviewed changes

franmomu force-pushed the metadata_generics branch 3 times, most recently from 9e8232b to 6b1745b Compare April 6, 2021 07:13

franmomu added 13 commits April 6, 2021 09:17

Use class string arguments

9a56cc0

Use class string annotation

16e8615

Fix possible false argument

01707f5

Fix some phpdoc

5a5e6a3

Extract method to get class string variable

d8c5f79

Raise psalm level to 3

6b24d36

Add generics to ClassMetadata

4eab605

Fix psalm issues

5893ad2

Use a more specific value type

54d344b

Explain psalm ignored errors

8e6fc2f

Make explicit param types

a067fdf

Some method can receive a string which could be a class-string or an alias of a class-string.

Use real classes for tests

07ed4b6

Use ::class instead of string

df82d13

franmomu force-pushed the metadata_generics branch from 6b1745b to df82d13 Compare April 6, 2021 07:26

orklah reviewed Apr 6, 2021

View reviewed changes

This was referenced Apr 20, 2021

Use class-string for parameters #168

Merged

Use psalm errorLevel 3 #169

Merged

Make Classmetadata generic #171

Merged

greg0ire closed this Apr 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Raise Psalm level to 3 and add generics to ClassMetadata #159

Raise Psalm level to 3 and add generics to ClassMetadata #159

franmomu commented Feb 14, 2021

greg0ire commented Feb 14, 2021

greg0ire commented Apr 3, 2021

franmomu commented Apr 3, 2021

greg0ire commented Apr 3, 2021

orklah left a comment

orklah Apr 5, 2021

franmomu Apr 6, 2021

orklah Apr 6, 2021

vv12131415 Apr 11, 2021

orklah Apr 5, 2021

orklah Apr 5, 2021

franmomu Apr 6, 2021

orklah Apr 5, 2021

franmomu Apr 6, 2021

orklah Apr 5, 2021

orklah Apr 5, 2021

orklah Apr 5, 2021

orklah Apr 5, 2021

orklah Apr 5, 2021

franmomu Apr 6, 2021

orklah Apr 6, 2021

orklah Apr 5, 2021

orklah Apr 6, 2021

franmomu Apr 6, 2021

orklah Apr 6, 2021

franmomu Apr 6, 2021

orklah Apr 6, 2021

greg0ire commented Apr 24, 2021

	public function getParentClasses($class)
	{
	if (! class_exists($class)) {
	throw MappingException::nonExistingClass($class);
	}

	return class_parents($class);
	}

Raise Psalm level to 3 and add generics to ClassMetadata #159

Raise Psalm level to 3 and add generics to ClassMetadata #159

Conversation

franmomu commented Feb 14, 2021

greg0ire commented Feb 14, 2021

greg0ire commented Apr 3, 2021

franmomu commented Apr 3, 2021

greg0ire commented Apr 3, 2021

orklah 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

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

greg0ire commented Apr 24, 2021