Integrate existing Elasticsearch support to Symfony, API Platform #190
Conversation
Makefile
Outdated
| @@ -1,7 +1,7 @@ | |||
| dist: test | |||
|
|
|||
| test: es-up | |||
| vendor/bin/simple-phpunit | |||
There was a problem hiding this comment.
This is committed in error, ignore, I'll undo.
dkarlovi
force-pushed
the
elasticsearch-integration
branch
from
8a5d143 to
cf06fcd
Compare
composer.json
Outdated
| @@ -1,5 +1,6 @@ | |||
| { | |||
| "homepage": "https://rollerworks.github.io/", | |||
| "name": "rollerworks/search", | |||
There was a problem hiding this comment.
Temporary, just to make it installable from my fork.
dkarlovi
force-pushed
the
elasticsearch-integration
branch
from
cf06fcd to
f0757f5
Compare
|
@sstok can we talk about this one? What do you feel is required to get this merged? |
dkarlovi
force-pushed
the
elasticsearch-integration
branch
from
f0757f5 to
0480842
Compare
| $configuration = (array) $configuration['elasticsearch']; | ||
| ArrayKeysValidator::assertOnlyKeys($configuration, ['mappings', 'identifiers_normalizer'], $configPath); | ||
|
|
||
| // this snippet looks weird, factory should create the proper instance on its own |
There was a problem hiding this comment.
Can you explain this one? If needs improving please open an issue afterwards so we don't loose it.
There was a problem hiding this comment.
Basically, I don't think the factory should leave it up to it's user to compose the cached version, you should just request an instance and get it, cache-decorated or not.
| @@ -38,7 +39,7 @@ public function getConfigTreeBuilder() | |||
| ->children() | |||
| ->arrayNode('processor') | |||
| ->info('SearchProcessor configuration') | |||
| ->{class_exists(Psr7SearchProcessor::class) && interface_exists(HttpFoundationFactoryInterface::class) ? 'canBeDisabled' : 'canBeEnabled'}() | |||
| ->{class_exists(Psr7SearchProcessor::class) && class_exists(HttpFoundationFactoryInterface::class) ? 'canBeDisabled' : 'canBeEnabled'}() | |||
There was a problem hiding this comment.
This needs to be reverted. Properly a bad rebase. I have no idea why this even worked in the first place 😳
There was a problem hiding this comment.
Yeah, I did not do that, might have been CS fixer.
|
|
||
| <services> | ||
| <service id="rollerworks_search.elasticsearch.factory" class="Rollerworks\Component\Search\Elasticsearch\ElasticsearchFactory" public="true"> | ||
| <argument type="service" id="rollerworks_search.elasticsearch.factory.cache" on-invalid="null" /> |
There was a problem hiding this comment.
rollerworks_search.elasticsearch.factory.cache -> rollerworks_search.elasticsearch.cache
There was a problem hiding this comment.
Nice catch, thanks.
dkarlovi
force-pushed
the
elasticsearch-integration
branch
2 times, most recently
from
4c4b529 to
1cd8235
Compare
|
I'm adding tests etc, so don't merge if you were going to. 👍 |
|
Thanks for the warning 😄 |
dkarlovi
force-pushed
the
elasticsearch-integration
branch
from
1cd8235 to
34d84ea
Compare
|
BTW sorry I keep squashing, just realized you might be reading individual commits which makes it awkward. I'll just add WIP commits and squash before merge. Also, we should try to use the new build process ASAP, fixing CS Fixer and PHPStan issues would allow us to find and fix new ones in new commits. |
| */ | ||
| public function process(ContainerBuilder $container) | ||
| { | ||
| if (!$container->hasDefinition('fos_elastica.client.default')) { |
There was a problem hiding this comment.
@sstok can you help me out here? For some reason, this will always return false (as if FOS Elastica is not loaded, even though it's registered) and I'm getting
1) Rollerworks\Bundle\SearchBundle\Tests\Functional\ApiPlatformTest::testEmptySearchCodeIsValid
Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "rollerworks_search.elasticsearch.client". Did you mean this: "rollerworks_search.elasticsearch.factory"?
There was a problem hiding this comment.
Maybe try has() if it's an alias it might not be detected 🤔
There was a problem hiding this comment.
No, nothing related to Elastica is found for some reason. I'm trying to debug this and hitting a wall, can't figure it out. :(
There was a problem hiding this comment.
Bundle is definetly registered as I'm seeing compiler passes from it further down the list if I dump the compiler passes from the entire app in the compiler. I've also tried all the PassConfig types, it should work without any of that but it doesn't.
| @@ -95,14 +96,4 @@ public function getCacheDir() | |||
|
|
|||
| return rtrim($tmpDir, '/\\').'/rollerworks-search-'.sha1(__DIR__).'/'.substr(sha1($this->config), 0, 6); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
@sstok these were to blame, thanks for that. 😭 It took me forever to figure it out. Problem was, for some bizzare reason, the instance of $container FOS Elastica and Search bundle were getting was not the same instance.
There was a problem hiding this comment.
Which is exactly the reason why it worked in my app, but not in the functional test app. (┛◉Д◉)┛彡┻━┻
There was a problem hiding this comment.
😐 properly not needed anymore then 😄 sorry ¯_(ツ)_/¯
There was a problem hiding this comment.
The upside is I'm now thoroughly familiar with the Symfony Container compile process. :D
dkarlovi
force-pushed
the
elasticsearch-integration
branch
from
ec79de2 to
308f9f9
Compare
|
@sstok if this looks OK to you, I'll rebase and squash so you can merge. |
|
Also, this was my first time using Prophecy (not a fan) so feel free to comment if I missed any important points. |
dkarlovi
force-pushed
the
elasticsearch-integration
branch
from
c142b84 to
a9969bc
Compare
dkarlovi
force-pushed
the
elasticsearch-integration
branch
from
a9969bc to
00ee5b3
Compare
dkarlovi
changed the title
|
@sstok seeing you're away end of week, could we see this gets merged before then? |
|
Thank you @dkarlovi |
|
\o/ :+1: |
Porting rollerworks/search-api-platform#8, rollerworks/RollerworksSearchBundle#37 to new mono-repo.
Closes #179.