Fix change detection for non-scalar readonly properties #9998
Conversation
This fixes a bug where non-scalar readonly properties (like Uuid) will cause an "Attempting to change readonly property..." error despite the underlying value not actually changing. The implementation isn't for a specific object type like the ones provided by symfony/uid (which is used in the new test cases), but instead relies on the common convention of value objects defining an equals() method for correct equality checking.
|
This has no relation with readonly properties. Other types doont allow object compare via equals() either. |
| @@ -47,6 +47,7 @@ | |||
| "psr/log": "^1 || ^2 || ^3", | |||
| "squizlabs/php_codesniffer": "3.7.1", | |||
| "symfony/cache": "^4.4 || ^5.4 || ^6.0", | |||
| "symfony/uid": "^4.4 || ^5.4 || ^6.0", | |||
There was a problem hiding this comment.
I'm not keen on adding that dependency, tbh. We should be able to reproduce the problem without it.
There was a problem hiding this comment.
That's fair. I only added it because it's technically supposed to be supported out of the box (but that might be a symfony concern, so it makes sense to not add it in doctrine)
| { | ||
| $objectValue = parent::getValue($object); | ||
|
|
||
| if (is_object($objectValue) && method_exists($objectValue, 'equals')) { |
There was a problem hiding this comment.
I'm not comfortable with applying duck typing here. I understand the problem that you're trying to solve, but I'd rather not compare incomparable values than to guess how to compare them.
There was a problem hiding this comment.
I am not fan of duck typing here either, but other than adding explicit type checks (and thus requiring a non-dev dependency for every supported type), I am not sure how else the problem could be fixed
There was a problem hiding this comment.
I mean, we could just assume equality if we cannot compare the values. I would prefer that solution over calling a method that just looks like it does the right thing…
There was a problem hiding this comment.
Would exposing a configuration hook for providing custom equality checks be acceptable here?
There was a problem hiding this comment.
The above question still stands. I am happy to work towards another fix, but would like a confirmation that the approach is acceptable before I do so
|
@beberlei the failure I am attempting to fix is specific to readonly property support. The one of the added test cases (same) fails without the change and the error that comes out of it is blatantly incorrect |
|
Not sure how I missed this related issue: #9505 |
|
This has been stale for quite some time now. doctrine:
orm:
type_comparison:
# Map of methods that check for equality
default: 'equals' # default for every other object type or even just fallback to that !== comarison
Symfony\Uid\Uuid: 'compare'This way, the If that sounds like a good plan, please let us know. So we can get this feature integrated into doctrine. |
|
I think, we can close this PR because of implementation issues. I'll happily review a new attempt that works without additional dependencies or duck typing. Some kind of discovery mechanism for comparator functions as suggested could be the solution, but I'm not sure about that. |
|
An alternative was suggested, but never approved/declined 🤷♂️ |
|
Closing it is good and all. But we gave some suggestions and got no feedback for it. The current behavior is clearly a bug and should be fixed. I think @nCrazed and I both would be happy to contribute, but we would kindly appreciate any kind of feedback to the proposed suggestion(s). |
No description provided.