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

Adds metadata field type validation against Entity property type #10662

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
56 changes: 56 additions & 0 deletions lib/Doctrine/ORM/Mapping/DefaultTypedFieldMapper.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,27 @@
use DateInterval;
use DateTime;
use DateTimeImmutable;
use Doctrine\DBAL\Types\AsciiStringType;
use Doctrine\DBAL\Types\BigIntType;
use Doctrine\DBAL\Types\BooleanType;
use Doctrine\DBAL\Types\DateImmutableType;
use Doctrine\DBAL\Types\DateIntervalType;
use Doctrine\DBAL\Types\DateTimeImmutableType;
use Doctrine\DBAL\Types\DateTimeType;
use Doctrine\DBAL\Types\DateTimeTzImmutableType;
use Doctrine\DBAL\Types\DateTimeTzType;
use Doctrine\DBAL\Types\DateType;
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\TimeImmutableType;
use Doctrine\DBAL\Types\TimeType;
use Doctrine\DBAL\Types\Type;
use Doctrine\DBAL\Types\Types;
use ReflectionEnum;
Expand Down Expand Up @@ -36,12 +57,47 @@ final class DefaultTypedFieldMapper implements TypedFieldMapper
'string' => Types::STRING,
];

/** It maps built-in Doctrine types to PHP types */
private const BUILTIN_TYPES_MAP = [
AsciiStringType::class => 'string',
BigIntType::class => 'string',
BooleanType::class => 'bool',
DateType::class => DateTime::class,
DateImmutableType::class => DateTimeImmutable::class,
DateIntervalType::class => DateInterval::class,
DateTimeType::class => DateTime::class,
DateTimeImmutableType::class => DateTimeImmutable::class,
DateTimeTzType::class => DateTime::class,
DateTimeTzImmutableType::class => DateTimeImmutable::class,
DavideBicego marked this conversation as resolved.
Show resolved Hide resolved
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',
TimeType::class => DateTime::class,
TimeImmutableType::class => DateTimeImmutable::class,
DavideBicego marked this conversation as resolved.
Show resolved Hide resolved
];
DavideBicego marked this conversation as resolved.
Show resolved Hide resolved

/** @param array<class-string|ScalarName, class-string<Type>|string> $typedFieldMappings */
public function __construct(array $typedFieldMappings = [])
{
$this->typedFieldMappings = array_merge(self::DEFAULT_TYPED_FIELD_MAPPINGS, $typedFieldMappings);
}

/**
* @internal
*
* @param class-string<Type> $typeName
DavideBicego marked this conversation as resolved.
Show resolved Hide resolved
*/
public function getBuiltInType(string $typeName): string
{
return self::BUILTIN_TYPES_MAP[$typeName] ?? $typeName;
}
DavideBicego marked this conversation as resolved.
Show resolved Hide resolved

/**
* {@inheritDoc}
*/
Expand Down
53 changes: 53 additions & 0 deletions lib/Doctrine/ORM/Tools/SchemaValidator.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,14 @@
use Doctrine\ORM\EntityManagerInterface;
use Doctrine\ORM\Mapping\ClassMetadata;
use Doctrine\ORM\Mapping\ClassMetadataInfo;
use Doctrine\ORM\Mapping\DefaultTypedFieldMapper;
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 class_exists;
Expand All @@ -20,11 +25,14 @@
use function get_class;
use function implode;
use function in_array;
use function sprintf;

/**
* Performs strict validation of the mapping schema
*
* @link www.doctrine-project.com
*
* @psalm-import-type FieldMapping from ClassMetadataInfo
*/
class SchemaValidator
{
Expand Down Expand Up @@ -92,6 +100,8 @@ public function validateClass(ClassMetadataInfo $class)
}
}

array_push($ce, ...$this->validatePropertiesTypes($class));

if ($class->isEmbeddedClass && count($class->associationMappings) > 0) {
$ce[] = "Embeddable '" . $class->name . "' does not support associations";

Expand Down Expand Up @@ -298,4 +308,47 @@ public function getUpdateSchemaList(): array

return $schemaTool->getUpdateSchemaSql($allMetadata, true);
}

