[Bug 24966] New: Fix calls to maybe_add where method call does not return a value
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24966 Bug ID: 24966 Summary: Fix calls to maybe_add where method call does not return a value Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: critical Priority: P5 - low Component: SIP2 Assignee: koha-bugs@lists.koha-community.org Reporter: kyle@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org CC: colin.campbell@ptfs-europe.com For reasons unknown to me, a call like: -- $resp .= maybe_add( FID_CALL_NUMBER, $item->call_number, $server ); -- will not work as expected if the item has no callnumber. One would expect the parameters to the subroutine to be: 'CY', under, and a SIPServer object. What is actually received is: 'CY', and a SIPServer object. We ingest the parameters like so: -- sub maybe_add { my ($fid, $value, $server) = @_; -- So, what happens is $value is populated with the server object! This can cause bad output like this: -- OUTPUT MSG: '101YNN20200324 063701AOBPL|AB32503201584185|AQBPL|AJCat /|CK001|CRn|CSJ 636.8 CLU|CYC4::SIP::SIPServer=HASH(0x1ea0e58)|DAC4::SIP::SIPServer=HASH(0x1ea0e58)|'--- I do not know why this is happening, but there are a couple solutions: 1) Move the $server parameter to be the first parameter of the subroutine call 2) Check the value of $value and swap $value and $server if it is the server object. I will write a patch implementing the latter solution, as it is a two line change, versus a many line change for the former. -- 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=24966 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |kyle@bywatersolutions.com |ity.org | -- 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=24966 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24966 --- Comment #1 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 101559 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=101559&action=edit Bug 24966: Fix calls to maybe_add where method call does not return a value For reasons unknown to me, a call like: -- $resp .= maybe_add( FID_CALL_NUMBER, $item->call_number, $server ); -- will not work as expected if the item has no callnumber. One would expect the parameters to the subroutine to be: 'CY', under, and a SIPServer object. What is actually received is: 'CY', and a SIPServer object. We ingest the parameters like so: -- sub maybe_add { my ($fid, $value, $server) = @_; -- So, what happens is $value is populated with the server object! This can cause bad output like this: -- OUTPUT MSG: '101YNN20200324 063701AOBPL|AB32503201584185|AQBPL|AJCat /|CK001|CRn|CSJ 636.8 CLU|CYC4::SIP::SIPServer=HASH(0x1ea0e58)|DAC4::SIP::SIPServer=HASH(0x1ea0e58)|' -- Test Plan: 1) On master, perform a checkin of an item not on hold using the sip cli tester 2) Note some fields contain something like 'C4::SIP::SIPServer=HASH(0x1ea0e58)' 3) Apply this patch 4) Restart the SIP server 5) Perform the SIP checkin again 6) Those fields from step 2 should be gone! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24966 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24966 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #101559|0 |1 is obsolete| | --- Comment #2 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 101632 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=101632&action=edit Bug 24966: Fix calls to maybe_add where method call does not return a value For reasons unknown to me, a call like: -- $resp .= maybe_add( FID_CALL_NUMBER, $item->call_number, $server ); -- will not work as expected if the item has no callnumber. One would expect the parameters to the subroutine to be: 'CY', under, and a SIPServer object. What is actually received is: 'CY', and a SIPServer object. We ingest the parameters like so: -- sub maybe_add { my ($fid, $value, $server) = @_; -- So, what happens is $value is populated with the server object! This can cause bad output like this: -- OUTPUT MSG: '101YNN20200324 063701AOBPL|AB32503201584185|AQBPL|AJCat /|CK001|CRn|CSJ 636.8 CLU|CYC4::SIP::SIPServer=HASH(0x1ea0e58)|DAC4::SIP::SIPServer=HASH(0x1ea0e58)|' -- Test Plan: 1) On master, perform a checkin of an item not on hold using the sip cli tester 2) Note some fields contain something like 'C4::SIP::SIPServer=HASH(0x1ea0e58)' 3) Apply this patch 4) Restart the SIP server 5) Perform the SIP checkin again 6) Those fields from step 2 should be gone! Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Barbara Johnson <Barbara.Johnson@bedfordtx.gov> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24966 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #3 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Could you provide a test? I did not manage to write one that fails without your patch. I suspect a place where we return () instead of undef -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24966 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24966 --- Comment #4 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #3)
Could you provide a test? I did not manage to write one that fails without your patch. I suspect a place where we return () instead of undef
Try as I might, I cannot replicate the error outside of production. Instead of this patch, would a patch reordering the parameters by more acceptable? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24966 --- Comment #5 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- I am not sure I understand, how would you do that? My fair is that the real bug is hidden somewhere else, and fixing it here will keep it hidden. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24966 --- Comment #6 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #5)
I am not sure I understand, how would you do that?
My fair is that the real bug is hidden somewhere else, and fixing it here will keep it hidden.
I agree, bug for the life of me I cannot even understand how this is possible. I think we might be even getting into perl internals finding the bug! I will write a version of the alternate I proposed. It's a more 'correct' fix anyway imo. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24966 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24966 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #101632|0 |1 is obsolete| | --- Comment #7 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 102584 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=102584&action=edit Bug 24966: [Alt] Fix calls to maybe_add where method call does not return a value For reasons unknown to me, a call like: -- $resp .= maybe_add( FID_CALL_NUMBER, $item->call_number, $server ); -- will not work as expected if the item has no callnumber. One would expect the parameters to the subroutine to be: 'CY', under, and a SIPServer object. What is actually received is: 'CY', and a SIPServer object. We ingest the parameters like so: -- sub maybe_add { my ($fid, $value, $server) = @_; -- So, what happens is $value is populated with the server object! This can cause bad output like this: -- OUTPUT MSG: '101YNN20200324 063701AOBPL|AB32503201584185|AQBPL|AJCat /|CK001|CRn|CSJ 636.8 CLU|CYC4::SIP::SIPServer=HASH(0x1ea0e58)|DAC4::SIP::SIPServer=HASH(0x1ea0e58)|' -- Test Plan: 1) On master, perform a checkin of an item not on hold using the sip cli tester 2) Note some fields contain something like 'C4::SIP::SIPServer=HASH(0x1ea0e58)' 3) Apply this patch 4) Restart the SIP server 5) Perform the SIP checkin again 6) Those fields from step 2 should be gone! Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Barbara Johnson <Barbara.Johnson@bedfordtx.gov> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24966 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #102584|0 |1 is obsolete| | --- Comment #8 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 102585 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=102585&action=edit Bug 24966: [Alt] Fix calls to maybe_add where method call does not return a value For reasons unknown to me, a call like: -- $resp .= maybe_add( FID_CALL_NUMBER, $item->call_number, $server ); -- will not work as expected if the item has no callnumber. One would expect the parameters to the subroutine to be: 'CY', under, and a SIPServer object. What is actually received is: 'CY', and a SIPServer object. We ingest the parameters like so: -- sub maybe_add { my ($fid, $value, $server) = @_; -- So, what happens is $value is populated with the server object! This can cause bad output like this: -- OUTPUT MSG: '101YNN20200324 063701AOBPL|AB32503201584185|AQBPL|AJCat /|CK001|CRn|CSJ 636.8 CLU|CYC4::SIP::SIPServer=HASH(0x1ea0e58)|DAC4::SIP::SIPServer=HASH(0x1ea0e58)|' -- Test Plan: 1) On master, perform a checkin of an item not on hold using the sip cli tester 2) Note some fields contain something like 'C4::SIP::SIPServer=HASH(0x1ea0e58)' 3) Apply this patch 4) Restart the SIP server 5) Perform the SIP checkin again 6) Those fields from step 2 should be gone! Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Barbara Johnson <Barbara.Johnson@bedfordtx.gov> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24966 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |BLOCKED CC| |m.de.rooy@rijksmuseum.nl --- Comment #9 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Looking here -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24966 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Failed QA --- Comment #10 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- We're not on the right track here. But hang on, found something.. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24966 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24966 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #102585|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24966 --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Since you refer to a wrong message with CY, I should have started looking for that in the first place ;) Check CY in Constants FID_HOLD_PATRON_ID => 'CY', Now git grep FID_HOLD_PATRON_ID Sip/Constants.pm: FID_HOLD_PATRON_ID Sip/Constants.pm: FID_HOLD_PATRON_ID => 'CY', Sip/MsgType.pm:$resp .= maybe_add( FID_HOLD_PATRON_ID, $item->hold_patron_bcode, $server ); Ah, this call should be the one. If hold_patron_bcode returns an implicit undef in list context (read: empty list), than we found the culprit. Look for it: sub hold_patron_bcode { my $self = shift; my $borrowernumber = (@_ ? shift: $self->hold_patron_id()) or return; my $holder = Koha::Patrons->find( $borrowernumber ); if ($holder and $holder->cardnumber ) { return $holder->cardnumber; } return; } And yes! The last return in list context makes $server shift to the second position. So how should we address that? It is used only once. We could fix the call with scalar or explicitly return empty string. I choose for the latter here. Preventing a second bad call in future. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24966 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Trivial patch Status|Failed QA |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24966 --- Comment #12 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 102689 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=102689&action=edit Bug 24966: (QA follow-up) Fix return value of hold_patron_bcode It is used in list context, but we need a scalar value. Can be fixed by adding scalar's, or returning empty string as here. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24966 --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Kyle M Hall from comment #6)
I agree, bug for the life of me I cannot even understand how this is possible. I think we might be even getting into perl internals finding the bug!
No perl internals here ;) Just the difference between scalar and list context.. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24966 --- Comment #14 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Another note when git grepping maybe_add: Sip/MsgType.pm: $resp .= maybe_add( FID_SCREEN_MSG, $msg, $server, $server ); two servers ? this occurs more than once, 15x ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24966 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|critical |major -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24966 --- Comment #15 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Should not we need another QA review on your patch Marcel? Should not we provide a test to cover the change? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24966 --- Comment #16 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to Marcel de Rooy from comment #14)
Another note when git grepping maybe_add:
Sip/MsgType.pm: $resp .= maybe_add( FID_SCREEN_MSG, $msg, $server, $server ); two servers ? this occurs more than once, 15x ?
Thanks Marcel! That is definitely a bug as well. I'll file a separate report for that! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24966 --- Comment #17 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #15)
Should not we need another QA review on your patch Marcel? Should not we provide a test to cover the change?
Leaving that to the RM. Thx for feedback. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24966 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #102689|0 |1 is obsolete| | --- Comment #18 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 102697 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=102697&action=edit Bug 24966: (QA follow-up) Fix return value of hold_patron_bcode It is used in list context, but we need a scalar value. Can be fixed by adding scalar's, or returning empty string as here. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24966 --- Comment #19 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 102698 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=102698&action=edit Bug 24966: Add unit tests Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24966 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=25107 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24966 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |20.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24966 --- Comment #20 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Nice work everyone! Pushed to master for 20.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24966 Joy Nelson <joy@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|20.05.00 |20.05.00, 19.11.06 released in| | CC| |joy@bywatersolutions.com Status|Pushed to master |Pushed to stable --- Comment #21 from Joy Nelson <joy@bywatersolutions.com> --- backported to 19.11.x for 19.11.06 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24966 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |Pushed to oldstable CC| |lucas@bywatersolutions.com Version(s)|20.05.00, 19.11.06 |20.05.00, 19.11.06, released in| |19.05.11 --- Comment #22 from Lucas Gass <lucas@bywatersolutions.com> --- backported to 19.05.x for 19.05.11 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org