check readonly property via serialization #11617
Conversation
garak
force-pushed
the
fix-readonly-property
branch
from
f76e781 to
ed20cf5
Compare
|
Thanks for working on this! Please improve your commit message according to the contributing guide. |
Make possible to use value objects (most notably implementations of UUIDs) as readonly properties
garak
force-pushed
the
fix-readonly-property
branch
from
ed20cf5 to
13ad404
Compare
| use Doctrine\Tests\Models\ValueObjects\Uuid; | ||
|
|
||
| #[Entity] | ||
| #[Table(name: 'library')] |
There was a problem hiding this comment.
Does the table name matter? If not, I'd remove that annotation entirely, let's eliminate unnecessary distractions.
| } | ||
|
|
||
| public function testSecondWriteWithDifferentValue(): void | ||
| public function sameValueProvider(): Generator |
There was a problem hiding this comment.
Please add a docblock documenting the return type (it should match the signature of testSecondWriteWithSameValue()
| @@ -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)) { | |||
There was a problem hiding this comment.
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.
Lines 2319 to 2320 in 982d606
There was a problem hiding this comment.
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.
Ok, I'm undecided myself. Let's see what other maintainers have to say.
There was a problem hiding this comment.
this looks extremely expensive to call serialize
There was a problem hiding this comment.
this looks extremely expensive to call serialize
"extremely", really?
| @@ -43,6 +43,9 @@ public function testSecondWriteWithSameValue(object $entity, string $property, m | |||
| self::assertSame($value, $reflection->getValue($entity)); | |||
| } | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
This should use array{type of arg1, type of arg2, type of arg3...}
|
My take on this issue is still what I've said in #9505 (comment). Implementing guesswork on when two objects should be considered equal is a very slippery slope. I'm against doing that. Serialization is especially tricky because it might have side effects and we cannot make the general assumption that every object is serializable. |
But we already do that, don't we? |
| @@ -44,9 +44,9 @@ public function testSecondWriteWithSameValue(object $entity, string $property, m | |||
| } | |||
|
|
|||
| /** | |||
| * @return Generator<string, array<string, string|object>> | |||
| * @return Generator<string, array{entity: object, property: string, value: string|object, sameValue: string|object}> | |||
There was a problem hiding this comment.
Oh wow didn't know we could use named arguments here 💡
This is a remake of #9998
It should fix #9505