Upgrade to Algolia client v2 #244
Conversation
| @@ -15,7 +15,7 @@ public function update($searchableEntities) | |||
| $batch = $this->doUpdate($searchableEntities); | |||
|
|
|||
| foreach ($batch as $indexName => $response) { | |||
| $this->algolia->initIndex($indexName)->waitTask($response['taskID']); | |||
| $this->algolia->waitTask($indexName, $response['taskID']); | |||
There was a problem hiding this comment.
What's the rationale of not using $this->algolia->index($indexName)->waitTask($response['taskID']). It feels weird to have this waitTask method taking the index name as 1st parameter.
There was a problem hiding this comment.
We recently exposed waitTask in the client for 2 main reasons:
- To make it easier to work with multiple index endpoint (which are on the client)
- To make it easier to work with other API, like analytics that returns a index name and a task ID.
More details: algolia/algoliasearch-client-php#410
I thought it would best to avoid instanciating an index "just" for a waitTask but I didn't benchmark anything, I guess the performance gain would be negligable. It just felt easier.
Note that internally, Client::waitTask will instanciate an Index until we find a better solution to not duplicate the wait logic, which I don't think is possible with PHP 5.3 because:
- Traits are not available (introduce in 5.4)
thisis not available in closures (introduce in 5.4)
There was a problem hiding this comment.
And thanks for taking the time to review 🙏 Feedback is greatly appreciated!
| @@ -15,7 +14,9 @@ public function boot() | |||
| { | |||
| parent::boot(); | |||
|
|
|||
| Version::addSuffixUserAgentSegment('Symfony Search Bundle', self::VERSION); | |||
| Version::addSuffixUserAgentSegment('Symfony', SfKernel::VERSION); | |||
| if (class_exists('Algolia\AlgoliaSearch\Support\Config')) { | |||
There was a problem hiding this comment.
I'm not sure what the PHP requirement of this package is (there's none in composer.json which is bad enough), but starting with 5.5.0 there's the ::class constant that you should use instead of magic strings. This allows IDEs to find code like this when looking for usages of a class.
There was a problem hiding this comment.
I didn't specify it because I'm maintaining the same as Symfony 3.4, which requires ^5.5.9. I will update the PR. For the client, I'm maintaing 5.3 so I sort of lost the habit of ::class ^^
There was a problem hiding this comment.
That makes sense. I would still recommend applying the PHP requirement explicitly in composer.json to be on the safe side to avoid relying on other libraries' constraints.
| <argument key="$applicationID">%env(ALGOLIA_APP_ID)%</argument> | ||
| <service id="algolia_client" class="Algolia\AlgoliaSearch\Client" public="false"> | ||
| <factory class="Algolia\AlgoliaSearch\Client" method="create"/> | ||
| <argument key="$appId ">%env(ALGOLIA_APP_ID)%</argument> |
There was a problem hiding this comment.
There are extra spaces behind the key here.
|
@julienbourdeau: Are you maybe willing to rebase this and finish fully? The 1.x has a "slight" performance problem:
EditAfter some testing it looks like calling the
@nunomaduro I think upgrading to v2 SDK should be prioritized. If not maybe we can add an option to v1 SDK (even if EOL now) to let user specify the timeout with default value of |
|
Having the same issue as @kiler129 ... @julienbourdeau : Any update on this one ? |
|
@neyric Thanks for asking, we are going to develop |
|
I'm closing this issue due to a lack of activity. I will take this pull request in consideration once I start work on the migration of the api client |
What was changed
Update to version Version 2 of the API client. The version isn't out yet, this PR is here to check how hard it is.
See more details on discourse: https://discourse.algolia.com/t/introducing-php-api-client-v2/5635