Implemented SphinxAdapter#218
Conversation
Added a new adapter to support searches through SphinxClient
Added Sphinx adapter documentation.
Added comment param to Sphinx docs.
Added missing setCutoff + setMaxMatches into Adapter to control query output.
Total_found is not corresponding with the actual available pages, but with the possible available results. Changed to total.
|
@stof could you check this pull request? It's a ready-to-eat meal )) |
sampart
left a comment
sampart
left a comment
There was a problem hiding this comment.
Thank you for contributing your work back to the community, @m4ikel! I'm sorry about the delay in this getting reviewed. I've got a few comments about changes I think this needs before we merge it. Thanks in anticipation. Sam
| ``` | ||
|
|
||
| ### SphinxAdapter | ||
| To paginate a [Sphinx](http://php.net/manual/en/class.sphinxclient.php) query: |
There was a problem hiding this comment.
Is there a better page you could link to? The PHP client page doesn't actually say what Sphinx is. Is it http://sphinxsearch.com/?
| /* | ||
| * This file is part of the Pagerfanta package. | ||
| * | ||
| * (c) Maikel Doezé <maikel.doeze@marqin.nl> |
There was a problem hiding this comment.
Can you remove the copyright message? I know that many of the files have a copyright message relating to Pablo Diez, but for simplicity, we want to move towards one copyright framework for the whole project rather than per-file statements.
| class SphinxAdapter implements AdapterInterface | ||
| { | ||
| private $client; | ||
| private $query; |
There was a problem hiding this comment.
You don't need the extra indent here.
| * @param SphinxClient $client A Sphinx client. | ||
| * @param $query A Sphinx query. | ||
| * @param $index A Sphinx index. | ||
| * @param $comment A Sphinx comment. |
There was a problem hiding this comment.
Please could you fix the indentation on these two lines?
| $this->client = $client; | ||
| $this->query = $query; | ||
| $this->index = $index; | ||
| $this->comment = $comment; |
There was a problem hiding this comment.
Could you tidy the indentation here too, please?
| * Constructor. | ||
| * | ||
| * @param SphinxClient $client A Sphinx client. | ||
| * @param $query A Sphinx query. |
There was a problem hiding this comment.
Are @query, $index and $comment all strings? Please could you update the docblock to show this?
| /* | ||
| * setMaxMatches | ||
| * | ||
| * @param $maxMatches Controls how much matches searchd will keep in RAM while searching. |
There was a problem hiding this comment.
Could you put a variable type in the docblock here too please?
| /* | ||
| * setCutoff | ||
| * | ||
| * @param $maxMatches Used for advanced performance control. It tells searchd to forcibly stop search query once cutoff matches have been found and processed. |
There was a problem hiding this comment.
Please could you put a variable type in this docblock too?
There was a problem hiding this comment.
Also, the variable name in the docblock doesn't match the parameter name.
| private $index; | ||
| private $comment; | ||
| private $results; | ||
| private $maxMatches = 0; |
There was a problem hiding this comment.
Are you missing a setter for $maxMatches?
| $this->comment = $comment; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
You've got a bit of extra indentation here that you don't need.
|
And tests are missing |
Removed copyright notice, fixed docblocks added getters and changed readme.
3 participants
I needed a SphinxClient adapter for my own use but it might be useful for other people, so here we go.