Allow back to search breadcrumb links to retain page/limit

[LuomaJuha]

Without this, if you search something, go to page 3, enter a record and return to search results, you will be redirected to page 1.

[demiankatz]

Thanks, @LuomaJuha! I'm trying to remember why we didn't do this in the first place, just in case there's a potential negative side effect of the change. I suspect that this feature was originally designed to persist options from the previous search into the next search (see the hidden sort/limit settings in searchbox.phtml), and of course in that context, persisting the page number would be a problem. But then it was later expanded upon to fully reconstitute the previous search, and in that context, having the page number IS useful.

In any case, I think what you've done here almost certainly makes sense, but I want to take a little time to test and make sure I'm not forgetting something, because some small part of me thinks there might be a reason we didn't do this sooner. @EreMaijala, do you have anything to add? :-)

[demiankatz]

@LuomaJuha, I spent a little time playing with this and found a scenario where it can be broken. Try this (I used a standard test instance created by phing startup for this example):

1.) Perform a search for "test", go to page 4, and click on the first record. The resulting page will have an appropriate "Search" link in the breadcrumbs.

2.) Open a new tab, perform a search for "publication", go to page 8, and click on the first record. This result will also have an appropriate breadcrumb link.

3.) Now go back to the record from step 1 and refresh the page. Now the breadcrumb link points to page 8 of the "test" search instead of page 4 -- we've inappropriately combined together the two different searches.

I think this is because we intentionally do not store the page number in the search table of the database since result counts can change over time and it's better to always point at the beginning of the set. Without that data to work from, @EreMaijala's "sid" parameter checking can't fully verify that everything matches up, and it just assumes that all the session search parameters are applicable.

I suspect that fixing this could be very complicated, and the benefits are probably limited. Maybe we should agree to just live with this... but I think this is the kind of behavior that I was worried about finding.

Any thoughts/ideas?

Meanwhile, I'm going to run the full test suite just to be sure there are no unexpected results there. I'll report back with results when that is done.

[demiankatz]

Automated tests are passing! :-)

[EreMaijala]

@demiankatz Just a thought that may or may not work, but if we had "additional parameters" in the search table, we could use them when appropriate and ignore them e.g. when clicking the search in the history. But since we don't want the search deduplication to include the page number, we could still get unexpected results by doing the same search in two tabs.

[demiankatz]

That makes sense to me, @EreMaijala -- having the pagination of the same result set bleed from one tab into another is much less serious to me than having the pagination of one result set bleed into a different result set, since that could end up pointing at an out-of-range page.

I thought we had an open JIRA ticket about saving extra search parameters, and I was going to reference it here, but I can't seem to find it. Is this something we talked about at a past Summit, or am I suffering from a false memory?

[EreMaijala]

@demiankatz It's https://openlibraryfoundation.atlassian.net/browse/VUFIND-1364 and it's done, but I think we might need another one to not abruptly change the "(not used by default)" part..

[LuomaJuha]

Hmm, I could try to produce something for practice and create maybe a new column for the search, something like extra_parameters LONGTEXT and functions getExtraParameters and setExtraParameters?

[demiankatz]

@LuomaJuha, I don't think changes to the database schema should be necessary -- the "extra details" are already getting stored in the serialized object stored in the database (see this code). I think all we would need to do is make sure the extra details field is used to store page data and that the data is used appropriately when deminifying (and, as @EreMaijala points out, remove the comment in the Minified class saying that the field is not used by default).

[LuomaJuha]

@demiankatz Thanks, will look to that then! I'll put this pr into a draft for now and return when I've come up with something.

[demiankatz]

Thanks, @LuomaJuha!

[LuomaJuha]

@demiankatz Well, here is the second implementation. I tried to alter the extraData and it worked, but it felt too much to change the functionality so radically, so I had a chat with @EreMaijala and decided to go with this approach. It now implements a new additionalParameters which is used as default.

