yiisoft / yii2-elasticsearch

Yii 2 Elasticsearch extension
http://www.yiiframework.com
BSD 3-Clause "New" or "Revised" License
427 stars 253 forks source link

Fix suggester #284

Closed beowulfenator closed 4 years ago

beowulfenator commented 4 years ago
Q A
Is bugfix? yes
New feature? no
Breaks BC? yes
Tests pass? yes
Fixed issues #216
beowulfenator commented 4 years ago

There are issues I'd like to discuss. This update is technically not backwards compatible, because the returned data is in a slightly different format. The _suggest endpoint used to return suggest data only, _search returns it all. So if in the past data could get accessed as

$result['customer_name'][0]['options']

now it is

$result['suggest']['customer_name'][0]['options']

We could return $result['suggest'], not $result, but that removes some useful data, for example, which shards/indices were successful, which failed.

The other problem is input data format. The suggest() method (and query() method, for that matter) are written to support both arrays and strings as input. If it's an array, it is Json::encode()'d. This creates some ugliness in the code, or otherwise I'd need to decode the input string.

Thoughts on this?

samdark commented 4 years ago

There are many installations running on 2.1 dev branch so these may break. While technically we haven't promised 2.1 release will be compatible with 2.1 dev API or 2.0, it's a good idea to keep the format unless there's a very good reason for a change.

samdark commented 4 years ago

How about keeping suggest() returning $result['suggest'] but adding search() as well?

beowulfenator commented 4 years ago

I guess you are right, we could return $result['suggest'], and if full result is needed, then they could search(). I've updated the code.

What is your opinion on the other issue: building JSON body by hand?

samdark commented 4 years ago

I'm not sure what ugliness is that? The presence of encode itself?

beowulfenator commented 4 years ago

This:

$body = '{"suggest":'.$suggester.',"size":0}';
samdark commented 4 years ago

Well, it's not that ugly by itself but the fact that there's no escaping or anything kind of is. I see no other way to achieve it though...

beowulfenator commented 4 years ago

Shall we merge it then?