Description
Kyle M Hall
2016-06-16 11:08:42 UTC
Created attachment 52456 [details] [review] Bug 16750 - Redirect from selectbranchprinter.pl to additem.pl causes software error How to reproduce: 1) Edit and save an item in the items editor 2) You should be redirected back to the items editor, note that the biblionumber is passed in the url 3) Choose "set library" from the upper right hand pulldown on the page 4) Choose a different library, click submit 5) You should get the error message: Software error: Can't call method "fields" on an undefined value at /home/vagrant/kohaclone/cataloguing/additem.pl line 736. Test Plan: 1) Apply this patch 2) Follow plan above 3) No software error should occur at step 5 Created attachment 52462 [details] [review] Bug 16750 - Redirect from selectbranchprinter.pl to additem.pl causes software error How to reproduce: 1) Edit and save an item in the items editor 2) You should be redirected back to the items editor, note that the biblionumber is passed in the url 3) Choose "set library" from the upper right hand pulldown on the page 4) Choose a different library, click submit 5) You should get the error message: Software error: Can't call method "fields" on an undefined value at /home/vagrant/kohaclone/cataloguing/additem.pl line 736. Test Plan: 1) Apply this patch 2) Follow plan above 3) No software error should occur at step 5 Signed-off-by: Marc Véron <veron@veron.ch> (In reply to Kyle M Hall from comment #0) > 2) You should be redirected back to the items editor, note that the > biblionumber is passed in the url "is *not* passed" I guess? I don't get this error (using plack?) I don't like the idea to pass GET parameter to a POST form, it sounds weird. I suppose this error is going to happen for all POST forms, should not we try to provide a global fix? Looking at circ/selectbranchprinter.pl it seems that oldreferer must be set to avoid such a failure. (In reply to Jonathan Druart from comment #3) > (In reply to Kyle M Hall from comment #0) > > 2) You should be redirected back to the items editor, note that the > > biblionumber is passed in the url > > "is *not* passed" I guess? > I don't get this error (using plack?) > I don't like the idea to pass GET parameter to a POST form, it sounds weird. > I suppose this error is going to happen for all POST forms, should not we > try to provide a global fix? > Looking at circ/selectbranchprinter.pl it seems that oldreferer must be set > to avoid such a failure. I think a global fix would be much better, but I'm not sure how that could be accomplished if it all aside from switching all possible post forms to get. That seems like a less than ideal solutions. If you have any ideas I'd love to hear them! Created attachment 52932 [details] [review] [ALT] Bug 16750: Correctly redirect after setting library Kyle, could you have a look at this patch? It seems to work as you want. Jonathan, maybe a silly question: wouldn't it be better to get referer from the header in selectbranchprinter.pl? (In reply to Srdjan Jankovic from comment #7) > Jonathan, maybe a silly question: wouldn't it be better to get referer from > the header in selectbranchprinter.pl? It's what does the current code, but the HTTP_REFERER does not contain the POST params. But actually I am not sure it's a good idea: with this patch the form will be submitted again and the item will be saved a second time. Ah I see. I usually go for storing that in the session, but this is fine too. I'll let Kyle have a look, and then sign off. (In reply to Jonathan Druart from comment #8) > (In reply to Srdjan Jankovic from comment #7) > > Jonathan, maybe a silly question: wouldn't it be better to get referer from > > the header in selectbranchprinter.pl? > > It's what does the current code, but the HTTP_REFERER does not contain the > POST params. > But actually I am not sure it's a good idea: with this patch the form will > be submitted again and the item will be saved a second time. I think the creation of the extra item is blocker for your alternative patch. If you can find a way to avoid that I'd be happy to sign yours. Otherwise my patch, though a more specific case, is safer. The problem with your patch is that it will only fix 1 page whereas we have plenty of others with the same issue. (In reply to Jonathan Druart from comment #11) > The problem with your patch is that it will only fix 1 page whereas we have > plenty of others with the same issue. I totally agree. I much prefer your patch except for the risk of accidentally adding data to the database. I'm sure this behavior exists not only for additem.pl, but for any cgi driven data creation that does not involve a redirect. Do you have any thoughts on how to avoid that? This looks like it is in discussion and not ready for sign off. Please correct the status change if I am incorrect. Created attachment 90462 [details] [review] Bug 16750 - Redirect from selectbranchprinter.pl to additem.pl causes software error How to reproduce: 1) Edit and save an item in the items editor 2) You should be redirected back to the items editor, note that the biblionumber is not passed in the url 3) Choose "set library" from the upper right hand pulldown on the page 4) Choose a different library, click submit 5) You should get the error message: Software error: Can't call method "fields" on an undefined value at /home/vagrant/kohaclone/cataloguing/additem.pl line 736. Test Plan: 1) Apply this patch 2) Follow plan above 3) No software error should occur at step 5 Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Three years later, still a valid bug. In testing Kyle's patch solves the issue - Jonathan's leads to 404 I think we should open new bug for the more general fix and deal with those issues there and fix up at least this known error Created attachment 90553 [details] [review] Bug 16750: Redirect from selectbranchprinter.pl to additem.pl causes software error How to reproduce: 1) Edit and save an item in the items editor 2) You should be redirected back to the items editor, note that the biblionumber is passed in the url 3) Choose "set library" from the upper right hand pulldown on the page 4) Choose a different library, click submit 5) You should get the error message: Software error: Can't call method "fields" on an undefined value at /home/vagrant/kohaclone/cataloguing/additem.pl line 736. Test Plan: 1) Apply this patch 2) Follow plan above 3) No software error should occur at step 5 Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> I've often run into this bug by accidentally refreshing the page (very annoying if it happens during a presentation). The fix is small but will be very helpful to avoid this issue. I'd love a global fix, but I think this is worth having for the refresh issue alone. Nice work! Pushed to master for 19.11.00 (In reply to Katrin Fischer from comment #18) > I've often run into this bug by accidentally refreshing the page (very > annoying if it happens during a presentation). The fix is small but will be > very helpful to avoid this issue. I'd love a global fix, but I think this is > worth having for the refresh issue alone. I have opened bug 23117 for the global fix (if it is what you meant). Pushed to 19.05.x for 19.05.01 backported to 18.11.x for 18.11.07 |