fixed prepareQueryElement() "set" strategy issue #485

Merged
merged 6 commits into from Apr 1, 2013

2 participants

@kriswallsmith
Doctrine member

After this change the set strategy allows you to query for a value in a specific embedded document.

Before

$userRepo->findOneBy(array('profiles.facebook.id' => 1234));
-->
db.users.findOne({ "profiles.facebook": 1234 }) // what happened to ".id"?

After

$userRepo->findOneBy(array('profiles.facebook.id' => 1234));
-->
db.users.findOne({ "profiles.facebook.id": 1234 })
@jmikola jmikola and 1 other commented on an outdated diff Jan 31, 2013
...Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php
@@ -909,7 +909,11 @@ private function prepareQueryElement(&$fieldName, $value = null, $metadata = nul
$mapping = $metadata->fieldMappings[$e[0]];
$e[0] = $mapping['name'];
@jmikola
Doctrine member
jmikola added a note Jan 31, 2013

Do you have any idea why only the mapping name for $e[0] is resolved? The other path components seem to be used as-is.

@kriswallsmith
Doctrine member

The rest of the parts are resolved differently depending on multiple factors I don't totally understand…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmikola jmikola and 1 other commented on an outdated diff Jan 31, 2013
...Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php
@@ -909,7 +909,11 @@ private function prepareQueryElement(&$fieldName, $value = null, $metadata = nul
$mapping = $metadata->fieldMappings[$e[0]];
$e[0] = $mapping['name'];
$fieldName = $e[0] . '.' .$e[1];
- if ($e[1] != '$') {
+ if ($mapping['strategy'] === 'set' && isset($e[2])) {
@jmikola
Doctrine member
jmikola added a note Jan 31, 2013

Is there any case where the path may be longer than X.Y.Z, or would larger paths be prepared separately from this method?

@kriswallsmith
Doctrine member

Yes, there could very well be a nested scenario.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmikola
Doctrine member

Just updated the PR with some additional refactoring, and I extended the test case to go one level deeper.

@kriswallsmith and @jwage: I recall one of you saying this was a quick fix that didn't address the real problem. Do either of you have a failing test case I could add in here?

The refactoring so far doesn't really change the behavior of prepareQueryElement() at all. I also still need to revise the innards of if (isset($mapping['targetDocument'])).

@jmikola jmikola merged commit c1a90e7 into master Apr 1, 2013

1 check passed

Details default The Travis build passed
@jmikola
Doctrine member

@kriswallsmith: Please delete the fix/set-strategy-defect branch when you get a chance. I just want to make sure you're not pointing a submodule at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment