[Bug 20592] New: updateitem.pl causes database errors when empty non-public item notes updated
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20592 Bug ID: 20592 Summary: updateitem.pl causes database errors when empty non-public item notes updated Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: minor Priority: P5 - low Component: Cataloging Assignee: koha-bugs@lists.koha-community.org Reporter: dcook@prosentient.com.au QA Contact: testopia@bugs.koha-community.org CC: m.de.rooy@rijksmuseum.nl If you go to /cgi-bin/koha/catalogue/moredetail.pl?biblionumber=1&itemnumber=1#item1, and click "Update" next to an empty "Non-public note" field, it will generate a database error. Typically, this is hidden in your Apache logs, but if you have DEBUG turned on, the database error will be fatal. Example logs: [Tue Apr 17 12:43:48.607368 2018] [cgi:error] [pid 1992] [client ...:22158] AH01215: [Tue Apr 17 12:43:48 2018] updateitem.pl: DBD::mysql::st execute failed: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'WHERE itemnumber='1'' at line 1 [for Statement "UPDATE items SET WHERE itemnumber=?" with ParamValues: 0="1"] at ../git/C4/Items.pm line 2038.: ../git/catalogue/updateitem.pl, referer: http://koha/cgi-bin/koha/catalogue/moredetail.pl?biblionumber=1&itemnumber=1 [Tue Apr 17 12:43:48.607852 2018] [cgi:error] [pid 1992] [client ...:22158] AH01215: [Tue Apr 17 12:43:48 2018] updateitem.pl: DBD::mysql::st execute failed: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'WHERE itemnumber='1'' at line 1 [for Statement "UPDATE items SET WHERE itemnumber=?" with ParamValues: 0="1"] at ../git/C4/Items.pm line 2038.: ../git/catalogue/updateitem.pl, referer: http://koha/cgi-bin/koha/catalogue/moredetail.pl?biblionumber=1&itemnumber=1 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20592 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20592 --- Comment #1 from David Cook <dcook@prosentient.com.au> --- Created attachment 74286 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=74286&action=edit Bug 20592 - updateitem.pl causes database errors when empty non-public item notes updated Previously, clicking "Update" next to non-public item note when the text box was empty would generate a database error. This patches adds a condition where ModItem() is only called if there are actual changes to be made to the record. _TEST PLAN_ 1) Create a bibliographic record 2) Create an item for that bib record 3) Go to /cgi-bin/koha/catalogue/moredetail.pl?biblionumber=1&itemnumber=1 4) Click "Update" next to "Non-public note" 5) Note a "DBD::mysql::st execute failed" error in your logs 6) Apply patch 7) Click "Update" next to "Non-public note" 8) Note that there is no longer an error 9) Add text to the box and click "Update" to ensure update still works -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20592 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtompset@hotmail.com Status|Needs Signoff |In Discussion --- Comment #2 from M. Tompsett <mtompset@hotmail.com> --- I believe this is the wrong way to do it. The problem is ModItem requires an itemnumber, and the itemnumber check is too late in the function. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20592 --- Comment #3 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 74399 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=74399&action=edit Bug 20592: Checks for itemnumber sooner Two lines of code and move them earlier. Add a check to make sure there are changes. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20592 --- Comment #4 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 74400 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=74400&action=edit Bug 20592: Scope creep - fix other noise -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20592 --- Comment #5 from M. Tompsett <mtompset@hotmail.com> --- Test plan is the same as comment #1. Testing on a kohadevbox after a reset all. blanking the log before clicking update: echo | sudo tee /var/log/koha/kohadev/plack-error.log Used the same url given: http://localhost:8081/cgi-bin/koha/catalogue/moredetail.pl?biblionumber=1&itemnumber=1 Before my two patches, noise and update triggers more. After my two patches, no noise. David's patch works, but if there are other places which need the same check they would have to be modified too. The ModItem function should be validating its parameters. Also, I don't believe you need to check for biblionumber in your condition, David. My first patch adds that validation, so other functions receive the same fix. My second just cleans up some noise, since the first warning wasn't the only noise triggered. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20592 --- Comment #6 from David Cook <dcook@prosentient.com.au> --- (In reply to M. Tompsett from comment #2)
I believe this is the wrong way to do it. The problem is ModItem requires an itemnumber, and the itemnumber check is too late in the function.
No, that's not the problem. The problem is an an empty $item hashref being passed into C4::Items::_koha_modify_item(), which creates an invalid SQL query. But I agree that a better solution would be to change ModItem rather than updateitem.pl. I considered that, but I figured I would make the lightest touch possible and test the one place where I knew there was a problem. Do you know whether or not it's appropriate to return early from ModItem? What impact does that have across Koha? -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20592 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20592 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #74286|0 |1 is obsolete| | Attachment #74399|0 |1 is obsolete| | Attachment #74400|0 |1 is obsolete| | --- Comment #7 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 74824 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=74824&action=edit Bug 20592: Add tests -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20592 --- Comment #8 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 74825 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=74825&action=edit Bug 20592: Checks for itemnumber sooner Two lines of code and move them earlier. Add a check to make sure there are changes. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20592 --- Comment #9 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 74826 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=74826&action=edit Bug 20592: Scope creep - fix other noise -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20592 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |mtompset@hotmail.com |ity.org | CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #10 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- I agree with Mark, better to fix ModItem instead of hijacking the controller :) -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20592 --- Comment #11 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to David Cook from comment #6)
But I agree that a better solution would be to change ModItem rather than updateitem.pl. I considered that, but I figured I would make the lightest touch possible and test the one place where I knew there was a problem. Do you know whether or not it's appropriate to return early from ModItem? What impact does that have across Koha?
I would not expect side-effects as ModItem does not return a useful value. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20592 --- Comment #12 from David Cook <dcook@prosentient.com.au> --- (In reply to Jonathan Druart from comment #11)
(In reply to David Cook from comment #6)
But I agree that a better solution would be to change ModItem rather than updateitem.pl. I considered that, but I figured I would make the lightest touch possible and test the one place where I knew there was a problem. Do you know whether or not it's appropriate to return early from ModItem? What impact does that have across Koha?
I would not expect side-effects as ModItem does not return a useful value.
I meant more in terms of other parts of Koha catching a fatal error with an eval{} or something like that, and acting on the error. But I'm not fussed either way :). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20592 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de Status|Needs Signoff |Patch doesn't apply --- Comment #13 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Can you please rebase? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20592 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #74826|0 |1 is obsolete| | --- Comment #14 from M. Tompsett <mtompset@hotmail.com> --- Comment on attachment 74826 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=74826 Bug 20592: Scope creep - fix other noise This noise was fixed elsewhere (Bug 20620) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20592 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #74824|0 |1 is obsolete| | --- Comment #15 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 78453 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=78453&action=edit Bug 20592: Add tests NOTE: It isn't really a test, but it does trigger the return line which was added to ModItem. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20592 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #74825|0 |1 is obsolete| | --- Comment #16 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 78454 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=78454&action=edit Bug 20592: Checks for itemnumber sooner Two lines of code and move them earlier. Add a check to make sure there are changes. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20592 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff --- Comment #17 from M. Tompsett <mtompset@hotmail.com> --- Second patch needs sign off, because as far as I can tell, I wrote it. :) First patch was provided by Jonathan, and I have signed it, off by running prove in combination with both patches. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20592 Cori Lynn Arnold <carnold@dgiinc.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #78454|0 |1 is obsolete| | --- Comment #18 from Cori Lynn Arnold <carnold@dgiinc.com> --- Created attachment 80728 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=80728&action=edit Bug 20592: Checks for itemnumber sooner Two lines of code and move them earlier. Add a check to make sure there are changes. Followed the test plan, both the _BEFORE_ and after do as described. Signed-off-by: Cori Lynn Arnold <carnold@dgiinc.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20592 Cori Lynn Arnold <carnold@dgiinc.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |carnold@dgiinc.com Status|Needs Signoff |Signed Off --- Comment #19 from Cori Lynn Arnold <carnold@dgiinc.com> --- signed off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20592 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20592 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #78453|0 |1 is obsolete| | Attachment #80728|0 |1 is obsolete| | --- Comment #20 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 80731 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=80731&action=edit Bug 20592: Add tests NOTE: It isn't really a test, but it does trigger the return line which was added to ModItem. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20592 --- Comment #21 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 80732 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=80732&action=edit Bug 20592: Return early in ModItem if nothing to update Two lines of code and move them earlier. Add a check to make sure there are changes. Followed the test plan, both the _BEFORE_ and after do as described. Signed-off-by: Cori Lynn Arnold <carnold@dgiinc.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20592 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com Status|Passed QA |Pushed to Master --- Comment #22 from Nick Clemens <nick@bywatersolutions.com> --- Awesome work all! Pushed to master for 18.11 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20592 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com Status|Pushed to Master |Pushed to Stable --- Comment #23 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Pushed to 18.05.x for 18.05.06 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20592 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Status|Pushed to Stable |RESOLVED CC| |fridolin.somers@biblibre.co | |m --- Comment #24 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- Pushed to 17.11.x for 17.11.13 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org