Bug 16750 - Redirect from selectbranchprinter.pl to additem.pl causes software error
Summary: Redirect from selectbranchprinter.pl to additem.pl causes software error
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-06-16 11:08 UTC by Kyle M Hall
Modified: 2019-06-26 14:47 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.01


Attachments
Bug 16750 - Redirect from selectbranchprinter.pl to additem.pl causes software error (1.66 KB, patch)
2016-06-16 11:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16750 - Redirect from selectbranchprinter.pl to additem.pl causes software error (1.80 KB, patch)
2016-06-16 13:06 UTC, Marc Véron
Details | Diff | Splinter Review
[ALT] Bug 16750: Correctly redirect after setting library (1.75 KB, patch)
2016-06-28 14:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16750 - Redirect from selectbranchprinter.pl to additem.pl causes software error (1.84 KB, patch)
2019-06-10 18:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 16750: Redirect from selectbranchprinter.pl to additem.pl causes software error (1.90 KB, patch)
2019-06-13 05:58 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2016-06-16 11:08:42 UTC
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.
Comment 1 Kyle M Hall 2016-06-16 11:10:10 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2016-06-16 13:06:37 UTC
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>
Comment 3 Jonathan Druart 2016-06-19 11:41:20 UTC
(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.
Comment 4 Kyle M Hall 2016-06-27 13:55:46 UTC
(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!
Comment 5 Jonathan Druart 2016-06-28 14:41:37 UTC
Created attachment 52932 [details] [review]
[ALT] Bug 16750: Correctly redirect after setting library
Comment 6 Jonathan Druart 2016-06-28 14:42:04 UTC
Kyle, could you have a look at this patch? It seems to work as you want.
Comment 7 Srdjan Jankovic 2016-07-06 04:19:56 UTC
Jonathan, maybe a silly question: wouldn't it be better to get referer from the header in selectbranchprinter.pl?
Comment 8 Jonathan Druart 2016-07-06 14:17:41 UTC
(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.
Comment 9 Srdjan Jankovic 2016-07-07 01:17:28 UTC
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.
Comment 10 Kyle M Hall 2016-07-07 14:14:54 UTC
(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.
Comment 11 Jonathan Druart 2016-07-07 15:31:42 UTC
The problem with your patch is that it will only fix 1 page whereas we have plenty of others with the same issue.
Comment 12 Kyle M Hall 2016-07-08 15:09:51 UTC
(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?
Comment 13 Christopher Brannon 2016-08-07 05:48:24 UTC
This looks like it is in discussion and not ready for sign off.  Please correct the status change if I am incorrect.
Comment 15 Nick Clemens 2019-06-10 18:27:43 UTC
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>
Comment 16 Nick Clemens 2019-06-10 18:28:53 UTC
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
Comment 17 Katrin Fischer 2019-06-13 05:58:28 UTC
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>
Comment 18 Katrin Fischer 2019-06-13 05:59:41 UTC
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.
Comment 19 Martin Renvoize 2019-06-13 11:07:37 UTC
Nice work!

Pushed to master for 19.11.00
Comment 20 Jonathan Druart 2019-06-13 16:06:00 UTC
(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).
Comment 21 Fridolin SOMERS 2019-06-17 07:58:09 UTC
Pushed to 19.05.x for 19.05.01
Comment 22 Lucas Gass 2019-06-26 14:47:32 UTC
backported to 18.11.x for 18.11.07