[demiankatz]

Thanks, @LuomaJuha -- I think this is shaping up nicely, but I had a few thoughts and suggestions.

I confess that I haven't had time for more hands-on testing, but I'll definitely give things another look once we've finished discussing these ideas. :-)

[demiankatz]

Thanks, @LuomaJuha! I have a few more thoughts to share. Apologies if any of them are not productive -- I just think this can potentially get confusing, so I want to make sure we do all we can to make it as clear as possible. :-)

[demiankatz]

Is there a reason that we need to store optional parameters in a specific property here? Could we instead just proxy the relevant settings in the params object, e.g. have getOptionalParameters return

[
    'page' => $this->getParams()->getPage(),
    'limit' => $this->getParams()->getLimit(),
]

and have the setter proxy the appropriate params setters when the incoming array contains appropriate values?

And if we do it that way, should these methods actually go on the Params class rather than the Results class, or might there be a reason to keep them tied to results?

I realize this may all be by design, especially if doing it "by proxy" causes problems with search deduplication, etc. I can't remember exactly how that works, so depending on the existing approach, my idea might or might not be problematic. Just wondering if this has been considered, because I think this approach could be a bit confusing -- i.e. why are we storing page and limit in two different places?

[LuomaJuha]

Hmm, good point, I'll try and see what happens!

[demiankatz]

@LuomaJuha, also please note that because of recent changes to the dev branch, you should merge dev into this PR when you have a chance so that it can complete the "Tests with PHP 8.2" task -- otherwise it will remain stuck in a "waiting for status to be reported" state indefinitely.

[demiankatz]

Thanks for the progress on this, @LuomaJuha -- it's looking good, but I need to find some time to test it; I'll let you know as soon as I'm able to!

[demiankatz]

Sorry it's taken me so long to give this another look. I think we're nearly done apart from the semantics (though sometimes that's the hardest part). See below for thoughts... :-)

[demiankatz]

Thanks, @LuomaJuha, this solution looks very safe -- I'm confident it will do what is desired without causing unanticipated side effects.

My only concern is that I don't really like the naming of "optional parameters" since it's not at all clear what that means -- it's very broad.

My suggestion is that perhaps we should rename them to "remembered pagination parameters" or something like that, and then we can also improve our comments to offer better clarity. For example:

/**
 * Get remembered pagination parameters from saved search. We track these separately since
 * in some contexts we want to use them (e.g. linking back to a search in breadcrumbs), but in
 * other contexts we want to ignore them (e.g. comparing two searches to see if they represent
 * the same query -- because page 1 and page 2 still represent the same overall search).
 *
 * @return array
 */
public function getRememberedPaginationParameters(): array

Maybe that's too verbose, and we could use "getLastPaginationParameters" or "getSavedPaginationParameters". Or maybe that's too narrow, and we need a broader term than "pagination" to encompass other things that might be saved (like view mode, for example). I'm definitely open to other ideas -- but hopefully you see the general direction I'm trying to move this in. :-)

[LuomaJuha]

Yea, that is true! Hmm, I'll see if my brain comes up with any reasonable name, and if not I'll stick with this plan.

[demiankatz]

I guess we could also pick @EreMaijala's brain for opinions if needed. :-)

[LuomaJuha]

getSearchControlParameters is something that my brain was thinking. Don't know if it sounds good enough

[demiankatz]

How about getSearchContextParameters? "Control" suggests to me that the parameters could change the way the search behaves, but Context is perhaps more suggestive of where we are within the search.

[demiankatz]

Or maybe getSavedSearchContextParameters, to make it clear that this context was previously saved and is now being restored (as opposed to just returning the current values of page/limit). I realize it's getting longer again, though...

[LuomaJuha]

Well, we now have more room for longer function names! :)

[demiankatz]

True!

[demiankatz]

This is working correctly for me and all tests are passing. Thanks again -- merging now!