From a2230485b2f54b6046f7e1f736d15be18cfef2fa Mon Sep 17 00:00:00 2001 From: Jakub Caban Date: Sun, 18 Apr 2021 14:26:41 +0200 Subject: [PATCH] Fix typed properties for default metadata (#7939) (#8589) * Make Column::$type, Column::$nullable and JoinColumn::$nullable nullable by default * Add tests for mapped typed properties (type and nullable) * Fix Yaml driver tests and remove driver exceptions thrown too early * Fix PHP driver tests * Fix static PHP driver tests * Fix XML driver tests * Coding Standards * Deprecate unused MappingException method * Add manyToOne test and check nullable at the right place * Coding Standards * Bugfix: Temporarily change association join columns in CascadeRemoveOrderTest to circumvent new CommitOrderCalculator bug. * phpcs Co-authored-by: Benjamin Eberlei --- doctrine-mapping.xsd | 4 +- .../ORM/Mapping/ClassMetadataInfo.php | 5 ++ lib/Doctrine/ORM/Mapping/Column.php | 10 +-- .../ORM/Mapping/Driver/AnnotationDriver.php | 4 - .../ORM/Mapping/Driver/AttributeDriver.php | 4 - lib/Doctrine/ORM/Mapping/Driver/XmlDriver.php | 20 ++++- .../ORM/Mapping/Driver/YamlDriver.php | 4 +- lib/Doctrine/ORM/Mapping/JoinColumn.php | 6 +- lib/Doctrine/ORM/Mapping/ManyToOne.php | 2 +- lib/Doctrine/ORM/Mapping/MappingException.php | 2 + .../Models/TypedProperties/UserTyped.php | 83 +++++++++++++++++++ .../ORM/Functional/CascadeRemoveOrderTest.php | 1 + .../ORM/Mapping/AbstractMappingDriverTest.php | 43 ++++++++++ .../Tests/ORM/Mapping/ClassMetadataTest.php | 4 + ...Tests.Models.TypedProperties.UserTyped.php | 63 ++++++++++++++ ...s.Models.TypedProperties.UserTyped.dcm.xml | 29 +++++++ ...s.Models.TypedProperties.UserTyped.dcm.yml | 26 ++++++ 17 files changed, 285 insertions(+), 25 deletions(-) create mode 100644 tests/Doctrine/Tests/ORM/Mapping/php/Doctrine.Tests.Models.TypedProperties.UserTyped.php create mode 100644 tests/Doctrine/Tests/ORM/Mapping/xml/Doctrine.Tests.Models.TypedProperties.UserTyped.dcm.xml create mode 100644 tests/Doctrine/Tests/ORM/Mapping/yaml/Doctrine.Tests.Models.TypedProperties.UserTyped.dcm.yml diff --git a/doctrine-mapping.xsd b/doctrine-mapping.xsd index 83ac9e4aa66..94795e439b6 100644 --- a/doctrine-mapping.xsd +++ b/doctrine-mapping.xsd @@ -541,7 +541,7 @@ - + @@ -559,7 +559,7 @@ - + diff --git a/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php b/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php index a33af81b9cc..a98075f1e4c 100644 --- a/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php +++ b/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php @@ -1758,6 +1758,7 @@ protected function _validateAndCompleteOneToOneMapping(array $mapping) [ 'name' => $this->namingStrategy->joinColumnName($mapping['fieldName'], $this->name), 'referencedColumnName' => $this->namingStrategy->referenceColumnName(), + 'nullable' => true, ], ]; } @@ -1775,6 +1776,10 @@ protected function _validateAndCompleteOneToOneMapping(array $mapping) } } + if (! isset($joinColumn['nullable'])) { + $joinColumn['nullable'] = true; + } + if (empty($joinColumn['name'])) { $joinColumn['name'] = $this->namingStrategy->joinColumnName($mapping['fieldName'], $this->name); } diff --git a/lib/Doctrine/ORM/Mapping/Column.php b/lib/Doctrine/ORM/Mapping/Column.php index 4b57501833d..8ef273695e0 100644 --- a/lib/Doctrine/ORM/Mapping/Column.php +++ b/lib/Doctrine/ORM/Mapping/Column.php @@ -35,7 +35,7 @@ final class Column implements Annotation public $name; /** @var mixed */ - public $type = 'string'; + public $type; /** @var int */ public $length; @@ -57,8 +57,8 @@ final class Column implements Annotation /** @var bool */ public $unique = false; - /** @var bool */ - public $nullable = false; + /** @var bool|null */ + public $nullable; /** @var array */ public $options = []; @@ -71,12 +71,12 @@ final class Column implements Annotation */ public function __construct( ?string $name = null, - string $type = 'string', + ?string $type = null, ?int $length = null, ?int $precision = null, ?int $scale = null, bool $unique = false, - bool $nullable = false, + ?bool $nullable = null, array $options = [], ?string $columnDefinition = null ) { diff --git a/lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php b/lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php index 6810aca9c44..0178ba64078 100644 --- a/lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php +++ b/lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php @@ -370,10 +370,6 @@ public function loadMetadataForClass($className, ClassMetadata $metadata) // @Column, @OneToOne, @OneToMany, @ManyToOne, @ManyToMany $columnAnnot = $this->reader->getPropertyAnnotation($property, Mapping\Column::class); if ($columnAnnot) { - if ($columnAnnot->type === null) { - throw MappingException::propertyTypeIsRequired($className, $property->getName()); - } - $mapping = $this->columnToArray($property->getName(), $columnAnnot); $idAnnot = $this->reader->getPropertyAnnotation($property, Mapping\Id::class); diff --git a/lib/Doctrine/ORM/Mapping/Driver/AttributeDriver.php b/lib/Doctrine/ORM/Mapping/Driver/AttributeDriver.php index 06d11bef944..819c595378e 100644 --- a/lib/Doctrine/ORM/Mapping/Driver/AttributeDriver.php +++ b/lib/Doctrine/ORM/Mapping/Driver/AttributeDriver.php @@ -258,10 +258,6 @@ public function loadMetadataForClass($className, ClassMetadata $metadata): void $embeddedAttribute = $this->reader->getPropertyAnnotation($property, Mapping\Embedded::class); if ($columnAttribute !== null) { - if ($columnAttribute->type === null) { - throw MappingException::propertyTypeIsRequired($className, $property->getName()); - } - $mapping = $this->columnToArray($property->getName(), $columnAttribute); if ($this->reader->getPropertyAnnotation($property, Mapping\Id::class)) { diff --git a/lib/Doctrine/ORM/Mapping/Driver/XmlDriver.php b/lib/Doctrine/ORM/Mapping/Driver/XmlDriver.php index bbd20f40f09..d3286680833 100644 --- a/lib/Doctrine/ORM/Mapping/Driver/XmlDriver.php +++ b/lib/Doctrine/ORM/Mapping/Driver/XmlDriver.php @@ -409,9 +409,12 @@ public function loadMetadataForClass($className, ClassMetadata $metadata) foreach ($xmlRoot->{'one-to-one'} as $oneToOneElement) { $mapping = [ 'fieldName' => (string) $oneToOneElement['field'], - 'targetEntity' => (string) $oneToOneElement['target-entity'], ]; + if (isset($oneToOneElement['target-entity'])) { + $mapping['targetEntity'] = (string) $oneToOneElement['target-entity']; + } + if (isset($associationIds[$mapping['fieldName']])) { $mapping['id'] = true; } @@ -462,10 +465,13 @@ public function loadMetadataForClass($className, ClassMetadata $metadata) foreach ($xmlRoot->{'one-to-many'} as $oneToManyElement) { $mapping = [ 'fieldName' => (string) $oneToManyElement['field'], - 'targetEntity' => (string) $oneToManyElement['target-entity'], 'mappedBy' => (string) $oneToManyElement['mapped-by'], ]; + if (isset($oneToManyElement['target-entity'])) { + $mapping['targetEntity'] = (string) $oneToManyElement['target-entity']; + } + if (isset($oneToManyElement['fetch'])) { $mapping['fetch'] = constant('Doctrine\ORM\Mapping\ClassMetadata::FETCH_' . (string) $oneToManyElement['fetch']); } @@ -509,9 +515,12 @@ public function loadMetadataForClass($className, ClassMetadata $metadata) foreach ($xmlRoot->{'many-to-one'} as $manyToOneElement) { $mapping = [ 'fieldName' => (string) $manyToOneElement['field'], - 'targetEntity' => (string) $manyToOneElement['target-entity'], ]; + if (isset($manyToOneElement['target-entity'])) { + $mapping['targetEntity'] = (string) $manyToOneElement['target-entity']; + } + if (isset($associationIds[$mapping['fieldName']])) { $mapping['id'] = true; } @@ -554,9 +563,12 @@ public function loadMetadataForClass($className, ClassMetadata $metadata) foreach ($xmlRoot->{'many-to-many'} as $manyToManyElement) { $mapping = [ 'fieldName' => (string) $manyToManyElement['field'], - 'targetEntity' => (string) $manyToManyElement['target-entity'], ]; + if (isset($manyToManyElement['target-entity'])) { + $mapping['targetEntity'] = (string) $manyToManyElement['target-entity']; + } + if (isset($manyToManyElement['fetch'])) { $mapping['fetch'] = constant('Doctrine\ORM\Mapping\ClassMetadata::FETCH_' . (string) $manyToManyElement['fetch']); } diff --git a/lib/Doctrine/ORM/Mapping/Driver/YamlDriver.php b/lib/Doctrine/ORM/Mapping/Driver/YamlDriver.php index cb139e4145b..8ebf7061b33 100644 --- a/lib/Doctrine/ORM/Mapping/Driver/YamlDriver.php +++ b/lib/Doctrine/ORM/Mapping/Driver/YamlDriver.php @@ -420,7 +420,7 @@ public function loadMetadataForClass($className, ClassMetadata $metadata) foreach ($element['oneToOne'] as $name => $oneToOneElement) { $mapping = [ 'fieldName' => $name, - 'targetEntity' => $oneToOneElement['targetEntity'], + 'targetEntity' => $oneToOneElement['targetEntity'] ?? null, ]; if (isset($associationIds[$mapping['fieldName']])) { @@ -515,7 +515,7 @@ public function loadMetadataForClass($className, ClassMetadata $metadata) foreach ($element['manyToOne'] as $name => $manyToOneElement) { $mapping = [ 'fieldName' => $name, - 'targetEntity' => $manyToOneElement['targetEntity'], + 'targetEntity' => $manyToOneElement['targetEntity'] ?? null, ]; if (isset($associationIds[$mapping['fieldName']])) { diff --git a/lib/Doctrine/ORM/Mapping/JoinColumn.php b/lib/Doctrine/ORM/Mapping/JoinColumn.php index 7511b7dd226..e9bf26dde91 100644 --- a/lib/Doctrine/ORM/Mapping/JoinColumn.php +++ b/lib/Doctrine/ORM/Mapping/JoinColumn.php @@ -40,8 +40,8 @@ final class JoinColumn implements Annotation /** @var bool */ public $unique = false; - /** @var bool */ - public $nullable = true; + /** @var bool|null */ + public $nullable; /** @var mixed */ public $onDelete; @@ -60,7 +60,7 @@ public function __construct( ?string $name = null, string $referencedColumnName = 'id', bool $unique = false, - bool $nullable = true, + ?bool $nullable = null, $onDelete = null, ?string $columnDefinition = null, ?string $fieldName = null diff --git a/lib/Doctrine/ORM/Mapping/ManyToOne.php b/lib/Doctrine/ORM/Mapping/ManyToOne.php index 00e59aa1310..e7bc2df0f51 100644 --- a/lib/Doctrine/ORM/Mapping/ManyToOne.php +++ b/lib/Doctrine/ORM/Mapping/ManyToOne.php @@ -52,7 +52,7 @@ final class ManyToOne implements Annotation * @param array $cascade */ public function __construct( - string $targetEntity, + ?string $targetEntity = null, ?array $cascade = null, string $fetch = 'LAZY', ?string $inversedBy = null diff --git a/lib/Doctrine/ORM/Mapping/MappingException.php b/lib/Doctrine/ORM/Mapping/MappingException.php index 3933f7bfdbc..888e49a7b5f 100644 --- a/lib/Doctrine/ORM/Mapping/MappingException.php +++ b/lib/Doctrine/ORM/Mapping/MappingException.php @@ -382,6 +382,8 @@ public static function classIsNotAValidEntityOrMappedSuperClass($className) } /** + * @deprecated 2.9 no longer in use + * * @param string $className * @param string $propertyName * diff --git a/tests/Doctrine/Tests/Models/TypedProperties/UserTyped.php b/tests/Doctrine/Tests/Models/TypedProperties/UserTyped.php index 82dfa9faccd..8d3cdb0174a 100644 --- a/tests/Doctrine/Tests/Models/TypedProperties/UserTyped.php +++ b/tests/Doctrine/Tests/Models/TypedProperties/UserTyped.php @@ -7,46 +7,129 @@ use DateInterval; use DateTime; use DateTimeImmutable; +use Doctrine\ORM\Mapping as ORM; +use Doctrine\ORM\Mapping\ClassMetadataInfo; use Doctrine\Tests\Models\CMS\CmsEmail; /** * @Entity * @Table(name="cms_users_typed") */ +#[ORM\Entity, ORM\Table(name: 'cms_users_typed')] class UserTyped { /** * @Id @Column * @GeneratedValue */ + #[ORM\Id, ORM\Column, ORM\GeneratedValue] public int $id; /** @Column(length=50) */ + #[ORM\Column(length: 50)] public ?string $status; /** @Column(length=255, unique=true) */ + #[ORM\Column(length: 255, unique: true)] public string $username; /** @Column */ + #[ORM\Column] public DateInterval $dateInterval; /** @Column */ + #[ORM\Column] public DateTime $dateTime; /** @Column */ + #[ORM\Column] public DateTimeImmutable $dateTimeImmutable; /** @Column */ + #[ORM\Column] public array $array; /** @Column */ + #[ORM\Column] public bool $boolean; /** @Column */ + #[ORM\Column] public float $float; /** * @OneToOne(cascade={"persist"}, orphanRemoval=true) * @JoinColumn */ + #[ORM\OneToOne(cascade: ['persist'], orphanRemoval: true), ORM\JoinColumn] public CmsEmail $email; + + /** @ManyToOne */ + #[ORM\ManyToOne] + public ?CmsEmail $mainEmail; + + public static function loadMetadata(ClassMetadataInfo $metadata): void + { + $metadata->setInheritanceType(ClassMetadataInfo::INHERITANCE_TYPE_NONE); + $metadata->setPrimaryTable( + ['name' => 'cms_users_typed'] + ); + + $metadata->mapField( + [ + 'id' => true, + 'fieldName' => 'id', + ] + ); + $metadata->setIdGeneratorType(ClassMetadataInfo::GENERATOR_TYPE_AUTO); + + $metadata->mapField( + [ + 'fieldName' => 'status', + 'length' => 50, + ] + ); + $metadata->mapField( + [ + 'fieldName' => 'username', + 'length' => 255, + 'unique' => true, + ] + ); + $metadata->mapField( + ['fieldName' => 'dateInterval'] + ); + $metadata->mapField( + ['fieldName' => 'dateTime'] + ); + $metadata->mapField( + ['fieldName' => 'dateTimeImmutable'] + ); + $metadata->mapField( + ['fieldName' => 'array'] + ); + $metadata->mapField( + ['fieldName' => 'boolean'] + ); + $metadata->mapField( + ['fieldName' => 'float'] + ); + + $metadata->mapOneToOne( + [ + 'fieldName' => 'email', + 'cascade' => + [0 => 'persist'], + 'joinColumns' => + [ + 0 => + [], + ], + 'orphanRemoval' => true, + ] + ); + + $metadata->mapManyToOne( + ['fieldName' => 'mainEmail'] + ); + } } diff --git a/tests/Doctrine/Tests/ORM/Functional/CascadeRemoveOrderTest.php b/tests/Doctrine/Tests/ORM/Functional/CascadeRemoveOrderTest.php index 4823c6c6dc4..c58947f0419 100644 --- a/tests/Doctrine/Tests/ORM/Functional/CascadeRemoveOrderTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/CascadeRemoveOrderTest.php @@ -158,6 +158,7 @@ class CascadeRemoveOrderEntityG * targetEntity="Doctrine\Tests\ORM\Functional\CascadeRemoveOrderEntityO", * inversedBy="oneToMany" * ) + * @JoinColumn(nullable=true, onDelete="SET NULL") */ private $ownerO; diff --git a/tests/Doctrine/Tests/ORM/Mapping/AbstractMappingDriverTest.php b/tests/Doctrine/Tests/ORM/Mapping/AbstractMappingDriverTest.php index e4b0463a486..b7f3f50e425 100644 --- a/tests/Doctrine/Tests/ORM/Mapping/AbstractMappingDriverTest.php +++ b/tests/Doctrine/Tests/ORM/Mapping/AbstractMappingDriverTest.php @@ -21,6 +21,7 @@ use Doctrine\Tests\Models\Cache\City; use Doctrine\Tests\Models\CMS\CmsAddress; use Doctrine\Tests\Models\CMS\CmsAddressListener; +use Doctrine\Tests\Models\CMS\CmsEmail; use Doctrine\Tests\Models\CMS\CmsUser; use Doctrine\Tests\Models\Company\CompanyContract; use Doctrine\Tests\Models\Company\CompanyContractListener; @@ -41,6 +42,7 @@ use Doctrine\Tests\Models\DDC889\DDC889Entity; use Doctrine\Tests\Models\DDC964\DDC964Admin; use Doctrine\Tests\Models\DDC964\DDC964Guest; +use Doctrine\Tests\Models\TypedProperties\UserTyped; use Doctrine\Tests\OrmTestCase; use function assert; @@ -51,6 +53,7 @@ use function strtolower; use const CASE_UPPER; +use const PHP_VERSION_ID; abstract class AbstractMappingDriverTest extends OrmTestCase { @@ -262,6 +265,46 @@ public function testStringFieldMappings(ClassMetadata $class): ClassMetadata return $class; } + public function testFieldIsNullableByType(): void + { + if (PHP_VERSION_ID < 70400) { + $this->markTestSkipped('requies PHP 7.4'); + } + + $class = $this->createClassMetadata(UserTyped::class); + + // Explicit Nullable + $this->assertTrue($class->isNullable('status')); + + // Explicit Not Nullable + $this->assertFalse($class->isNullable('username')); + + // Join table Nullable + $this->assertFalse($class->getAssociationMapping('email')['joinColumns'][0]['nullable']); + $this->assertEquals(CmsEmail::class, $class->getAssociationMapping('email')['targetEntity']); + + $this->assertTrue($class->getAssociationMapping('mainEmail')['joinColumns'][0]['nullable']); + $this->assertEquals(CmsEmail::class, $class->getAssociationMapping('mainEmail')['targetEntity']); + } + + public function testFieldTypeFromReflection(): void + { + if (PHP_VERSION_ID < 70400) { + $this->markTestSkipped('requies PHP 7.4'); + } + + $class = $this->createClassMetadata(UserTyped::class); + + $this->assertEquals('integer', $class->getTypeOfField('id')); + $this->assertEquals('string', $class->getTypeOfField('username')); + $this->assertEquals('dateinterval', $class->getTypeOfField('dateInterval')); + $this->assertEquals('datetime', $class->getTypeOfField('dateTime')); + $this->assertEquals('datetime_immutable', $class->getTypeOfField('dateTimeImmutable')); + $this->assertEquals('json', $class->getTypeOfField('array')); + $this->assertEquals('boolean', $class->getTypeOfField('boolean')); + $this->assertEquals('float', $class->getTypeOfField('float')); + } + /** * @depends testEntityTableNameAndInheritance */ diff --git a/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php b/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php index 122782096bd..7a9d2ebfe2c 100644 --- a/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php +++ b/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php @@ -132,6 +132,10 @@ public function testFieldIsNullableByType(): void $cm->mapOneToOne(['fieldName' => 'email', 'joinColumns' => [[]]]); $this->assertFalse($cm->getAssociationMapping('email')['joinColumns'][0]['nullable']); $this->assertEquals(CmsEmail::class, $cm->getAssociationMapping('email')['targetEntity']); + + $cm->mapManyToOne(['fieldName' => 'mainEmail']); + $this->assertTrue($cm->getAssociationMapping('mainEmail')['joinColumns'][0]['nullable']); + $this->assertEquals(CmsEmail::class, $cm->getAssociationMapping('mainEmail')['targetEntity']); } public function testFieldTypeFromReflection(): void diff --git a/tests/Doctrine/Tests/ORM/Mapping/php/Doctrine.Tests.Models.TypedProperties.UserTyped.php b/tests/Doctrine/Tests/ORM/Mapping/php/Doctrine.Tests.Models.TypedProperties.UserTyped.php new file mode 100644 index 00000000000..9fbcbe55d44 --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Mapping/php/Doctrine.Tests.Models.TypedProperties.UserTyped.php @@ -0,0 +1,63 @@ +setInheritanceType(ClassMetadataInfo::INHERITANCE_TYPE_NONE); +$metadata->setPrimaryTable( + ['name' => 'cms_users_typed'] +); + +$metadata->mapField( + [ + 'id' => true, + 'fieldName' => 'id', + ] +); +$metadata->setIdGeneratorType(ClassMetadataInfo::GENERATOR_TYPE_AUTO); + +$metadata->mapField( + [ + 'fieldName' => 'status', + 'length' => 50, + ] +); +$metadata->mapField( + [ + 'fieldName' => 'username', + 'length' => 255, + 'unique' => true, + ] +); +$metadata->mapField( + ['fieldName' => 'dateInterval'] +); +$metadata->mapField( + ['fieldName' => 'dateTime'] +); +$metadata->mapField( + ['fieldName' => 'dateTimeImmutable'] +); +$metadata->mapField( + ['fieldName' => 'array'] +); +$metadata->mapField( + ['fieldName' => 'boolean'] +); +$metadata->mapField( + ['fieldName' => 'float'] +); + +$metadata->mapOneToOne( + [ + 'fieldName' => 'email', + 'cascade' => [0 => 'persist'], + 'joinColumns' => [[]], + 'orphanRemoval' => true, + ] +); + +$metadata->mapManyToOne( + ['fieldName' => 'mainEmail'] +); diff --git a/tests/Doctrine/Tests/ORM/Mapping/xml/Doctrine.Tests.Models.TypedProperties.UserTyped.dcm.xml b/tests/Doctrine/Tests/ORM/Mapping/xml/Doctrine.Tests.Models.TypedProperties.UserTyped.dcm.xml new file mode 100644 index 00000000000..90dd5a46362 --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Mapping/xml/Doctrine.Tests.Models.TypedProperties.UserTyped.dcm.xml @@ -0,0 +1,29 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/tests/Doctrine/Tests/ORM/Mapping/yaml/Doctrine.Tests.Models.TypedProperties.UserTyped.dcm.yml b/tests/Doctrine/Tests/ORM/Mapping/yaml/Doctrine.Tests.Models.TypedProperties.UserTyped.dcm.yml new file mode 100644 index 00000000000..503a67fb34d --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Mapping/yaml/Doctrine.Tests.Models.TypedProperties.UserTyped.dcm.yml @@ -0,0 +1,26 @@ +Doctrine\Tests\Models\TypedProperties\UserTyped: + type: entity + table: cms_users_typed + id: + id: + generator: + strategy: AUTO + fields: + status: + length: 50 + username: + length: 255 + unique: true + dateInterval: ~ + dateTime: ~ + dateTimeImmutable: ~ + array: ~ + boolean: ~ + float: ~ + oneToOne: + email: + cascade: [ persist ] + orphanRemoval: true + joinColumn: [] + manyToOne: + mainEmail: []