/** @return list<string> containing the found issues */
private function validatePropertiesTypes(ClassMetadataInfo $class): array
{
return array_values(
array_filter(
array_map(
/** @param FieldMapping $fieldMapping */
static function (array $fieldMapping) use ($class): string|null {
$fieldName = $fieldMapping['fieldName'];
$propertyType = $class->reflFields[$fieldName]?->getType();
DavideBicego marked this conversation as resolved.
Show resolved Hide resolved

// 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 = (new DefaultTypedFieldMapper())->getBuiltInType(get_class(Type::getType($fieldMapping['type'])));
DavideBicego marked this conversation as resolved.
Show resolved Hide resolved

// If the property type is the same as the metadata field type, we are ok
if ($metadataFieldType === $propertyType->getName()) {
return null;
}

return sprintf(
"The field '%s#%s' has the property type '%s' that differs from the metadata field type '%s'.",
$class->name,
$fieldName,
$propertyType->getName(),
$metadataFieldType,
);
},
$class->fieldMappings,
),
static fn (?string $issue) => $issue !== null,
DavideBicego marked this conversation as resolved.
Show resolved Hide resolved
)
);
}
}
67 changes: 60 additions & 7 deletions tests/Doctrine/Tests/ORM/Tools/SchemaValidatorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -45,21 +45,21 @@ protected function setUp(): void
public function testCmsModelSet(string $path): void
{
$this->em->getConfiguration()
->getMetadataDriverImpl()
->addPaths([$path]);
->getMetadataDriverImpl()
->addPaths([$path]);

self::assertEmpty($this->validator->validateMapping());
}

public static function modelSetProvider(): array
{
return [
'cms' => [__DIR__ . '/../../Models/CMS'],
'company' => [__DIR__ . '/../../Models/Company'],
'ecommerce' => [__DIR__ . '/../../Models/ECommerce'],
'forum' => [__DIR__ . '/../../Models/Forum'],
'cms' => [__DIR__ . '/../../Models/CMS'],
'company' => [__DIR__ . '/../../Models/Company'],
'ecommerce' => [__DIR__ . '/../../Models/ECommerce'],
'forum' => [__DIR__ . '/../../Models/Forum'],
'navigation' => [__DIR__ . '/../../Models/Navigation'],
'routing' => [__DIR__ . '/../../Models/Routing'],
'routing' => [__DIR__ . '/../../Models/Routing'],
];
}

Expand Down Expand Up @@ -229,6 +229,31 @@ public function testAbstractChildClassNotPresentInDiscriminator(): void

self::assertEmpty($ce);
}

public function testMetadataFieldTypeNotCoherentWithEntityPropertyType(): void
{
$class = $this->em->getClassMetadata(InvalidEntity3::class);
$ce = $this->validator->validateClass($class);

self::assertEquals(
["The field 'Doctrine\Tests\ORM\Tools\InvalidEntity3#property' has the property type 'int' that differs from the metadata field type 'bool'."],
$ce
);
}

public function testMetadataFieldTypeNotCoherentWithEntityPropertyTypeWithInheritance(): void
{
$class = $this->em->getClassMetadata(InvalidChildEntity::class);
$ce = $this->validator->validateClass($class);

self::assertEquals(
[
"The field 'Doctrine\Tests\ORM\Tools\InvalidChildEntity#property' has the property type 'int' that differs from the metadata field type 'bool'.",
"The field 'Doctrine\Tests\ORM\Tools\InvalidChildEntity#anotherProperty' has the property type 'string' that differs from the metadata field type 'bool'.",
],
$ce
);
}
}

/** @MappedSuperclass */
Expand Down Expand Up @@ -675,3 +700,31 @@ abstract class Issue9095AbstractChild extends Issue9095Parent
class Issue9095Child extends Issue9095AbstractChild
{
}

/** @Entity */
class InvalidEntity3
{
/**
* @var int
* @Id
* @Column
*/
protected $key;

/**
* @Column(type="boolean")
*/
protected int $property;
Copy link
Member

Choose a reason for hiding this comment

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

Ooooops, the test fails here because Doctrine didn't yet drop PHP 7.3 support (REALLY?!)

I think the best way to handle this would be to:

  1. split this out to a separate file
  2. mark the tests with @requires php>=7.4
  3. exclude the separated file from analysis by tooling like phpcs/phpstan

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would you prevent the file from being read with php < 7.4?
The test should not be executed with a @requires php 70400 but the file contains some classes definition too...

Copy link
Member

Choose a reason for hiding this comment

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

I don't remember how the test suite works precisely, but can check it tomorrow.

I'd say something like require_once __DIR__ . '/some-php-7.4-only-file.php and then reference the classes in that file that way

Copy link
Member

@greg0ire greg0ire May 6, 2023

Choose a reason for hiding this comment

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

Or use autoload, or team up with @MatTheCat who is having the same issue on https://github.com/doctrine/orm/pull/10666/files#r1186698321

We could have a subdirectories with tests that require a specific version of PHP, and then future contributors would not need to bother with @requires or with splitting test files and companion classes.

}

/** @Entity */
class InvalidChildEntity extends InvalidEntity3
{
/** @Column(type="string") */
protected int $property;
DavideBicego marked this conversation as resolved.
Show resolved Hide resolved

/**
* @Column(type="boolean")
*/
private string $anotherProperty;
}