We have multiple sites that are still exhibiting bug 11369. For whatever reason, the cookies on these sites are larger and thus the maximum of 10 is still too large. This setting should be controlled by a system preference to the number can be tailored on a per-site basis.
Created attachment 33616 [details] [review] Bug 13265 - Still too many search cursor cookies We have multiple sites that are still exhibiting bug 11369. For whatever reason, the cookies on these sites are larger and thus the maximum of 10 is still too large. This setting should be controlled by a system preference to the number can be tailored on a per-site basis. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Set the new system preference MaxSearchCookies to a high number ( e.g. 20 ). 4) Perform searches until you get the error described in bug 11369 5) Delete your cookies so you can use the staff intranet again 5) Set the system preference to a much lower number ( e.g. 5 ) 6) Repeat setp 4, you should no longer get the error.
I have an idea to fix this that would use the sessions table (like the OPAC browser but without its current limitations). The fix I have in mind will allow the result browser to work in multiple tabs without bloating the request header which leads to the fatal error. It's more work, but I think it'll be a more robust fix. That said, I wouldn't be opposed to this patch getting pushed in the mean-time...
The search cookie is no longer used in master, search history is stored in the database. Have we missed anything?
(In reply to Abby from comment #3) > The search cookie is no longer used in master, search history is stored in > the database. > > Have we missed anything? Interesting! I haven't looked at master recently, so I'm not sure. I wonder which commit changed the use of the search cookie...
(In reply to David Cook from comment #4) > (In reply to Abby from comment #3) > > The search cookie is no longer used in master, search history is stored in > > the database. > > > > Have we missed anything? > > Interesting! I haven't looked at master recently, so I'm not sure. I wonder > which commit changed the use of the search cookie... I think its not the same thing, user search history is the history of wich search query he has used. Bug 11369 concerns the browser used after a search to go to previous or next result.
(In reply to Fridolin SOMERS from comment #5) > (In reply to David Cook from comment #4) > > (In reply to Abby from comment #3) > > > The search cookie is no longer used in master, search history is stored in > > > the database. > > > > > > Have we missed anything? > > > > Interesting! I haven't looked at master recently, so I'm not sure. I wonder > > which commit changed the use of the search cookie... > > I think its not the same thing, user search history is the history of wich > search query he has used. > Bug 11369 concerns the browser used after a search to go to previous or next > result. If that's true, shouldn't my test plan fail?
Version is 'unspecified', is this for master? Wanted to test with patch, does not apply: Applying: Bug 13265 - Still too many search cursor cookies Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/cataloguing.pref Auto-merging installer/data/mysql/updatedatabase.pl CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl Auto-merging installer/data/mysql/sysprefs.sql Failed to merge in the changes. Patch failed at 0001 Bug 13265 - Still too many search cursor cookies
$ git bz apply 13265 Bug 13265 - Still too many search cursor cookies 33616 - Bug 13265 - Still too many search cursor cookies Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 13265 - Still too many search cursor cookies Using index info to reconstruct a base tree... M installer/data/mysql/sysprefs.sql M installer/data/mysql/updatedatabase.pl M koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/cataloguing.pref M koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt M koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt M koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/cataloguing.pref Auto-merging installer/data/mysql/updatedatabase.pl CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl Auto-merging installer/data/mysql/sysprefs.sql Failed to merge in the changes. Patch failed at 0001 Bug 13265 - Still too many search cursor cookies The copy of the patch that failed is found in: /home/magnus/scripts/kohaclone/.git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-13265---Still-too-many-search-cursor-cookies-bMkL_V.patch
Created attachment 37973 [details] [review] Bug 13265 - Still too many search cursor cookies We have multiple sites that are still exhibiting bug 11369. For whatever reason, the cookies on these sites are larger and thus the maximum of 10 is still too large. This setting should be controlled by a system preference to the number can be tailored on a per-site basis. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Set the new system preference MaxSearchCookies to a high number ( e.g. 20 ). 4) Perform searches until you get the error described in bug 11369 5) Delete your cookies so you can use the staff intranet again 5) Set the system preference to a much lower number ( e.g. 5 ) 6) Repeat setp 4, you should no longer get the error.
Kyle, Don't you think it would be better to correctly fix the issue and store the searches using the sessionStorage? It will avoid YASP.
(In reply to Jonathan Druart from comment #10) > Kyle, > Don't you think it would be better to correctly fix the issue and store the > searches using the sessionStorage? > It will avoid YASP. Yes, but as David said, this will work in the mean-time, unless someone is willing to write and submit a counter patch!
Created attachment 39284 [details] [review] Bug 13265 - Still too many search cursor cookies We have multiple sites that are still exhibiting bug 11369. For whatever reason, the cookies on these sites are larger and thus the maximum of 10 is still too large. This setting should be controlled by a system preference to the number can be tailored on a per-site basis. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Set the new system preference MaxSearchCookies to a high number ( e.g. 20 ). 4) Perform searches until you get the error described in bug 11369 5) Delete your cookies so you can use the staff intranet again 5) Set the system preference to a much lower number ( e.g. 5 ) 6) Repeat setp 4, you should no longer get the error. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Works, but does not remove all superfluous cookies. The code still removes only the oldest one.
Created attachment 39391 [details] [review] [COUNTER-PATCH] Bug 13265: Use sessionStorage to store searches instead of cookies This is a counter patch. The idea is to provide a permanent solution for the cookie length issue we occurred on storing the searches (intranet side). Test plan: Launch as many searches as you can (don't forget to sleep). You should not get any error. Confirm there is no regression using the results browser.
(In reply to Kyle M Hall from comment #11) > (In reply to Jonathan Druart from comment #10) > > Kyle, > > Don't you think it would be better to correctly fix the issue and store the > > searches using the sessionStorage? > > It will avoid YASP. > > Yes, but as David said, this will work in the mean-time, unless someone is > willing to write and submit a counter patch! Here it is!
Created attachment 39438 [details] [review] [Signed-off] Bug 13265: Use sessionStorage to store searches instead of cookies This is a counter patch. The idea is to provide a permanent solution for the cookie length issue we occurred on storing the searches (intranet side). Test plan: Launch as many searches as you can (don't forget to sleep). You should not get any error. Confirm there is no regression using the results browser. Tested with 6 parralel searches in different tabs (with alternatively browising up and down). No problems found. Signed-off-by: Marc Véron <veron@veron.ch>
Signed off counter patch ( Bug 13265: Use sessionStorage to store searches instead of cookies )
Kyle, could you have a look at my counter patch please?
Created attachment 39948 [details] [review] [PASSED QA] [PASSED QA] Bug 13265: Use sessionStorage to store searches instead of cookies This is a counter patch. The idea is to provide a permanent solution for the cookie length issue we occurred on storing the searches (intranet side). Test plan: Launch as many searches as you can (don't forget to sleep). You should not get any error. Confirm there is no regression using the results browser. Tested with 6 parralel searches in different tabs (with alternatively browising up and down). No problems found. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Patch pushed to master. Thanks Jonathan!
Pushed to 3.20.x will be in 3.20.1
Patch pushed to 3.18.x will be in 3.18.08
*** Bug 12510 has been marked as a duplicate of this bug. ***