Skip to content

ReflectionDescriptor: deduce database internal type based on parent #582

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 25, 2024
Merged

ReflectionDescriptor: deduce database internal type based on parent #582

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
af955bb
ReflectionDescriptor: deduce database internal type based on parent
janedbal Jun 25, 2024
ed7f604
Guard by hasClass
janedbal Jun 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,10 @@ Type descriptors don't have to deal with nullable types, as these are transparen

If your custom type's `convertToPHPValue()` and `convertToDatabaseValue()` methods have proper typehints, you don't have to write your own descriptor for it. The `PHPStan\Type\Doctrine\Descriptors\ReflectionDescriptor` can analyse the typehints and do the rest for you.

If parent of your type is one of the Doctrine's non-abstract ones, `ReflectionDescriptor` will reuse its descriptor even for expression resolution (e.g. `AVG(t.cost)`).
For example, if you extend `Doctrine\DBAL\Types\DecimalType`, it will know that sqlite fetches that as `float|int` and other drivers as `numeric-string`.
If you extend only `Doctrine\DBAL\Types\Type`, you should use custom descriptor and optionally implement even `DoctrineTypeDriverAwareDescriptor` to provide driver-specific resolution.

### Registering type descriptors

When you write a custom type descriptor, you have to let PHPStan know about it. Add something like this into your `phpstan.neon`:
Expand Down
11 changes: 11 additions & 0 deletions src/Type/Doctrine/DefaultDescriptorRegistry.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,15 @@ public function get(string $type): DoctrineTypeDescriptor
return $this->descriptors[$typeClass];
}

/**
* @throws DescriptorNotRegisteredException
*/
public function getByClassName(string $className): DoctrineTypeDescriptor
{
if (!isset($this->descriptors[$className])) {
throw new DescriptorNotRegisteredException();
}
return $this->descriptors[$className];
}

}
32 changes: 29 additions & 3 deletions src/Type/Doctrine/Descriptors/ReflectionDescriptor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,12 @@
namespace PHPStan\Type\Doctrine\Descriptors;

use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Types\Type as DbalType;
use PHPStan\DependencyInjection\Container;
use PHPStan\Reflection\ParametersAcceptorSelector;
use PHPStan\Reflection\ReflectionProvider;
use PHPStan\Type\Doctrine\DefaultDescriptorRegistry;
use PHPStan\Type\Doctrine\DescriptorNotRegisteredException;
use PHPStan\Type\MixedType;
use PHPStan\Type\ObjectType;
use PHPStan\Type\Type;
Expand All @@ -13,19 +17,27 @@
class ReflectionDescriptor implements DoctrineTypeDescriptor
{

/** @var class-string<\Doctrine\DBAL\Types\Type> */
/** @var class-string<DbalType> */
private $type;

/** @var ReflectionProvider */
private $reflectionProvider;

/** @var Container */
private $container;

/**
* @param class-string<\Doctrine\DBAL\Types\Type> $type
* @param class-string<DbalType> $type
*/
public function __construct(string $type, ReflectionProvider $reflectionProvider)
public function __construct(
string $type,
ReflectionProvider $reflectionProvider,
Container $container
)
{
$this->type = $type;
$this->reflectionProvider = $reflectionProvider;
$this->container = $container;
}

public function getType(): string
Expand Down Expand Up @@ -57,6 +69,20 @@ public function getWritableToDatabaseType(): Type

public function getDatabaseInternalType(): Type
{
$registry = $this->container->getByType(DefaultDescriptorRegistry::class);
$parents = $this->reflectionProvider->getClass($this->type)->getParentClassesNames();

foreach ($parents as $dbalTypeParentClass) {
try {
// this assumes that if somebody inherits from DecimalType,
// the real database type remains decimal and we can reuse its descriptor
return $registry->getByClassName($dbalTypeParentClass)->getDatabaseInternalType();

} catch (DescriptorNotRegisteredException $e) {
continue;
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also have another idea how to implement this without the requirement of known parent. Then, you can extend even abstract Doctrine\DBAL\Types\Type and still have proper type (which is not working in the implementation here).

We use it our codebase, but it may feel a bit more hacky:

public function getDatabaseInternalType(): Type {
        /** @var DbalType $dbalType */
        $dbalType = new $this->type();
        $schemaColumn = new SchemaColumn('dummy', $dbalType, []);
        $fullSqlType = strtolower($dbalType->getSQLDeclaration($schemaColumn->toArray(), new MySQL80Platform()));

        $typeName = Strings::replace($fullSqlType, '~^([a-z ]+).*?$~', '$1');

        return match ($typeName) {
            'tinyint',
            'smallint',
            'mediumint',
            'int',
            'bigint' => new IntegerType(),
            'decimal',
            'numeric' => new IntersectionType([new StringType(), new AccessoryNumericStringType()]),
            'double precision' => new FloatType(),
            'varchar',
            'char',
            'binary',
            'tinytext',
            'text',
            'longtext',
            'json',
            'date',
            'datetime',
            'time' => new StringType(),
            default => throw new LogicException("Unexpected type: $fullSqlType"),
        };
    }

If you feel that this more generic approach is suitable even for phpstan-doctrine, I can try implementing it in #506 (as ReflectionDescriptor actually need to implement DoctrineTypeDriverAwareDescriptor).

Copy link
Member

Choose a reason for hiding this comment

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

What about combining both, to cover more use-cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If both, I think we can merge this and add that extra layer using getSQLDeclaration on top of this later.


return new MixedType();
}

Expand Down
8 changes: 4 additions & 4 deletions tests/Rules/Doctrine/ORM/EntityColumnRuleTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,10 @@ protected function getRule(): Rule
new StringType(),
new SimpleArrayType(),
new UuidTypeDescriptor(FakeTestingUuidType::class),
new ReflectionDescriptor(CarbonImmutableType::class, $this->createBroker()),
new ReflectionDescriptor(CarbonType::class, $this->createBroker()),
new ReflectionDescriptor(CustomType::class, $this->createBroker()),
new ReflectionDescriptor(CustomNumericType::class, $this->createBroker()),
new ReflectionDescriptor(CarbonImmutableType::class, $this->createBroker(), self::getContainer()),
new ReflectionDescriptor(CarbonType::class, $this->createBroker(), self::getContainer()),
new ReflectionDescriptor(CustomType::class, $this->createBroker(), self::getContainer()),
new ReflectionDescriptor(CustomNumericType::class, $this->createBroker(), self::getContainer()),
]),
$this->createReflectionProvider(),
true,
Expand Down