Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 13 commits into from
38 changes: 29 additions & 9 deletions lib/Doctrine/Persistence/AbstractManagerRegistry.php
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,10 @@ abstract class AbstractManagerRegistry implements ManagerRegistry
/** @var string */
private $defaultManager;

/** @var string */
/**
* @var string
* @psalm-var class-string
*/
private $proxyInterfaceName;

/**
Expand All @@ -39,6 +42,8 @@ abstract class AbstractManagerRegistry implements ManagerRegistry
* @param string $defaultConnection
* @param string $defaultManager
* @param string $proxyInterfaceName
*
* @psalm-param class-string $proxyInterfaceName
*/
public function __construct($name, array $connections, array $managers, $defaultConnection, $defaultManager, $proxyInterfaceName)
{
Expand Down Expand Up @@ -158,13 +163,9 @@ public function getManager($name = null)
*/
public function getManagerForClass($class)
{
// Check for namespace alias
if (strpos($class, ':') !== false) {
[$namespaceAlias, $simpleClassName] = explode(':', $class, 2);
$class = $this->getAliasNamespace($namespaceAlias) . '\\' . $simpleClassName;
}
$className = $this->getRealClassName($class);

$proxyClass = new ReflectionClass($class);
$proxyClass = new ReflectionClass($className);

if ($proxyClass->implementsInterface($this->proxyInterfaceName)) {
$parentClass = $proxyClass->getParentClass();
Expand All @@ -173,20 +174,39 @@ public function getManagerForClass($class)
return null;
}

$class = $parentClass->getName();
$className = $parentClass->getName();
}

foreach ($this->managers as $id) {
$manager = $this->getService($id);

if (! $manager->getMetadataFactory()->isTransient($class)) {
if (! $manager->getMetadataFactory()->isTransient($className)) {
return $manager;
}
}

return null;
}

/**
* @param string $className
*
* @psalm-return class-string
*/
private function getRealClassName($className): string
{
// Check for namespace alias
if (strpos($className, ':') !== false) {
[$namespaceAlias, $simpleClassName] = explode(':', $className, 2);

/** @psalm-var class-string */
return $this->getAliasNamespace($namespaceAlias) . '\\' . $simpleClassName;
}

/** @psalm-var class-string */
return $className;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

/**
* {@inheritdoc}
*/
Expand Down
10 changes: 10 additions & 0 deletions lib/Doctrine/Persistence/Mapping/AbstractClassMetadataFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use function array_map;
use function array_reverse;
use function array_unshift;
use function assert;
use function explode;
use function sprintf;
use function str_replace;
Expand Down Expand Up @@ -163,6 +164,8 @@ abstract protected function initialize();
* @param string $simpleClassName
*
* @return string
*
* @psalm-return class-string
*/
abstract protected function getFqcnFromAlias($namespaceAlias, $simpleClassName);

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

$realClassName = $this->getRealClass($className);
}

Expand Down Expand Up @@ -307,6 +311,8 @@ public function setMetadataFor($className, $class)
* @param string $name
*
* @return string[]
*
* @psalm-param class-string $name
*/
protected function getParentClasses($name)
{
Expand Down Expand Up @@ -337,6 +343,8 @@ protected function getParentClasses($name)
* @param string $name The name of the class for which the metadata should get loaded.
*
* @return string[]
*
* @psalm-param class-string $name
*/
protected function loadMetadata($name)
{
Expand Down Expand Up @@ -483,6 +491,8 @@ private function getRealClass(string $class): string
$this->createDefaultProxyClassNameResolver();
}

assert($this->proxyClassNameResolver !== null);

return $this->proxyClassNameResolver->resolveClassName($class);
}

Expand Down
6 changes: 5 additions & 1 deletion lib/Doctrine/Persistence/Mapping/ClassMetadata.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,17 @@

/**
* Contract for a Doctrine persistence layer ClassMetadata class to implement.
*
* @template T of object
*/
interface ClassMetadata
{
/**
* Gets the fully-qualified class name of this persistent class.
*
* @return string
*
* @psalm-return class-string<T>
*/
public function getName();

Expand All @@ -28,7 +32,7 @@ public function getIdentifier();
/**
* Gets the ReflectionClass instance for this mapped class.
*
* @return ReflectionClass
* @return ReflectionClass<T>
*/
public function getReflectionClass();

Expand Down
2 changes: 2 additions & 0 deletions lib/Doctrine/Persistence/Mapping/ClassMetadataFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ public function setMetadataFor($className, $class);
* @param string $className
*
* @return bool
*
* @psalm-param class-string $className
*/
public function isTransient($className);
}
6 changes: 2 additions & 4 deletions lib/Doctrine/Persistence/Mapping/Driver/AnnotationDriver.php
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ abstract class AnnotationDriver implements MappingDriver
/**
* Name of the entity annotations as keys.
*
* @var string[]
* @var array<class-string, bool|int>
*/
protected $entityAnnotationClasses = [];

Expand Down Expand Up @@ -170,9 +170,7 @@ public function setFileExtension($fileExtension)
* A class is non-transient if it is annotated with an annotation
* from the {@see AnnotationDriver::entityAnnotationClasses}.
*
* @param string $className
*
* @return bool
* {@inheritDoc}
*/
public function isTransient($className)
{
Expand Down
2 changes: 2 additions & 0 deletions lib/Doctrine/Persistence/Mapping/Driver/MappingDriver.php
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ public function getAllClassNames();
* @param string $className
*
* @return bool
*
* @psalm-param class-string $className
*/
public function isTransient($className);
}
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,6 @@ public function findMappingFile($className)
}
}

