-
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
check readonly property via serialization #11617
base: 2.20.x
Are you sure you want to change the base?
Changes from 1 commit
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 |
---|---|---|
@@ -0,0 +1,22 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace Doctrine\Tests\Models\ReadonlyProperties; | ||
|
||
use Doctrine\ORM\Mapping\Column; | ||
use Doctrine\ORM\Mapping\Entity; | ||
use Doctrine\ORM\Mapping\GeneratedValue; | ||
use Doctrine\ORM\Mapping\Id; | ||
use Doctrine\ORM\Mapping\Table; | ||
use Doctrine\Tests\Models\ValueObjects\Uuid; | ||
|
||
#[Entity] | ||
#[Table(name: 'library')] | ||
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. Does the table name matter? If not, I'd remove that annotation entirely, let's eliminate unnecessary distractions. |
||
class Library | ||
{ | ||
#[Column] | ||
#[Id] | ||
#[GeneratedValue(strategy: 'NONE')] | ||
public readonly Uuid $uuid; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace Doctrine\Tests\Models\ValueObjects; | ||
|
||
class Uuid | ||
{ | ||
/** @var string */ | ||
public $id; | ||
|
||
public function __construct(string $id) | ||
{ | ||
$this->id = $id; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,9 @@ | |
use Doctrine\ORM\Mapping\ReflectionReadonlyProperty; | ||
use Doctrine\Tests\Models\CMS\CmsTag; | ||
use Doctrine\Tests\Models\ReadonlyProperties\Author; | ||
use Doctrine\Tests\Models\ReadonlyProperties\Library; | ||
use Doctrine\Tests\Models\ValueObjects\Uuid; | ||
use Generator; | ||
use InvalidArgumentException; | ||
use LogicException; | ||
use PHPUnit\Framework\TestCase; | ||
|
@@ -17,36 +20,83 @@ | |
*/ | ||
class ReflectionReadonlyPropertyTest extends TestCase | ||
{ | ||
public function testSecondWriteWithSameValue(): void | ||
/** | ||
* @dataProvider sameValueProvider | ||
*/ | ||
public function testSecondWriteWithSameValue(object $entity, string $property, mixed $value, mixed $sameValue): void | ||
{ | ||
$author = new Author(); | ||
|
||
$wrappedReflection = new ReflectionProperty($author, 'name'); | ||
$wrappedReflection = new ReflectionProperty($entity, $property); | ||
$reflection = new ReflectionReadonlyProperty($wrappedReflection); | ||
|
||
$reflection->setValue($author, 'John Doe'); | ||
$reflection->setValue($entity, $value); | ||
|
||
self::assertSame('John Doe', $wrappedReflection->getValue($author)); | ||
self::assertSame('John Doe', $reflection->getValue($author)); | ||
self::assertSame($value, $wrappedReflection->getValue($entity)); | ||
self::assertSame($value, $reflection->getValue($entity)); | ||
|
||
$reflection->setValue($author, 'John Doe'); | ||
$reflection->setValue($entity, $sameValue); | ||
|
||
self::assertSame('John Doe', $wrappedReflection->getValue($author)); | ||
self::assertSame('John Doe', $reflection->getValue($author)); | ||
/* | ||
* Intentionally testing against the initial $value rather than the $sameValue that we just set above one in | ||
* order to catch false positives when dealing with object types | ||
*/ | ||
self::assertSame($value, $wrappedReflection->getValue($entity)); | ||
self::assertSame($value, $reflection->getValue($entity)); | ||
} | ||
|
||
public function testSecondWriteWithDifferentValue(): void | ||
public function sameValueProvider(): Generator | ||
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. Please add a docblock documenting the return type (it should match the signature of |
||
{ | ||
$author = new Author(); | ||
yield 'string' => [ | ||
'entity' => new Author(), | ||
'property' => 'name', | ||
'value' => 'John Doe', | ||
'sameValue' => 'John Doe', | ||
]; | ||
|
||
yield 'uuid' => [ | ||
'entity' => new Library(), | ||
'property' => 'uuid', | ||
'value' => new Uuid('438d5dc3-36c9-410a-88db-7a184856ebb8'), | ||
'sameValue' => new Uuid('438d5dc3-36c9-410a-88db-7a184856ebb8'), | ||
]; | ||
} | ||
|
||
$wrappedReflection = new ReflectionProperty($author, 'name'); | ||
/** | ||
* @dataProvider differentValueProvider | ||
*/ | ||
public function testSecondWriteWithDifferentValue( | ||
object $entity, | ||
string $property, | ||
mixed $value, | ||
mixed $differentValue, | ||
string $expectedExceptionMessage | ||
): void { | ||
$wrappedReflection = new ReflectionProperty($entity, $property); | ||
$reflection = new ReflectionReadonlyProperty($wrappedReflection); | ||
|
||
$reflection->setValue($author, 'John Doe'); | ||
$reflection->setValue($entity, $value); | ||
|
||
$this->expectException(LogicException::class); | ||
$this->expectExceptionMessage('Attempting to change readonly property Doctrine\Tests\Models\ReadonlyProperties\Author::$name.'); | ||
$reflection->setValue($author, 'Jane Doe'); | ||
$this->expectExceptionMessage($expectedExceptionMessage); | ||
$reflection->setValue($entity, $differentValue); | ||
} | ||
|
||
public function differentValueProvider(): Generator | ||
{ | ||
yield 'string' => [ | ||
'entity' => new Author(), | ||
'property' => 'name', | ||
'value' => 'John Doe', | ||
'differentValue' => 'Jane Doe', | ||
'expectedExceptionMessage' => 'Attempting to change readonly property Doctrine\Tests\Models\ReadonlyProperties\Author::$name.', | ||
]; | ||
|
||
yield 'uuid' => [ | ||
'entity' => new Library(), | ||
'property' => 'uuid', | ||
'value' => new Uuid('438d5dc3-36c9-410a-88db-7a184856ebb8'), | ||
'differentValue' => new Uuid('5d5049ee-01fd-4b66-9f82-9f637fff6a7d'), | ||
'expectedExceptionMessage' => 'Attempting to change readonly property Doctrine\Tests\Models\ReadonlyProperties\Library::$uuid.', | ||
]; | ||
} | ||
|
||
public function testNonReadonlyPropertiesAreForbidden(): void | ||
|
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.
Another way to do it would be using a loose comparison, although I'm not sure if there are any performance implications. I'd like to know if you've considered it though.
orm/src/UnitOfWork.php
Lines 2319 to 2320 in 982d606
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.
I didn't try it indeed. I'm not a big fan of loose comparison, which often leads to unexpected consequences in PHP.
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.
Ok, I'm undecided myself. Let's see what other maintainers have to say.
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 looks extremely expensive to call serialize
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.
"extremely", really?