Fix entity mapping not working correctly #33
Conversation
|
Thanks @kermitsxb for the PR. |
|
@emulienfou I have written a small test that assert that the searchService returns something. I took the index data part from the CommandsTest |
There was a problem hiding this comment.
Thanks a lot for this PR @kermitsxb!! And thanks for the test addition!
One question to be sure that I understand: does this error appear only when the hits array was empty?
src/Services/MeiliSearchService.php
Outdated
| @@ -215,6 +215,11 @@ public function search( | |||
| $ids = $this->engine->search($query, $this->searchableAs($className), $requestOptions); | |||
| $results = []; | |||
|
|
|||
| // Check if the engine return results in "hits" key | |||
There was a problem hiding this comment.
| // Check if the engine return results in "hits" key | |
| // Check if the engine returns results in "hits" key |
|
You're welcome @curquiza, thank you all to provide this amazing bundle. It looks like the meilisearch-http returns an array with the results of the search having an "hits" key (cf : https://github.com/meilisearch/MeiliSearch/blob/dec0e2545d5878a4b316f3f5549373f091dc8328/meilisearch-http/src/helpers/meilisearch.rs on line 239, the array is built before). This has been added 14 months ago. The Symfony function I've worked on didn't iterate on that array result but on the returned parent array. In order to keep the function working with people using older version I just set the array we iterate on later with the correct array if the key "hits" exists. |
Until MeiliSearch is stable, we don't want to add maintenance for the old version of MeiliSearch in the integration tools code bases. Plus, 14 months ago is a really long time for MeiliSearch, there were no integration plugins and this one didn't even exist. So, I don't really know where the bug comes from because I really doubt the users in the issue use one of the oldest versions of MeiliSearch -> they would have a LOT of other problems. |
|
I don't know where the bug comes from since I've started using the bundle and it wouldn't work properly. The only thing I know is that the result are not passed the good way into the function and the fix I've made make it work. |
|
Hi @kermitsxb it's probably a mistake from me and forgot to add unit testing for this part of code and tested it correctly. So instead of doing: if (isset($ids['hits'])) {
$ids = $ids['hits'];
}Can you directly update the foreach loop: foreach ($ids as $objectID)And I think we will need to check before entering in the |
|
@emulienfou that's strange that the issue was not encounter before 🤔 I'll correct that tomorrow |
|
Thanks to both of you guys for your involvement! |
|
Hello @kermitsxb! Do you have some issue with this PR? Is there something we can do for helping? |
|
Hello @curquiza ! Excuse me for the delay, I was on holiday and just came back to work yesterday, I'm gonna fixed this issue this week and update the PR ;) |
|
Take your time @kermitsxb! It was just to be informed about the status of this PR 🙂 |
…its" key found in results
|
Hi @curquiza @emulienfou, I've updated the PR with your recommendations. |
There was a problem hiding this comment.
I would rather the typo I have already noticed (https://github.com/meilisearch/meilisearch-symfony/pull/33/files#r542596071) before merging, otherwise, it seems ok 🙂
Is it ok for your @emulienfou?
curquiza
changed the title
|
Let's merge it because we don't have any feedback of emulienfou who might be busy 🙂 @emulienfou, you still can require changes of course if you see something wrong. bors merge |
Small fix for the bundle to work correctly with last version of meilisearch