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
Created attachment 74286 [details] [review] 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
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.
Created attachment 74399 [details] [review] Bug 20592: Checks for itemnumber sooner Two lines of code and move them earlier. Add a check to make sure there are changes.
Created attachment 74400 [details] [review] Bug 20592: Scope creep - fix other noise
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.
(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?
Created attachment 74824 [details] [review] Bug 20592: Add tests
Created attachment 74825 [details] [review] Bug 20592: Checks for itemnumber sooner Two lines of code and move them earlier. Add a check to make sure there are changes.
Created attachment 74826 [details] [review] Bug 20592: Scope creep - fix other noise
I agree with Mark, better to fix ModItem instead of hijacking the controller :)
(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.
(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 :).
Can you please rebase?
Comment on attachment 74826 [details] [review] Bug 20592: Scope creep - fix other noise This noise was fixed elsewhere (Bug 20620)
Created attachment 78453 [details] [review] 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>
Created attachment 78454 [details] [review] Bug 20592: Checks for itemnumber sooner Two lines of code and move them earlier. Add a check to make sure there are changes.
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.
Created attachment 80728 [details] [review] 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>
signed off
Created attachment 80731 [details] [review] 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>
Created attachment 80732 [details] [review] 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>
Awesome work all! Pushed to master for 18.11
Pushed to 18.05.x for 18.05.06
Pushed to 17.11.x for 17.11.13