Improve detection of the type from a PHP variable #2814

GromNaN · 2025-09-01T16:24:25Z

Q	A
Type	feature
BC Break	no
Fixed issues	Related to #2789

Summary

The method getTypeFromPHPVariable is used only by convertPHPToDatabaseValue. Which is used to process expressions in the aggregation builder.

With this new feature, we try to find the type for object class name; assuming custom types are registered using the FQCN of the objects they store.

docs/en/reference/custom-mapping-types.rst

jmikola · 2025-09-02T18:02:36Z

docs/en/reference/custom-mapping-types.rst

+                sprintf(
+                    'Could not convert database value "%s" from "%s" to %s',
+                    $value,
+                    get_debug_type($value),


Are use function lines necessary? I'm OK to omit them, but just want to ensure you've considered the question.

The use function are not necessary for global functions. That's a CS preference to use them. I omit them for brevity in the example.

docs/en/reference/custom-mapping-types.rst

jmikola · 2025-09-02T18:05:38Z

docs/en/reference/custom-mapping-types.rst

+
+This pattern can be adapted for any custom class—just implement the conversion logic for your value object.
+
+.. |FQCN| raw:: html <abbr title="Fully-Qualified Class Name">FQCN</abbr>


@GromNaN: this looks like it may be a false positive, since this is some very niche RST syntax

docs/en/reference/custom-mapping-types.rst

lib/Doctrine/ODM/MongoDB/Types/InvalidTypeException.php

lib/Doctrine/ODM/MongoDB/Types/Type.php

GromNaN

Thanks for the review. I added the type detection for DTO class name in property type.

docs/en/reference/custom-mapping-types.rst

GromNaN · 2025-09-03T12:57:19Z

docs/en/reference/custom-mapping-types.rst

+
+This pattern can be adapted for any custom class—just implement the conversion logic for your value object.
+
+.. |FQCN| raw:: html <abbr title="Fully-Qualified Class Name">FQCN</abbr>


I revert changes on this line. I'm not sure how this will be formatted.

docs/en/reference/custom-mapping-types.rst

lib/Doctrine/ODM/MongoDB/Types/Type.php

GromNaN · 2025-09-03T16:02:46Z

docs/en/reference/custom-mapping-types.rst

+By using the |FQCN| of the value object class as the type name, the type is
+automatically used when encountering a property of that class. This means you
+can omit the ``type`` option when defining the field mapping::
+
+.. code-block:: php
+
+    #[Field]
+    public Ramsey\Uuid\Uuid $id;


This is a new feature that can compete with embedded documents: adding custom logic to convert a value object into BSON.

Maybe the example should be with a custom class instead, since UUID are new supported using symfony/uid #2826

GromNaN · 2025-09-03T16:27:30Z

lib/Doctrine/ODM/MongoDB/Types/InvalidTypeException.php

We don't have a global namespace for exception like it is generally done in libraries. Existing exception names are:

Doctrine\ODM\MongoDB\MongoDBException

Doctrine\ODM\MongoDB\ConfigurationException

Doctrine\ODM\MongoDB\DocumentNotFoundException

Doctrine\ODM\MongoDB\Mapping\MappingException

Doctrine\ODM\MongoDB\Hydrator\HydratorException

Doctrine\ODM\MongoDB\LockException

Copilot

Pull Request Overview

This PR improves type detection for PHP variables in the MongoDB ODM by enhancing the getTypeFromPHPVariable method to automatically detect custom types based on object class names. The change allows custom types registered using their FQCN to be automatically selected when processing aggregation expressions.

Key changes:

Enhanced type detection to check if an object's class name corresponds to a registered custom type
Improved exception handling by introducing a dedicated InvalidTypeException
Added automatic type inference for object properties when custom types are registered with class FQCNs

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`lib/Doctrine/ODM/MongoDB/Types/Type.php`	Enhanced `getTypeFromPHPVariable` method to detect custom types by class name and improved type detection logic
`lib/Doctrine/ODM/MongoDB/Types/InvalidTypeException.php`	New dedicated exception class for invalid type scenarios
`lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadata.php`	Added automatic type detection for properties with registered custom types
`tests/Doctrine/ODM/MongoDB/Tests/Types/TypeTest.php`	Added comprehensive test coverage for the new type detection functionality
`tests/Doctrine/ODM/MongoDB/Tests/Functional/CustomTypeTest.php`	Added tests demonstrating custom type detection and proper test setup/teardown
`tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH2789Test.php`	Improved test isolation by properly managing type registry state
`docs/en/reference/custom-mapping-types.rst`	Updated documentation with UUID custom type example and FQCN usage patterns

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

tests/Doctrine/ODM/MongoDB/Tests/Types/TypeTest.php

tests/Doctrine/ODM/MongoDB/Tests/Functional/CustomTypeTest.php

lib/Doctrine/ODM/MongoDB/Types/Type.php

docs/en/reference/custom-mapping-types.rst

jmikola · 2025-09-11T15:09:07Z

docs/en/reference/custom-mapping-types.rst

+                return $value;
+            }
+
+            if ($value instanceof Binary) {


Would it make sense to restrict this to a MongoDB\BSON\Binary where getType() returns either TYPE_OLD_UUID or TYPE_UUID? As-is, it's not clear what would happen if you encountered binary data from the server with a different sub-type.

I'll note that convertToDatabaseValue() yields a Binary object with TYPE_UUID, so perhaps there's no reason to accept TYPE_OLD_UUID here.

If you are intentionally being liberal in what you accept and conservative in what you emit (Robustness Principle) and would like to leave this as-is, that's fine by me. You can optionally add a comment here explaining that design decision for the benefit of readers.

jmikola · 2025-09-11T15:12:48Z

docs/en/reference/custom-mapping-types.rst

+                return null;
+            }
+
+            if ($value instanceof Binary) {


Would you want to ensure you always yield a Binary with sub-type TYPE_UUID? If so, you might need to split this into two separate conditionals.

if ($value instanceof Binary && $value->getType() !== Binary::TYPE_UUID) { // Note: this may throw if the data is not exactly 16 bytes return new Binary($value->getBytes(), Binary::TYPE_UUID); } if ($value instanceof Binary) { return $value; }

I'd rather simplify the example.
https://github.com/doctrine/mongodb-odm/pull/2814/files#r2393846289

lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadata.php

alcaeus · 2025-09-15T11:38:16Z

docs/en/reference/custom-mapping-types.rst


        <field field-name="field" type="date_with_timezone" />

+Custom Type Example: Mapping a UUID Class


Note: I'm currently working on adding a type for Symfony UUIDs as I needed them for a project. That would make this entire chapter obsolete.

The chapter is about mapping a DTO with a custom type. UUID is only an example, we can find an other example.

GromNaN · 2025-10-01T08:34:20Z

docs/en/reference/custom-mapping-types.rst

+                sprintf(
+                    'Could not convert database value "%s" from "%s" to %s',
+                    $value,
+                    get_debug_type($value),


The use function are not necessary for global functions. That's a CS preference to use them. I omit them for brevity in the example.

GromNaN · 2025-10-01T08:35:19Z

docs/en/reference/custom-mapping-types.rst

+            if (null === $value || [] === $value) {
+                return null;
+            }
+
+            if ($value instanceof Binary) {
+                return $value;
+            }


I can simplify the example and remove this edge cases.

Suggested change

if (null === $value || [] === $value) {

return null;

}

if ($value instanceof Binary) {

return $value;

}

if (null === $value) {

return null;

}

Without the $value instanceof Binary check, wouldn't the method then throw InvalidArgumentException for a Binary value? Or are you comfortable assuming that a Binary would never be passed in?

I didn't follow why [] was originally being handled alongside null, so I don't have much thought on that change.

In the example of document class, the property has the ?\Ramsey\Uuid\Uuid type, which ensure the RamseyUuidType::convertToDatabaseValue() method never get anything else.
As soon as the field value is the database is not corrupted, the simple implementation will work perfectly. No need to add support for edge cases that are never encountered.

GromNaN · 2025-10-01T08:35:56Z

docs/en/reference/custom-mapping-types.rst

+                return null;
+            }
+
+            if ($value instanceof Binary) {


I'd rather simplify the example.
https://github.com/doctrine/mongodb-odm/pull/2814/files#r2393846289

GromNaN · 2025-10-01T08:36:53Z

docs/en/reference/custom-mapping-types.rst

+By using the |FQCN| of the value object class as the type name, the type is
+automatically used when encountering a property of that class. This means you
+can omit the ``type`` option when defining the field mapping::
+
+.. code-block:: php
+
+    #[Field]
+    public Ramsey\Uuid\Uuid $id;


Maybe the example should be with a custom class instead, since UUID are new supported using symfony/uid #2826

GromNaN added the Feature label Sep 1, 2025

GromNaN commented Sep 1, 2025

View reviewed changes

docs/en/reference/custom-mapping-types.rst Outdated Show resolved Hide resolved

GromNaN requested a review from Copilot September 1, 2025 16:26

This comment was marked as outdated.

Sign in to view

GromNaN mentioned this pull request Sep 1, 2025

Unable to use #[Version] with custom type #2789

Closed

GromNaN requested review from jmikola and paulinevos September 2, 2025 14:13

paulinevos reviewed Sep 2, 2025

View reviewed changes

docs/en/reference/custom-mapping-types.rst Show resolved Hide resolved

docs/en/reference/custom-mapping-types.rst Outdated Show resolved Hide resolved

docs/en/reference/custom-mapping-types.rst Outdated Show resolved Hide resolved

jmikola reviewed Sep 2, 2025

View reviewed changes

GromNaN force-pushed the type-from-php-variable branch from d446495 to d106a5a Compare September 3, 2025 15:59

GromNaN commented Sep 3, 2025

View reviewed changes

GromNaN requested review from jmikola, Copilot and paulinevos September 3, 2025 16:51

Copilot AI reviewed Sep 3, 2025

View reviewed changes

GromNaN commented Sep 10, 2025

View reviewed changes

docs/en/reference/custom-mapping-types.rst Outdated Show resolved Hide resolved

jmikola reviewed Sep 11, 2025

View reviewed changes

alcaeus reviewed Sep 15, 2025

View reviewed changes

GromNaN added this to the 2.13.0 milestone Sep 24, 2025

GromNaN commented Oct 1, 2025

View reviewed changes

GromNaN force-pushed the type-from-php-variable branch 2 times, most recently from 30cb393 to d1ea68b Compare October 1, 2025 15:12

Improve detection of the type from a PHP variable

ff5cd77

GromNaN force-pushed the type-from-php-variable branch from d1ea68b to 840f7b8 Compare October 3, 2025 14:27

Simplify type class code to the essential

f0ecc10

GromNaN force-pushed the type-from-php-variable branch from 840f7b8 to f0ecc10 Compare October 3, 2025 14:31


		This pattern can be adapted for any custom class—just implement the conversion logic for your value object.

		.. \|FQCN\| raw:: html <abbr title="Fully-Qualified Class Name">FQCN</abbr>


		<field field-name="field" type="date_with_timezone" />

		Custom Type Example: Mapping a UUID Class

Uh oh!

Improve detection of the type from a PHP variable #2814

Are you sure you want to change the base?

Improve detection of the type from a PHP variable #2814

Uh oh!

Conversation

GromNaN commented Sep 1, 2025

Summary

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GromNaN left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!