polyfractal / sherlock

MIT License
119 stars 28 forks source link

IDs filter requires array of strings for value #73

Closed samsullivan closed 11 years ago

samsullivan commented 11 years ago

This may be on purpose, since the documentation takes and array of strings as the value parameter. However, I think its an undesirable scenario since most often IDs are integers.

My use case involved me taking an array of primary keys from my database (to restrict the search results) and passing it to Sherlock::filterBuilder()->Ids()->type('type')->values($array_pk);. This caused an error because on line 40 of Ids.php it checks for a string argument in the array of values. Currently I have to select my PKs from the database and loop through converting to strings.

My suggestion is something like this; if you like it I can submit a pull request.

foreach ($args as $arg) {
    if (settype($arg, 'string')) {
        $this->params['values'][] = $arg;
    }
}
polyfractal commented 11 years ago

Actually, I just checked and Elasticsearch happily accepts numbers in addition to strings. Looks like they do the conversion internally. ES IDs are actually concatenated with the type so they are a string anyhow. No need for us to convert on our end.

If you could do a PR that removes the string checking entirely, and just sets $this->params['values'] = $args I'll merge it.

Thanks!

samsullivan commented 11 years ago

Will do after dinner tonight.