Differential D17384 Diff 42188 src/infrastructure/cluster/config/PhabricatorClusterSearchConfigOptionType.php
Changeset View
Changeset View
Standalone View
Standalone View
src/infrastructure/cluster/config/PhabricatorClusterSearchConfigOptionType.php
- This file was added.
<?php | |||||
final class PhabricatorClusterSearchConfigOptionType | |||||
extends PhabricatorConfigJSONOptionType { | |||||
public function validateOption(PhabricatorConfigOption $option, $value) { | |||||
if (!is_array($value)) { | |||||
throw new Exception( | |||||
pht( | |||||
'Search cluster configuration is not valid: value must be a '. | |||||
'list of search hosts.')); | |||||
} | |||||
$engines = PhabricatorSearchService::loadAllFulltextStorageEngines(); | |||||
foreach ($value as $index => $spec) { | |||||
if (!is_array($spec)) { | |||||
throw new Exception( | |||||
pht( | |||||
'Search cluster configuration is not valid: each entry in the '. | |||||
'list must be a dictionary describing a search service, but '. | |||||
'the value with index "%s" is not a dictionary.', | |||||
$index)); | |||||
} | |||||
try { | |||||
epriestley: Minor, but it would be a little cleaner to do the `checkMap()` before this, because the… | |||||
PhutilTypeSpec::checkMap( | |||||
$spec, | |||||
array( | |||||
'type' => 'string', | |||||
'hosts' => 'optional list<map<string, wild>>', | |||||
'roles' => 'optional map<string, wild>', | |||||
'port' => 'optional int', | |||||
'protocol' => 'optional string', | |||||
'path' => 'optional string', | |||||
'version' => 'optional int', | |||||
)); | |||||
} catch (Exception $ex) { | |||||
throw new Exception( | |||||
pht( | |||||
'Search engine configuration has an invalid service '. | |||||
'specification (at index "%s"): %s.', | |||||
$index, | |||||
$ex->getMessage())); | |||||
} | |||||
if (!array_key_exists($spec['type'], $engines)) { | |||||
throw new Exception( | |||||
pht('Invalid search engine type: %s. Valid types include: %s', | |||||
$spec['type'], | |||||
implode(', ', array_keys($engines)))); | |||||
} | |||||
Done Inline Actions
epriestley: - We should `checkMap()` each entry of the `$hosts` array too.
- We should require that 'type'… | |||||
if (isset($spec['hosts'])) { | |||||
foreach ($spec['hosts'] as $hostindex => $host) { | |||||
try { | |||||
PhutilTypeSpec::checkMap( | |||||
$host, | |||||
array( | |||||
'host' => 'string', | |||||
'roles' => 'optional map<string, wild>', | |||||
'port' => 'optional int', | |||||
'protocol' => 'optional string', | |||||
'path' => 'optional string', | |||||
'version' => 'optional int', | |||||
)); | |||||
} catch (Exception $ex) { | |||||
throw new Exception( | |||||
pht( | |||||
'Search cluster configuration has an invalid host '. | |||||
'specification (at index "%s"): %s.', | |||||
$hostindex, | |||||
$ex->getMessage())); | |||||
} | |||||
} | |||||
} | |||||
} | |||||
} | |||||
} |
Minor, but it would be a little cleaner to do the checkMap() before this, because the 'type' key might not be present (or might be an array or something) which could cause warnings/errors. Both checks look good, just swap the order so we make sure the keys/types are good before doing anything with 'type'.