Added conversion from PHP values to DB values during query preparation

[petr-buchin]

Q	A
Type	feature/improvement
BC Break	no
Fixed issues	#2103

Summary

This PR adds conversion of PHP values in queries to their DB representations using types functionality, and thus allows to query by php values rather that db values.

[alcaeus]

At a first glance this looks good, thanks! Can you please add some tests for this feature? I'd love to have this in 2.1!

[petr-buchin]

@alcaeus thanks for the review! Ok, I will add tests this week!

Also, could you please take a look at that one test that is failing?

[alcaeus]

Ignore that for now please - I just haven't found the time to introduce that test to the tip of my boot. It just needs a good kicking here and there. I'll try to fix it later today.

[petr-buchin]

@alcaeus I added two simple tests for this feature.

If you want me to cover some other cases with tests, just tell me what cases I should consider.

Thanks!

[alcaeus]

Sorry for the delay in reviewing this.

I think we also need to apply the same transformation to arrays. To make sure these cases are covered, I've suggested additional tests below.

[petr-buchin]

Hi @alcaeus, thanks for the review!

I think we also need to apply the same transformation to arrays
Do you mean we need to convert arrays the same way as we do with a single values?

If yes, how do you want to do this?
From what I see, there is no way to specify item type for a collection mapping (except reference collections), thus there is no way to know the type of an item in an array.

[petr-buchin]

@alcaeus I got your point: I updated tests as you requested and added exception for non-existent types.

I also found and fixed the bug: at the moment ODM user can query by providing target document for reference field like that:

['referenceField' => $targetDocumentValue]

but user cannot query by providing target document with a query operators, like that:

['referenceField' => ['$ne' => $targetDocumentValue]]

For now ODM will try to convert target document value in query using target document's id type.
I fixed this behavior by retrieving id from target document (see changes in DocumentPersister::prepareQueryExpression() method).

[alcaeus]

Sorry for not getting back to you sooner. From what I can tell, you understood what I meant with the array conversion. I have a few more suggestions below, but other than that this is looking good.

[alcaeus]

We can work around this by using a closure for the getReference logic above. Consider this short example:

public function dataProvider(): array
{
    return [[function (DocumentManager $dm) { return $dm->getReference(...); }]];
}

/** @dataProvider dataProvider */
public function testCase(Closure $getReference)
{
    $reference = $getReference($this->dm);
    // ...
}

[petr-buchin]

I've refactored this test using your advice, but I'm not sure that I understood you correctly.
Please look at the code and confirm if that's what you wanted.

The downside of closure is that we don't get $expected and $query as arguments of the test method as in the test above.

[alcaeus]

With the suggestion above, you can drop one array level:

yield 'Direct comparison' => [
    'query' => ...,
    'expected' => ...,
];

The key names for "query" and "expected" aren't necessary since PHPUnit passes the values of the array as arguments to the test. However, they make working with the data provider easier since you don't need to think which array is which and instead can use the parameter names.

[petr-buchin]

Done, but only for the test method that does not use closure for getReference() logic

[petr-buchin]

@alcaeus thanks for the suggestions!
I will fix this later today

[petr-buchin]

@alcaeus sorry about not getting back with fixes sooner.

I did all the fixes as you've requested, but I'm not sure about one single fix: please confirm if fix with getReference() logic, data provider and closure is what you wanted.

Thanks!

[alcaeus]

Thanks @petr-buchin!

[petr-buchin]

@alcaeus thanks for review and for help with this!