throw MappingException::mappingFileNotFound($className, substr($className, strrpos($className, '\\') + 1) . $this->fileExtension);
throw MappingException::mappingFileNotFound($className, substr($className, (int) strrpos($className, '\\') + 1) . $this->fileExtension);
}
}
4 changes: 2 additions & 2 deletions lib/Doctrine/Persistence/Mapping/MappingException.php
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ public static function fileMappingDriversRequireConfiguredDirectoryPath($path =

return new self(sprintf(
'File mapping drivers must have a valid directory path, ' .
'however the given path %s seems to be incorrect!',
$path
'however the given path "%s" seems to be incorrect!',
(string) $path
));
}

Expand Down
13 changes: 13 additions & 0 deletions lib/Doctrine/Persistence/Mapping/ReflectionService.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ interface ReflectionService
* @return string[]
*
* @throws MappingException
*
* @psalm-param class-string $class
* @psalm-return class-string[]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe use the extensive syntax if offset type is known?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.

*/
public function getParentClasses($class);

Expand All @@ -30,13 +33,17 @@ public function getParentClasses($class);
* @param string $class
*
* @return string
*
* @psalm-param class-string $class
*/
public function getClassShortName($class);

/**
* @param string $class
*
* @return string
*
* @psalm-param class-string $class
*/
public function getClassNamespace($class);

Expand All @@ -46,6 +53,8 @@ public function getClassNamespace($class);
* @param string $class
*
* @return ReflectionClass|null
*
* @psalm-param class-string $class
*/
public function getClass($class);

Expand All @@ -56,6 +65,8 @@ public function getClass($class);
* @param string $property
*
* @return ReflectionProperty|null
*
* @psalm-param class-string $class
*/
public function getAccessibleProperty($class, $property);

Expand All @@ -66,6 +77,8 @@ public function getAccessibleProperty($class, $property);
* @param mixed $method
*
* @return bool
*
* @psalm-param class-string $class
*/
public function hasPublicMethod($class, $method);
}
2 changes: 2 additions & 0 deletions lib/Doctrine/Persistence/Mapping/RuntimeReflectionService.php
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ public function getClassNamespace($class)

/**
* {@inheritDoc}
*
* @return ReflectionClass
*/
public function getClass($class)
{
Expand Down
8 changes: 5 additions & 3 deletions lib/Doctrine/Persistence/Mapping/StaticReflectionService.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,10 @@ public function getParentClasses($class)
*/
public function getClassShortName($className)
{
if (strpos($className, '\\') !== false) {
$className = substr($className, strrpos($className, '\\') + 1);
$nsSeparatorPosition = strrpos($className, '\\');

if ($nsSeparatorPosition !== false) {
$className = substr($className, $nsSeparatorPosition + 1);
}

return $className;
Expand All @@ -39,7 +41,7 @@ public function getClassNamespace($className)
{
$namespace = '';
if (strpos($className, '\\') !== false) {
$namespace = strrev(substr(strrev($className), strpos(strrev($className), '\\') + 1));
$namespace = strrev(substr(strrev($className), (int) strpos(strrev($className), '\\') + 1));
}

return $namespace;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,11 @@

namespace Doctrine\Persistence\Reflection;

use Closure;
use ReflectionProperty;

use function assert;

/**
* PHP Typed No Default Reflection Property - special override for typed properties without a default value.
*/
Expand Down Expand Up @@ -38,6 +41,9 @@ public function setValue($object, $value = null)
unset($this->$propertyName);
};
$unsetter = $unsetter->bindTo($object, $this->getDeclaringClass()->getName());

assert($unsetter instanceof Closure);

$unsetter();

return;
Expand Down
5 changes: 5 additions & 0 deletions phpstan-baseline.neon
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,8 @@ parameters:
message: "#^Method Doctrine\\\\Persistence\\\\Mapping\\\\AbstractClassMetadataFactory\\:\\:getRealClass\\(\\) should return class\\-string\\<T of object\\> but returns class\\-string\\<Doctrine\\\\Persistence\\\\Proxy\\<T of object\\>\\|T of object\\>\\.$#"
count: 1
path: lib/Doctrine/Persistence/Mapping/AbstractClassMetadataFactory.php

-
message: "#^Parameter \\#1 \\$class of method Doctrine\\\\Persistence\\\\Mapping\\\\RuntimeReflectionService\\:\\:getParentClasses\\(\\) expects class-string, string given.$#"
count: 1
path: tests/Doctrine/Tests/Persistence/Mapping/RuntimeReflectionServiceTest.php
22 changes: 21 additions & 1 deletion psalm.xml
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<?xml version="1.0"?>
<psalm
totallyTyped="false"
errorLevel="4"
errorLevel="3"
resolveFromConfigFile="true"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xmlns="https://getpsalm.org/schema/config"
Expand Down Expand Up @@ -39,5 +39,25 @@
<file name="tests/Doctrine/Tests/Persistence/Mapping/SymfonyFileLocatorTest.php"/>
</errorLevel>
</NullArgument>
<PossiblyNullReference>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<errorLevel type="suppress">
<!-- ReflectionProperty::getType() can return null, but there is ReflectionProperty::hasType()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, my mistake then :)

check before -->
<file name="lib/Doctrine/Persistence/Reflection/TypedNoDefaultReflectionProperty.php"/>
</errorLevel>
</PossiblyNullReference>
<ArgumentTypeCoercion>
<errorLevel type="suppress">
<!-- On purpose to use a non existing class for tests -->
<referencedFunction name="RuntimeReflectionServiceTest::testGetParentClassesForAbsentClass"/>
<file name="tests/Doctrine/Tests/Persistence/Mapping/RuntimeReflectionServiceTest.php"/>
</errorLevel>
</ArgumentTypeCoercion>
<MoreSpecificReturnType>
<errorLevel type="suppress">
<!-- FileDriver::loadMappingFile() in tests could have a more specific return, but is not needed -->
<file name="tests/Doctrine/Tests/Persistence/Mapping/FileDriverTest.php"/>
</errorLevel>
</MoreSpecificReturnType>
</issueHandlers>
</psalm>
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public function testGetAllClassNames(): void

class SimpleAnnotationDriver extends AnnotationDriver
{
/** @var bool[] */
/** @var array<class-string, bool|int> */
protected $entityAnnotationClasses = [Entity::class => true];

/**
Expand Down
Loading