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.

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

[GH-8723] Remove use of nullability to automatically detect nullable status #8732

Merged
merged 2 commits into from
May 31, 2021
Merged

[GH-8723] Remove use of nullability to automatically detect nullable status #8732

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6e6f81e
[GH-8723] Remove use of nullability to automatically detect nullable …
beberlei May 31, 2021
83fbfc2
[GH-8723] Make Column::$nullable default to false again, fix tests.
beberlei May 31, 2021
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: 1 addition & 3 deletions docs/en/reference/annotations-reference.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ Optional attributes:
- **unique**: Boolean value to determine if the value of the column
should be unique across all rows of the underlying entities table.

- **nullable**: Determines if NULL values allowed for this column. If not specified, default value is false. When using typed properties on entity class defaults to true when property is nullable.
- **nullable**: Determines if NULL values allowed for this column. If not specified, default value is false.

- **options**: Array of additional options:

Expand Down Expand Up @@ -649,8 +649,6 @@ Optional attributes:
constraint level. Defaults to false.
- **nullable**: Determine whether the related entity is required, or if
null is an allowed state for the relation. Defaults to true.
When using typed properties on entity class defaults to false when
property is not nullable.
- **onDelete**: Cascade Action (Database-level)
- **columnDefinition**: DDL SQL snippet that starts after the column
name and specifies the complete (non-portable!) column definition.
Expand Down
12 changes: 0 additions & 12 deletions lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1468,10 +1468,6 @@ private function validateAndCompleteTypedFieldMapping(array $mapping): array
$type = $this->reflClass->getProperty($mapping['fieldName'])->getType();

if ($type) {
if (! isset($mapping['nullable'])) {
$mapping['nullable'] = $type->allowsNull();
}

if (
! isset($mapping['type'])
&& ($type instanceof ReflectionNamedType)
Expand Down Expand Up @@ -1527,14 +1523,6 @@ private function validateAndCompleteTypedAssociationMapping(array $mapping): arr
$mapping['targetEntity'] = $type->getName();
}

if (isset($mapping['joinColumns'])) {
foreach ($mapping['joinColumns'] as &$joinColumn) {
if ($type->allowsNull() === false) {
Copy link

@zolex zolex May 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

instead of removing it, do we maybe want the same logic as for $mapping['nullable']?
e.g. if (! isset($joinColumn['nullable'])) $joinColumn['nullable'] = $type->allowsNull();

Copy link
Member Author

Choose a reason for hiding this comment

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

No, tthe column nullable behavior is also flawed, as there are legit cases where you want nullable behavior to be different between entity and database.

Copy link

@zolex zolex May 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

ah, at least for me the bug was only that I could not set the value at all in the JoinColumn annotation. it was always false even when providing it explicitly true.

but alright, I see setting the default based on the php type may be a bc where it is not defined in the annotation. so maybe this is really something for orm 3?

$joinColumn['nullable'] = false;
}
}
}

return $mapping;
}

Expand Down
6 changes: 3 additions & 3 deletions lib/Doctrine/ORM/Mapping/Column.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ final class Column implements Annotation
/** @var bool */
public $unique = false;

/** @var bool|null */
public $nullable;
/** @var bool */
public $nullable = false;

/** @var array<string,mixed> */
public $options = [];
Expand All @@ -76,7 +76,7 @@ public function __construct(
?int $precision = null,
?int $scale = null,
bool $unique = false,
?bool $nullable = null,
bool $nullable = false,
array $options = [],
?string $columnDefinition = null
) {
Expand Down
21 changes: 3 additions & 18 deletions tests/Doctrine/Tests/ORM/Mapping/AbstractMappingDriverTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,24 +265,6 @@ 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'));

$this->assertEquals(CmsEmail::class, $class->getAssociationMapping('email')['targetEntity']);
$this->assertEquals(CmsEmail::class, $class->getAssociationMapping('mainEmail')['targetEntity']);
}

public function testFieldTypeFromReflection(): void
{
if (PHP_VERSION_ID < 70400) {
Expand All @@ -299,6 +281,9 @@ public function testFieldTypeFromReflection(): void
$this->assertEquals('json', $class->getTypeOfField('array'));
$this->assertEquals('boolean', $class->getTypeOfField('boolean'));
$this->assertEquals('float', $class->getTypeOfField('float'));

$this->assertEquals(CmsEmail::class, $class->getAssociationMapping('email')['targetEntity']);
$this->assertEquals(CmsEmail::class, $class->getAssociationMapping('mainEmail')['targetEntity']);
}

/**
Expand Down
8 changes: 0 additions & 8 deletions tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,14 +120,6 @@ public function testFieldIsNullableByType(): void
$cm = new ClassMetadata(TypedProperties\UserTyped::class);
$cm->initializeReflection(new RuntimeReflectionService());

// Explicit Nullable
$cm->mapField(['fieldName' => 'status', 'length' => 50]);
$this->assertTrue($cm->isNullable('status'));

// Explicit Not Nullable
$cm->mapField(['fieldName' => 'username', 'length' => 50]);
$this->assertFalse($cm->isNullable('username'));

$cm->mapOneToOne(['fieldName' => 'email', 'joinColumns' => [[]]]);
$this->assertEquals(CmsEmail::class, $cm->getAssociationMapping('email')['targetEntity']);

Expand Down