Review options handling for index creation #1822
Conversation
deguif
force-pushed
the
review-options-handling-for-index-creation
branch
from
4786dfa to
c36e254
Compare
|
I'll probably need a bit more details in the PR description and a changelog ;-) |
deguif
force-pushed
the
review-options-handling-for-index-creation
branch
from
c36e254 to
6b935f4
Compare
|
Just added a better description. I'm adding a changelog. |
deguif
force-pushed
the
review-options-handling-for-index-creation
branch
from
6b935f4 to
09a22a3
Compare
| @@ -381,26 +381,31 @@ public function refresh(): Response | |||
| */ | |||
| public function create(array $args = [], $options = null): Response | |||
There was a problem hiding this comment.
General thought: Should we become more strict on what is allowed to be passed to options? The code below is way too complex as we piled onto it over to time not have a breaking change I guess. Good news is, your PR seems to simplify it at least.
There was a problem hiding this comment.
Update: Just read your PR message again. ++ on deprecation additions.
There was a problem hiding this comment.
Yes exactly, I will deprecate all types except array. So this will be backward compatible and will be easy to identify which part to drop when bumping a new major version.
There was a problem hiding this comment.
By the way there's this PR #1823 that is tied to deprecations, would be nice if you could have a look so that we can go forward and use this mechanism for all futures deprecations. I have several ones in mind ;)
| @@ -96,7 +96,9 @@ protected function _createIndex(?string $name = null, bool $delete = true, int $ | |||
| $client = $this->_getClient(); | |||
| $index = $client->getIndex($name); | |||
|
|
|||
| $index->create(['settings' => ['index' => ['number_of_shards' => $shards, 'number_of_replicas' => 1]]], $delete); | |||
| $index->create(['settings' => ['index' => ['number_of_shards' => $shards, 'number_of_replicas' => 1]]], [ | |||
There was a problem hiding this comment.
Out of curiosity: The old one would still work but we discourage using it like this?
There was a problem hiding this comment.
Yes the old one will still work, but it will be deprecated so this is to avoid triggering the future deprecation.
There are multiple changes. Maybe I should split the PR.
TypeErrorif not an expected type.$optionsis always converted to an array (I was planning to add deprecations with\trigger_error('xxx', E_USER_DEPRECATED)for other types in a following PR)