Skip to content

Commit

Permalink
fix php client not checking null region
Browse files Browse the repository at this point in the history
  • Loading branch information
shortcuts committed Aug 10, 2022
1 parent 37d4235 commit be6e4f4
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 29 deletions.
19 changes: 2 additions & 17 deletions templates/php/ConfigWithRegion.mustache
Original file line number Diff line number Diff line change
Expand Up @@ -7,23 +7,8 @@ use Algolia\AlgoliaSearch\Exceptions\AlgoliaException;

abstract class ConfigWithRegion extends Configuration
{
public static function create(
$appId,
$apiKey,
$region = null,
$allowedRegions = null
) {
if (
$region !== null &&
$allowedRegions !== null &&
!in_array($region, $allowedRegions, true)
) {
throw new AlgoliaException(
'`region` must be one of the following: ' .
implode(', ', $allowedRegions)
);
}

public static function create($appId, $apiKey, $region = null)
{
$config = [
'appId' => $appId,
'apiKey' => $apiKey,
Expand Down
23 changes: 13 additions & 10 deletions templates/php/api.mustache
Original file line number Diff line number Diff line change
Expand Up @@ -51,18 +51,21 @@ use {{invokerPackage}}\Support\Helpers;
*/
public static function create($appId = null, $apiKey = null, $region = null)
{
$allowedRegions = self::getAllowedRegions();
$config = {{configClassname}}::create($appId, $apiKey, $region, $allowedRegions);
$allowedRegions = [{{#allowedRegions}}'{{.}}'{{^-last}},{{/-last}}{{/allowedRegions}}];

if (
{{^fallbackToAliasHost}}$region == null ||{{/fallbackToAliasHost}}

This comment has been minimized.

Copy link
@damcou

damcou Aug 10, 2022

Contributor

prefer === instead of ==
Is this condition only useful for the Personalization client (where {{{hostWithFallback}}} is empty) ?

This comment has been minimized.

Copy link
@shortcuts

shortcuts Aug 10, 2022

Author Member

oops yeah the linter might have fixed it but I'll edit it here

personalization, sources, predict and query-suggestions

($region !== null && !in_array($region, $allowedRegions, true))
) {
throw new AlgoliaException(
'`region` {{^fallbackToAliasHost}}is required and {{/fallbackToAliasHost}}must be one of the following: ' .
implode(', ', $allowedRegions)
);
}

return static::createWithConfig($config);
}
$config = {{configClassname}}::create($appId, $apiKey, $region);

/**
* Returns the allowed regions for the config
*/
public static function getAllowedRegions()
{
return [{{#allowedRegions}}'{{.}}'{{^-last}},{{/-last}}{{/allowedRegions}}];

This comment has been minimized.

Copy link
@damcou

damcou Aug 10, 2022

Contributor

I like the fact that this is not needed anymore

This comment has been minimized.

Copy link
@shortcuts

shortcuts Aug 10, 2022

Author Member

Yep I thought moving this logic at the client-level would make the templates better, but also avoid some extra logic when manually initializing clients (e.g. in our tests)

return static::createWithConfig($config);
}
{{/hasRegionalHost}}

Expand Down
3 changes: 1 addition & 2 deletions templates/php/tests/client/suite.mustache
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,7 @@ class {{clientPrefix}}Test extends TestCase implements HttpClientInterface
$config = {{clientPrefix}}Config::create(
$appId,
$apiKey,
$region,
{{client}}::getAllowedRegions()
$region
);
$clusterHosts = {{client}}::getClusterHosts($config);
{{/hasRegionalHost}}
Expand Down

0 comments on commit be6e4f4

Please sign in to comment.