-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Adds metadata field type validation against Entity property type #10946
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,33 +4,73 @@ | |
|
||
namespace Doctrine\ORM\Tools; | ||
|
||
use Doctrine\DBAL\Types\AsciiStringType; | ||
use Doctrine\DBAL\Types\BigIntType; | ||
use Doctrine\DBAL\Types\BooleanType; | ||
use Doctrine\DBAL\Types\DecimalType; | ||
use Doctrine\DBAL\Types\FloatType; | ||
use Doctrine\DBAL\Types\GuidType; | ||
use Doctrine\DBAL\Types\IntegerType; | ||
use Doctrine\DBAL\Types\JsonType; | ||
use Doctrine\DBAL\Types\SimpleArrayType; | ||
use Doctrine\DBAL\Types\SmallIntType; | ||
use Doctrine\DBAL\Types\StringType; | ||
use Doctrine\DBAL\Types\TextType; | ||
use Doctrine\DBAL\Types\Type; | ||
use Doctrine\Deprecations\Deprecation; | ||
use Doctrine\ORM\EntityManagerInterface; | ||
use Doctrine\ORM\Mapping\ClassMetadata; | ||
use Doctrine\ORM\Mapping\ClassMetadataInfo; | ||
use ReflectionNamedType; | ||
|
||
use function array_diff; | ||
use function array_filter; | ||
use function array_key_exists; | ||
use function array_map; | ||
use function array_push; | ||
use function array_search; | ||
use function array_values; | ||
use function assert; | ||
use function class_exists; | ||
use function class_parents; | ||
use function count; | ||
use function get_class; | ||
use function implode; | ||
use function in_array; | ||
use function sprintf; | ||
|
||
use const PHP_VERSION_ID; | ||
|
||
/** | ||
* Performs strict validation of the mapping schema | ||
* | ||
* @link www.doctrine-project.com | ||
* | ||
* @psalm-import-type FieldMapping from ClassMetadata | ||
*/ | ||
class SchemaValidator | ||
{ | ||
/** @var EntityManagerInterface */ | ||
private $em; | ||
|
||
/** | ||
* It maps built-in Doctrine types to PHP types | ||
*/ | ||
private const BUILTIN_TYPES_MAP = [ | ||
AsciiStringType::class => 'string', | ||
BigIntType::class => 'string', | ||
BooleanType::class => 'bool', | ||
DecimalType::class => 'string', | ||
FloatType::class => 'float', | ||
GuidType::class => 'string', | ||
IntegerType::class => 'int', | ||
JsonType::class => 'array', | ||
SimpleArrayType::class => 'array', | ||
SmallIntType::class => 'int', | ||
StringType::class => 'string', | ||
TextType::class => 'string', | ||
]; | ||
|
||
public function __construct(EntityManagerInterface $em) | ||
{ | ||
$this->em = $em; | ||
|
@@ -92,6 +132,11 @@ | |
} | ||
} | ||
|
||
// PHP 7.4 introduces the ability to type properties, so we can't validate them in previous versions | ||
if (PHP_VERSION_ID >= 70400) { | ||
array_push($ce, ...$this->validatePropertiesTypes($class)); | ||
} | ||
|
||
if ($class->isEmbeddedClass && count($class->associationMappings) > 0) { | ||
$ce[] = "Embeddable '" . $class->name . "' does not support associations"; | ||
|
||
|
@@ -304,4 +349,66 @@ | |
|
||
return $schemaTool->getUpdateSchemaSql($allMetadata, true); | ||
} | ||
|
||
/** @return list<string> containing the found issues */ | ||
private function validatePropertiesTypes(ClassMetadataInfo $class): array | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have no choice but to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I understand this comment. Is this because of a possible different Persistence version and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm saying that in the unique call site of this method, we have no guarantee that we have a CM and not just a CMI (so yes, it does have to do with |
||
{ | ||
return array_values( | ||
array_filter( | ||
array_map( | ||
/** @param FieldMapping $fieldMapping */ | ||
function (array $fieldMapping) use ($class): ?string { | ||
$fieldName = $fieldMapping['fieldName']; | ||
assert(isset($class->reflFields[$fieldName])); | ||
$propertyType = $class->reflFields[$fieldName]->getType(); | ||
|
||
// If the field type is not a built-in type, we cannot check it | ||
if (! Type::hasType($fieldMapping['type'])) { | ||
return null; | ||
} | ||
|
||
// If the property type is not a named type, we cannot check it | ||
if (! ($propertyType instanceof ReflectionNamedType)) { | ||
return null; | ||
} | ||
|
||
$metadataFieldType = $this->findBuiltInType(Type::getType($fieldMapping['type'])); | ||
|
||
//If the metadata field type is not a mapped built-in type, we cannot check it | ||
if ($metadataFieldType === null) { | ||
return null; | ||
} | ||
|
||
$propertyType = $propertyType->getName(); | ||
|
||
// If the property type is the same as the metadata field type, we are ok | ||
if ($propertyType === $metadataFieldType) { | ||
return null; | ||
} | ||
|
||
return sprintf( | ||
"The field '%s#%s' has the property type '%s' that differs from the metadata field type '%s' returned by the '%s' DBAL type.", | ||
$class->name, | ||
$fieldName, | ||
$propertyType, | ||
$metadataFieldType, | ||
$fieldMapping['type'] | ||
); | ||
}, | ||
$class->fieldMappings | ||
) | ||
) | ||
); | ||
} | ||
|
||
/** | ||
* The exact DBAL type must be used (no subclasses), since consumers of doctrine/orm may have their own | ||
* customization around field types. | ||
*/ | ||
private function findBuiltInType(Type $type): ?string | ||
{ | ||
$typeName = get_class($type); | ||
|
||
return self::BUILTIN_TYPES_MAP[$typeName] ?? null; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace Doctrine\Tests\ORM\Functional\Ticket\GH10661; | ||
|
||
use Doctrine\ORM\EntityManagerInterface; | ||
use Doctrine\ORM\Tools\SchemaValidator; | ||
use Doctrine\Tests\OrmTestCase; | ||
|
||
/** | ||
* @requires PHP >= 7.4 | ||
*/ | ||
final class GH10661Test extends OrmTestCase | ||
{ | ||
/** @var EntityManagerInterface */ | ||
private $em; | ||
|
||
/** @var SchemaValidator */ | ||
private $validator; | ||
|
||
protected function setUp(): void | ||
{ | ||
$this->em = $this->getTestEntityManager(); | ||
$this->validator = new SchemaValidator($this->em); | ||
} | ||
|
||
public function testMetadataFieldTypeNotCoherentWithEntityPropertyType(): void | ||
{ | ||
$class = $this->em->getClassMetadata(InvalidEntity::class); | ||
$ce = $this->validator->validateClass($class); | ||
|
||
self::assertEquals( | ||
["The field 'Doctrine\Tests\ORM\Functional\Ticket\GH10661\InvalidEntity#property1' has the property type 'float' that differs from the metadata field type 'string' returned by the 'decimal' DBAL type."], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added the "returned by the 'decimal' DBAL type" part, so that people understand where we get the second type from. |
||
$ce | ||
); | ||
} | ||
|
||
public function testMetadataFieldTypeNotCoherentWithEntityPropertyTypeWithInheritance(): void | ||
{ | ||
$class = $this->em->getClassMetadata(InvalidChildEntity::class); | ||
$ce = $this->validator->validateClass($class); | ||
|
||
self::assertEquals( | ||
[ | ||
"The field 'Doctrine\Tests\ORM\Functional\Ticket\GH10661\InvalidChildEntity#property1' has the property type 'float' that differs from the metadata field type 'string' returned by the 'decimal' DBAL type.", | ||
"The field 'Doctrine\Tests\ORM\Functional\Ticket\GH10661\InvalidChildEntity#property2' has the property type 'int' that differs from the metadata field type 'string' returned by the 'string' DBAL type.", | ||
"The field 'Doctrine\Tests\ORM\Functional\Ticket\GH10661\InvalidChildEntity#anotherProperty' has the property type 'string' that differs from the metadata field type 'bool' returned by the 'boolean' DBAL type.", | ||
], | ||
$ce | ||
); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace Doctrine\Tests\ORM\Functional\Ticket\GH10661; | ||
|
||
use Doctrine\ORM\Mapping\Column; | ||
use Doctrine\ORM\Mapping\Entity; | ||
|
||
/** @Entity */ | ||
class InvalidChildEntity extends InvalidEntity | ||
{ | ||
/** @Column(type="string") */ | ||
protected int $property2; | ||
|
||
/** | ||
* @Column(type="boolean") | ||
*/ | ||
private string $anotherProperty; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace Doctrine\Tests\ORM\Functional\Ticket\GH10661; | ||
|
||
use Doctrine\ORM\Mapping\Column; | ||
use Doctrine\ORM\Mapping\Entity; | ||
use Doctrine\ORM\Mapping\Id; | ||
|
||
/** @Entity */ | ||
class InvalidEntity | ||
{ | ||
/** | ||
* @var int | ||
* @Id | ||
* @Column | ||
*/ | ||
protected $key; | ||
|
||
/** | ||
* @Column(type="decimal") | ||
*/ | ||
protected float $property1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I switched to decimal vs float here because I think it is a more common mistake, and it was the one I had in mind so I wanted to see how it looked for that case. |
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we should get rid of this harcoded map when DBAL v4 is out and switch to reflection on the DBAL type instead. The signatures will be typed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This list is intentional though:
instanceof
is NOT sufficient, because subclassing allows for types to be customized in userland.If
doctrine/dbal:^4
hasfinal
on all types, then we can useinstanceof
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I understand what you mean now:
convertToPhpType(): T
, so we look atT
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. It could go in a property that is wider than T, or even narrower but that's kind of dangerous I'd say. At the very least, the intersection of both types must not be empty.