Support dot syntax when preparing nested query values

[alcaeus]

Q	A
Type	bug
BC Break	no
Fixed issues	Fixes #2825, #2550, #2531, #2212

Summary

This fixes an issue when preparing elements for the query builder. The issue was that when $prepareValue in DocumentPersister::prepareQueryElement was true, the method would prepare identifier values and such, but not convert actual field values. Instead, this was done later in prepareQueryOrNewObj. However, that class then didn't contain the same dot logic contained in prepareQueryElement, leading to incomplete preparation of values.

This fix

[Copilot]

Pull Request Overview

This PR fixes a bug in nested query value preparation by ensuring that dot syntax field names are properly converted to database values during query preparation. The issue was that prepareQueryElement would handle nested field name mapping but skip database value conversion when $prepareValue was true, leaving this conversion to prepareQueryOrNewObj which lacked the dot syntax logic.

• Moved database value conversion logic directly into prepareQueryElement to handle nested fields properly
• Simplified prepareQueryOrNewObj to use array functions instead of manual loop for combining prepared elements
• Added comprehensive test coverage for nested field preparation scenarios

Reviewed Changes

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

File	Description
lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php	Fixed nested query value preparation by moving database conversion into prepareQueryElement and updating parameter handling
tests/Doctrine/ODM/MongoDB/Tests/Query/BuilderTest.php	Added test case and supporting document classes to verify nested field preparation works correctly

---

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

[GromNaN]

Perfect!

[alcaeus]

Note: there are some cases that still aren't covered by this pull requests. I'm currently producing test cases for these cases to fix them properly.

[alcaeus]

These tests needed changing, as the field name translation only worked by accident. The nested property in the association document has no targetDocument, so we can't reliably know how to handle field names. The old logic reused the current ClassMetadata instance in these cases, which just happened to be the correct one in the tests, but might as well be incorrect. For example, this is the reason why an id property in an embedded document mapped without a targetDocument was renamed to _id in queries, even when it shouldn't have to.

Instead of changing the test documents to have a targetDocument, I figured it would be more prudent to highlight this change here. If reviewers feel that this change is a bit too risky for a patch release or even a minor version, please speak now or forever hold your peace.

[GromNaN]

What is a upgrade path if someone hits this breaking change?

[alcaeus]

That's a good question, and the answer isn't straightforward. First, I should clarify that the instances where the ODM did field name translation, it likely didn't use the correct mappings, unless somebody had an A -> X -> A relationship. In all other cases, mappings wouldn't line up.

The problem only appeared in embeds without a target document, so the obvious solution would be to set a target document. If there is no common ancestor to the possible embedded documents, users would never be able to rely on mappings for queries anyways and would have to use field name translation themselves, i.e. write the field names as they appear in the database.

[GromNaN]

OK. I propose to target the minor version 2.13 instead of bugfix 2.12.2 and call it a feature.

[jmikola]

I concur with targeting 2.13 instead of 2.12.2.

The problem only appeared in embeds without a target document

Is there a legitimate use case for using EmbedMany or EmbedOne without a targetDocument mapping? When persisting to BSON, I assume ODM can use the class name to lookup metadata, but what happens when hydrating back from BSON? There'd be no hint for class metadata.

Perhaps this is a case where we should emit a warning/notice advising users to always specify targetDocument (even if it's a mapped superclass).

[alcaeus]

No use case that I know of, but I'd have to check if specifying a discriminatorMap in the mapping without a targetDocument will work.

[Copilot]

Pull Request Overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

---

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

[GromNaN]

I find the test easier to read with array comparison, but that we need an intermediate comparator to assert the same type and value. As $objectId == $oidString

        self::assertSameValueAndtype(['embedded.embedded.test' => $objectId], $filter);
        self::assertSameValueAndtype(['$set' => ['embedded.embedded.embedded' => ['test' => $objectId]]], $newObj);