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

check readonly property via serialization #11617

Open
wants to merge 3 commits into
base: 2.20.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
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
3 changes: 2 additions & 1 deletion src/Mapping/ReflectionReadonlyProperty.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
use function func_get_args;
use function func_num_args;
use function is_object;
use function serialize;
use function sprintf;

/** @internal */
Expand Down Expand Up @@ -42,7 +43,7 @@ public function setValue(mixed $objectOrValue, mixed $value = null): void

assert(is_object($objectOrValue));

if (parent::getValue($objectOrValue) !== $value) {
if (serialize(parent::getValue($objectOrValue)) !== serialize($value)) {
Copy link
Member

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

// phpcs:ignore SlevomatCodingStandard.Operators.DisallowEqualOperators.DisallowedEqualOperator
if ($managedCopyVersion == $entityVersion) {

Copy link
Author

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.

Copy link
Member

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.

Copy link
Member

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

Copy link
Author

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

"extremely", really?

throw new LogicException(sprintf('Attempting to change readonly property %s::$%s.', $this->class, $this->name));
}
}
Expand Down
20 changes: 20 additions & 0 deletions tests/Tests/Models/ReadonlyProperties/Library.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
<?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\Tests\Models\ValueObjects\Uuid;

#[Entity]
class Library
{
#[Column]
#[Id]
#[GeneratedValue(strategy: 'NONE')]
public readonly Uuid $uuid;
}
16 changes: 16 additions & 0 deletions tests/Tests/Models/ValueObjects/Uuid.php
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;
}
}
88 changes: 72 additions & 16 deletions tests/Tests/ORM/Mapping/ReflectionReadonlyPropertyTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -17,36 +20,89 @@
*/
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
/**
Copy link
Member

Choose a reason for hiding this comment

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

This should use array{type of arg1, type of arg2, type of arg3...}

* @return Generator<string, array{entity: object, property: string, value: string|object, sameValue: string|object}>
Copy link
Member

Choose a reason for hiding this comment

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

Oh wow didn't know we could use named arguments here 💡

*/
public static function sameValueProvider(): Generator
{
$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);
}

/**
* @return Generator<string, array{entity: object, property: string, value: string|object, sameValue: string|object, expectedExceptionMessage: string}>
*/
public static 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
Expand Down