Move search history from the SearchController to an helper Service #1018
Conversation
…wn helper service. So the ServiceManager can be used to get the latest search queries.
removed unnecessary field
|
This makes sense to me... though two comments: 1.) There appear to be some conflicts with the current master. Let me know if you need help resolving them. 2.) Rather than injecting the service locator into the history service, I think it would be better to pull the dependencies out in the factory and inject them directly. This makes the relationships between the objects more explicit and also makes it easier to write test code. I'm happy to make these changes myself if you don't have time. In any case, thanks for sharing this improvement! |
…ufind into separate-search-history
|
Hello Demian, |
…arch-history # Conflicts: # module/VuFind/src/VuFind/Controller/SearchController.php
There was a problem hiding this comment.
Thanks for the progress on this. I've taken care of the conflict resolution -- it was caused by the fact that getServiceLocator() has been deprecated in controllers, so I had to change all references to that method to access the serviceLocator property instead.
While reviewing the code, I noticed a couple of issues (one serious, one more cosmetic) highlighted here. I'm happy to help with implementation of either or both of these changes if you don't have time; please let me know!
| { | ||
| // Retrieve search history | ||
| $searchHistory = $this->searchTable->getSearches( | ||
| $this->sessionManager->getId(), |
There was a problem hiding this comment.
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).
| // 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 $this->params()->fromQuery('purge') == 'true' used in the controller to check for the purge parameter is removed. I think this should instead be used to pass a value to this function, and perhaps the $purged variable should be renamed to something more clear.
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).
|
@seboettg, I had a few minutes free, so I implemented my suggestions on this PR. This fixes the broken purge feature and makes the history object a little simpler. Please take a look and let me know if you approve. If you're happy with this approach, I think we can merge soon; otherwise, we can continue to discuss it! |
|
Hey Demian! |
|
@seboettg, thanks for the double-check. I will merge this to master shortly after the 4.1 release. |
This moves the search history logic from the SearchController to an own helper service. So the ServiceManager can be used to get the latest search queries.
For example, this could be useful to build search suggestions that recommend users their last search queries.