Refactor the Search Engine & Filter Builder #17
Conversation
jakubtobiasz
commented
Aug 23, 2024
jakubtobiasz
commented
- Filter Builder has been split into smaller, specific filter focused services
- Search Engine has been refactored to use Multi Search to improve returned facet distribution
composer.json
Outdated
| @@ -88,6 +88,7 @@ | |||
| }, | |||
| "autoload-dev": { | |||
| "psr-4": { | |||
| "TestApp\\": "tests/Application/src/", | |||
There was a problem hiding this comment.
What's the reasoning behind a new namespace?
There was a problem hiding this comment.
I've created a src inside the test app, as I wanted to skip putting it directly under the tests/Application. The namespace will look CurrentNamespace/src/FilterBuilder so I decided to create a shorter version, as it doesn't impact the project itself.
There was a problem hiding this comment.
so, maybe, to have it consistent with other namespaces name it Setono\\SyliusMeilisearchPlugin\\TestApp\\? I know it's long 💃 but who cares when we have autocomplete 🚀
jakubtobiasz
force-pushed
the
multisearch
branch
from
633319d to
e6b622b
Compare
| $brandQuery = []; | ||
| /** @var string $size */ | ||
| foreach ($facets['brand'] as $size) { | ||
| $brandQuery[] = sprintf('brand = "%s"', $size); | ||
| } | ||
|
|
||
| return '(' . implode(' OR ', $brandQuery) . ')'; |
There was a problem hiding this comment.
I suppose we can simplify the implementations of these anonymous classes, they're not relevant in the scope of this test
src/Engine/SearchEngine.php
Outdated
There was a problem hiding this comment.
I feel like this class is much less readable after the refactor than before, which should not be the case 😅 Let's maybe introduce another layer of abstraction, so this class would only gather the query and parameters, pass them to query builder (main query builder and subsearch query builders) and then execute the call on Meilisearch API?
Let's start the discussion 🖖
| @@ -6,5 +6,7 @@ | |||
|
|
|||
| interface FilterBuilderInterface | |||
| { | |||
| public function build(array $parameters): array; | |||
| public function build(array $facets): string|array; | |||
There was a problem hiding this comment.
We could potentially unify the return type, to always return array (no matter if with one or multiple elements)
jakubtobiasz
force-pushed
the
multisearch
branch
from
e6b622b to
ba1c25c
Compare
jakubtobiasz
force-pushed
the
multisearch
branch
from
ba1c25c to
01a317d
Compare
|
Closing in favor of #22 - thank you @jakubtobiasz 🫡 |
