Convert the value of the "version" field using its known type#2813
Convert the value of the "version" field using its known type#2813GromNaN merged 1 commit intodoctrine:2.12.xfrom
Conversation
| $this->class->reflFields[$this->class->versionField]->setValue($document, $nextVersion); | ||
| } | ||
|
|
||
| $data[$versionMapping['name']] = $type->convertPHPToDatabaseValue($nextVersion); |
There was a problem hiding this comment.
This is a static method called on the instance. So it was possible to change the behavior for a custom type by overriding the method, but that's not how it's supposed to be used.
In fact, this method should be final.
There was a problem hiding this comment.
Are you suggesting that the convertPHPToDatabaseValue() should be final? If so, I agree and we should probably track that with a separate ticket for the next major version.
There was a problem hiding this comment.
Yes, that's the idea. But a better approach would be to extract the static methods into a TypeRegistry, similar to DBAL. #2818
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where custom version field types were not being properly converted to database values. Instead of guessing the field type, the code now uses the configured type for the version field to ensure proper conversion.
- Replace static
Type::convertPHPToDatabaseValue()calls with instance$type->convertToDatabaseValue() - Add comprehensive test coverage for custom version field types
- Ensure custom types implementing
Versionablework correctly with version field operations
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php | Updates version field conversion to use the configured type instead of static type guessing |
| tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH2789Test.php | Adds test case demonstrating custom version field type functionality with Binary storage |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
jmikola
left a comment
jmikola
left a comment
There was a problem hiding this comment.
LGTM, but two small suggestions.
| $this->class->reflFields[$this->class->versionField]->setValue($document, $nextVersion); | ||
| } | ||
|
|
||
| $data[$versionMapping['name']] = $type->convertPHPToDatabaseValue($nextVersion); |
There was a problem hiding this comment.
Are you suggesting that the convertPHPToDatabaseValue() should be final? If so, I agree and we should probably track that with a separate ticket for the next major version.
Summary
Instead of guessing the type of the version field value in order to convert the value into a MongoDB compatible value, we can use the type that is configured for this field. Otherwise the detection of the field type from the value type fails for custom types.