Move search history from the SearchController to an helper Service #1018
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| <?php | ||
|
|
||
| namespace VuFind\Search; | ||
|
|
||
| use minSO; | ||
|
|
||
| class History | ||
| { | ||
|
|
||
| /** | ||
| * @var \VuFind\Db\Table\Search | ||
| */ | ||
| protected $searchTable; | ||
|
|
||
| /** | ||
| * @var \Zend\Session\SessionManager | ||
| */ | ||
| protected $sessionManager; | ||
|
|
||
| /** | ||
| * @var \VuFind\Search\Results\PluginManager | ||
| */ | ||
| protected $resultsManager; | ||
|
|
||
| /** | ||
| * @var \VuFind\Search\Memory | ||
| */ | ||
| protected $searchMemory; | ||
|
|
||
| /** | ||
| * History constructor | ||
| * @param \VuFind\Db\Table\Search $searchTable | ||
| * @param \Zend\Session\SessionManager $sessionManager | ||
| * @param \VuFind\Search\Results\PluginManager $resultsManager | ||
| * @param \VuFind\Search\Memory $searchMemory | ||
| */ | ||
| public function __construct($searchTable, $sessionManager, $resultsManager, $searchMemory) | ||
| { | ||
| $this->searchTable = $searchTable; | ||
| $this->sessionManager = $sessionManager; | ||
| $this->resultsManager = $resultsManager; | ||
| $this->searchMemory = $searchMemory; | ||
| } | ||
|
|
||
| /** | ||
| * @param int $userId | ||
| * @return array | ||
| */ | ||
| public function getSearchHistory($userId = null, $purged = false) | ||
| { | ||
| // Retrieve search history | ||
| $searchHistory = $this->searchTable->getSearches( | ||
| $this->sessionManager->getId(), | ||
| $userId | ||
| ); | ||
|
|
||
| // Build arrays of history entries | ||
| $saved = $unsaved = []; | ||
|
|
||
| // Loop through the history | ||
| /** @var \VuFind\Db\Row\Search $current */ | ||
| foreach ($searchHistory as $current) { | ||
|
|
||
| /** @var minSO $minSO */ | ||
| $minSO = $current->getSearchObject(); | ||
|
|
||
| // Saved searches | ||
| if ($current->saved == 1) { | ||
| $saved[] = $minSO->deminify($this->resultsManager); | ||
| } else { | ||
| // All the others... | ||
|
|
||
| // If this was a purge request we don't need this | ||
| if ($purged) { | ||
|
There was a problem hiding this comment. The purge search history functionality is broken by this refactoring. The It might also be more clean to separate the display and purge actions to separate functions -- that is, rather than purging as part of the retrieval of search history in the helper, instead have logic in the controller like: if (we need to purge) {
purge the unsaved history
forget the search in the search memory
}
return the history
That would require a little bit of refactoring, but would probably also be more efficient -- causing a single DELETE query followed by a single SELECT query, rather than one SELECT followed by potentially many DELETEs. It also has the benefit of decoupling the search memory and search history helper. (Note that if you want to test this, you just need to click the "Purge unsaved searches" link on the search history page). |
||
| $current->delete(); | ||
|
|
||
| // We don't want to remember the last search after a purge: | ||
| $this->searchMemory->forgetSearch(); | ||
| } else { | ||
| // Otherwise add to the list | ||
| $unsaved[] = $minSO->deminify($this->resultsManager); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return ['saved' => $saved, 'unsaved' => $unsaved]; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like it may be overkill to inject the session manager in order to extract the session ID here. I would suggest either injecting the session ID through the constructor (since it's essentially a constant value in the context of a single script execution) or else make it a parameter to getSearchHistory (since it's part of the input required to figure out which history to retrieve).