Move ClusterHosts instantiation logic from SearchClient to ClusterHosts

[ostrolucky]

Q	A
Bug fix?	no
New feature?	no
BC breaks?	no
Related Issue	algolia/search-bundle#317
Need Doc update	no

Describe your change

Moved logic for ClusterHosts instantiation to correct place. Will slim down logic in ApiWrapper and thus more easily reuse it in DI. Will aid with algolia/search-bundle#317

What problem is this fixing?

When injectin ApiWrapper into SearchClient manually, user has to replicate ClusterHosts logic in DI configuration. Extracting this logic will allow to reuse it when defining ApiWrapper service.

[julienbourdeau]

Did you need this change or are you cleaning up? Did you find a related issue?

If I remember correctly, we don't want to have cache for Insight or Analytics clients because there is only one host (no retry strategy here).

[julienbourdeau]

I think this __CLASS__ is the reason why I left this logic in the Client. There is the same logic for PlacesClient here: https://github.com/algolia/algoliasearch-client-php/blob/master/src/PlacesClient.php#L40
If we move this here, we'll have to figure another way since CLASS is always going to be ClusterHosts::class.

[ostrolucky]

We need to do that only if we want to reuse this factory method for PlacesClient. If we do, we can simply have new parameter

public static function createWithConfig(PlacesConfig $config, string $cacheKeyPrefix = __CLASS__) {
    ...
    $cacheKey = sprintf('%s-clusterHosts-%s', $cacheKeyPrefix, $config->getAppId());
    ...
}

WDYT?

[ostrolucky]

Did you need this change or are you cleaning up? Did you find a related issue?

Have you checked PR description? Anything more I should clarify there? We need extracted ClusterHosts instantiation logic so bundle can create service for it we can reuse in userspace.

If I remember correctly, we don't want to have cache for Insight or Analytics clients because there is only one host (no retry strategy here).

I didn't change instantiation logic for those clients, so no changes there - no cache is being used there. But good point, as it perhaps signifies we should change the function name, because it's meant to be used by SearchClient, potentially also PlacesClient only?

[chloelbn]

We found another way to let the users test their application without sending datas to Algolia. Thanks for your work!

[julienbourdeau]

Have you checked PR description?
I did not, I went straight to the code 😅

Overall, I'd rather not move this method to ClusterHosts because I dont think it should handle the cache management. Having to add a parameter for the cache key doesnt feel right.

Also, not using the SearchClient::create*() methods should definitely be a corner case, not the norm, so I think it's fine to have to handle the cache logic.

[ostrolucky]

@chloelbn Can you please reopen this? I believe this was closed prematurely. Your NullService doesn't solve difficulties with injecting custom HttpClient and logger. For example, I still need to do that. I want to reuse the logic of createWithConfig, but define my own ApiWrapper service. This is not possible at the moment.

@julienbourdeau No problem, I can leave it in SearchClient if you feel it makes more sense, but in separate method.