Instead of just running as_formatted, check if GetMarcBiblio returned a reference. If you e.g. did not pass an id, return 404 instead of 500.
Created attachment 23231 [details] [review] Bug 11329: Check for marc record in opac-showmarc Instead of just running as_formatted, check if GetMarcBiblio returned a reference. If you e.g. did not pass an id, return 404 instead of 500. Consistent with opac-export.pl Test plan: [1] Run opac-showmarc.pl with valid biblionumber in id parameter. [2] Remove id parameter from URL. You should get a 404 now.
Created attachment 23232 [details] [review] Bug 11329: Check for marc record in catalogue/showmarc Intranet counterpart patch. Same check as in opac/opac-showmarc. Test plan: [1] Run showmarc.pl with valid biblionumber in id parameter. [2] Remove id parameter from URL. You should get a 404 now.
Marcel, why do you think that these patches are necessary? I suppose, in the case of the OPAC, it might make sense to return a 404 instead of a 500, but when is an opac-showmarc.pl query string going to be malformed? In the case of the staff client, I got a 404 both before/after trying the patch, so I'm not sure that it's necessary. I imagine that there must already be checking higher up in showmarc.pl?
(In reply to David Cook from comment #3) > Marcel, why do you think that these patches are necessary? Hi David. Thanks for testing. They are not that important :) But I would like to reduce the 500s in my log (especially from opac, mainly coming from Googlebot). Testing if something is what it should be, cannot hurt BTW :) > I suppose, in the case of the OPAC, it might make sense to return a 404 > instead of a 500, but when is an opac-showmarc.pl query string going to be > malformed? That is indeed somewhat less likely, but not imaginary. 1) Someone saved a URL. The record does no longer exist. 2) User 1 deleted the record, Zebra has not been updated yet. User 2 clicks on the link. 3) If the marcxml of a record is malformed (due to some earlier error or so), GetMarcBiblio returns undef. We should check that and not assume a valid MARC::Record object. > In the case of the staff client, I got a 404 both before/after trying the > patch, so I'm not sure that it's necessary. I imagine that there must > already be checking higher up in showmarc.pl? Really? I got the following 500 error on an wrong biblio number: Can't call method "as_formatted" on an undefined value at /usr/share/koha/testclone/catalogue/showmarc.pl line 82. You should be able to repeat this error in current master..
Created attachment 24417 [details] [review] Bug 11329: Check for marc record in opac-showmarc Instead of just running as_formatted, check if GetMarcBiblio returned a reference. If you e.g. did not pass an id, return 404 instead of 500. Consistent with opac-export.pl Test plan: [1] Run opac-showmarc.pl with valid biblionumber in id parameter. [2] Remove id parameter from URL. You should get a 404 now. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 24418 [details] [review] Bug 11329: Check for marc record in catalogue/showmarc Intranet counterpart patch. Same check as in opac/opac-showmarc. Test plan: [1] Run showmarc.pl with valid biblionumber in id parameter. [2] Remove id parameter from URL. You should get a 404 now. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
It work well for me, i am redirected to 404. It is not the best, I think we should catch the error and display a suitable warning. But it improves the actual state.
Created attachment 24500 [details] [review] Bug 11329: Check for marc record in opac-showmarc Instead of just running as_formatted, check if GetMarcBiblio returned a reference. If you e.g. did not pass an id, return 404 instead of 500. Consistent with opac-export.pl Test plan: [1] Run opac-showmarc.pl with valid biblionumber in id parameter. [2] Remove id parameter from URL. You should get a 404 now. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 24501 [details] [review] Bug 11329: Check for marc record in opac-showmarc Instead of just running as_formatted, check if GetMarcBiblio returned a reference. If you e.g. did not pass an id, return 404 instead of 500. Consistent with opac-export.pl Test plan: [1] Run opac-showmarc.pl with valid biblionumber in id parameter. [2] Remove id parameter from URL. You should get a 404 now. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
I think it would be less confusing for end-users if we had a "no record found" error instead of a generic 404, but this works.
Created attachment 24502 [details] [review] Bug 11329: Check for marc record in catalogue/showmarc Intranet counterpart patch. Same check as in opac/opac-showmarc. Test plan: [1] Run showmarc.pl with valid biblionumber in id parameter. [2] Remove id parameter from URL. You should get a 404 now. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Passes koha-qa.pl, works as advertised.
Pushed to master. Thanks, Marcel! Note bug 11581.
Patches pushed to 3.14.x, will be in 3.14.5
Pushed to 3.12.x will be in 3.12.13