Skip to content
This repository has been archived by the owner on Aug 23, 2023. It is now read-only.

[GH-34] Instead of NULL, unitialize typed properties without default. #35

Merged
merged 1 commit into from
Mar 21, 2020
Merged

[GH-34] Instead of NULL, unitialize typed properties without default. #35

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 25 additions & 0 deletions lib/Doctrine/Common/Reflection/TypedNoDefaultReflectionProperty.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,29 @@ public function getValue($object = null)
{
return $object !== null && $this->isInitialized($object) ? parent::getValue($object) : null;
}

/**
* {@inheritDoc}
*
* Works around the problem with setting typed no default properties to
* NULL which is not supported, instead unset() to uninitialize.
*
* @link https://github.com/doctrine/orm/issues/7999
*/
public function setValue($object, $value = null)
{
if ($value === null) {
$propertyName = $this->getName();

$unsetter = function () use ($propertyName) {
unset($this->$propertyName);
};
$unsetter = $unsetter->bindTo($object, $this->getDeclaringClass()->getName());
$unsetter();

return;
}

parent::setValue($object, $value);
Comment on lines +34 to +46
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth to keep a property with the original closure? Doing that is safe since Closure#bindTo() is a immutable operation. It would avoid creating maaaaany unnecessary closures in runtime but we should benchmark it to see the impact (memory/processing time.)

That would simplify this quite a lot, considering the unsetter is created at construction time:

Suggested change
if ($value === null) {
$propertyName = $this->getName();
$unsetter = function () use ($propertyName) {
unset($this->$propertyName);
};
$unsetter = $unsetter->bindTo($object, $this->getDeclaringClass()->getName());
$unsetter();
return;
}
parent::setValue($object, $value);
if ($value === null) {
$this->unsetter->bindTo($object, $this->getDeclaringClass()->getName())();
return;
}
parent::setValue($object, $value);

Copy link
Member

Choose a reason for hiding this comment

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

We could also save the result of $this->getDeclaringClass()->getName() to reduce call count to these methods, btw.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wouldn't optimize this yet, because i don't see this happening often. It is needed for deleting entities for sure, when ORM sets the id to null. But why would users have a null value for a typed, non-null property themselves? That be a weird usage I don't recommend anyways

}
}
23 changes: 23 additions & 0 deletions tests/Doctrine/Tests_PHP74/Common/Reflection/TypedNoDefaultReflectionPropertyTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,32 @@ public function testGetValue() : void

self::assertNull($reflProperty->getValue($object));
}

public function testSetValueNull() : void
{
$reflection = new TypedNoDefaultReflectionProperty(TypedFoo::class, 'id');
$reflection->setAccessible(true);

$object = new TypedFoo();
$object->setId(1);

$reflection->setValue($object, null);

self::assertNull($reflection->getValue($object));
}
}

class TypedNoDefaultReflectionPropertyTestClass
{
public string $test;
}

class TypedFoo
{
private int $id;

public function setId($id)
{
$this->id = $id;
}
}