[Bug 21761] New: Koha::Object supports passing through 'update' which means we can side step 'set' + 'store'
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21761 Bug ID: 21761 Summary: Koha::Object supports passing through 'update' which means we can side step 'set' + 'store' Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: critical Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: martin.renvoize@ptfs-europe.com QA Contact: testopia@bugs.koha-community.org If we're adding triggers at the code level all over the place by overloading the store method we need to never ever let dbic handle it.. by allowing the 'update' method through we are exposing ourselves to places in the codebase skipping the store triggers. -- 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=21761 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org, | |kyle@bywatersolutions.com, | |martin.renvoize@ptfs-europe | |.com, tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21761 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Priority|P5 - low |P1 - high -- 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=21761 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21761 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=21610 --- Comment #1 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Indeed, I think we should fix it at Koha::Object level (once again), but on top of bug 21610. We could move the handle of default values to a specific (private) method and call it with get_dirty_columns on ->store, ->update or ->set. -- 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=21761 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl --- Comment #2 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- We should replace the remaining DBIx code fragments scattered here and there.. -- 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=21761 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=21465 -- 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=21761 --- Comment #3 from Tomás Cohen Arazi <tomascohen@gmail.com> --- I agree we should either remove it from AUTOLOAD or write our own as a wrapper on top of the class's ->store. -- 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=21761 --- Comment #4 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 86017 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86017&action=edit Bug 21761: Try to overload Koha::Object->update to handle incorrect values And use default values instead. This is a terrible patch, but publish it to help others having their try. -- 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=21761 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=23064 -- 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=21761 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |23185 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 [Bug 23185] Koha::Objects supports passing through 'update' which means we can side step 'set' + 'store' -- 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=21761 --- Comment #5 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 90914 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90914&action=edit Bug 21761: Call set + store as update -- 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=21761 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |In Discussion --- Comment #6 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Sorry Jonathan, I hadn't spotted you'd already written a patch here.. but aren't you over complicating it? I've submitted an alternative patch which I think does the same but is considerably simpler... catch me if I'm incorrect please. -- 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=21761 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=23186 -- 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=21761 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |23187 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23187 [Bug 23187] Koha::Objects supports find_or_create without passing through the Koha::Object store method -- 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=21761 --- Comment #7 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Apply the changes in the tests, then run it. You will get a warning with your patch: t/db_dependent/Koha/Object.t .. 7/11 Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018. -- 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=21761 --- Comment #8 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Also, I think, my version will be faster: the fix is done only the attributes we are updating, not all the attributes of the object. -- 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=21761 --- Comment #9 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- (In reply to Jonathan Druart from comment #7)
Apply the changes in the tests, then run it. You will get a warning with your patch:
t/db_dependent/Koha/Object.t .. 7/11 Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018.
Surely that highlights a bug in the current 'store' routine then rather than my patch ;) -- 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=21761 --- Comment #10 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- (In reply to Jonathan Druart from comment #8)
Also, I think, my version will be faster: the fix is done only the attributes we are updating, not all the attributes of the object.
Again, I would suggest this is a bug in our store routine as it stands.. update really should just be shortcut to set + store else we're introducing bugs by having two distinct ways of doing the same thing. Also, in my patch because you're calling the parent store method you're not only dealing with the over-rides withs Koha::Object but also any class specific overrides in, for example, Koha::Patron are you not? -- 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=21761 --- Comment #11 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Martin Renvoize from comment #10)
(In reply to Jonathan Druart from comment #8)
Also, I think, my version will be faster: the fix is done only the attributes we are updating, not all the attributes of the object.
Again, I would suggest this is a bug in our store routine as it stands.. update really should just be shortcut to set + store else we're introducing bugs by having two distinct ways of doing the same thing.
Ok! :)
Also, in my patch because you're calling the parent store method you're not only dealing with the over-rides withs Koha::Object but also any class specific overrides in, for example, Koha::Patron are you not?
I do not get what you mean. I think we should investigate the warning and provide tests. -- 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=21761 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #86017|0 |1 is obsolete| | -- 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=21761 --- Comment #12 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- To investigate: + $itemtype = $builder->build_object( { class => 'Koha::ItemTypes' }); + $itemtype->update({ notforloan => "" }); These two lines will warn "Argument "" isn't numeric in numeric eq (==)" -- 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=21761 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |rel_20_05_target, | |RM_priority -- 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=21761 --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- I was referred here again from report 23260 where I used an update on a Koha object without store. (Table items_last_borrower just 4 fields.) Can we get this "discussion" further? As stated in comment1 I think we need some hook subroutine on store, update etc. If there is no 'hook', could we still do some faster updates than looping with store and next? -- 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=21761 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Signed Off -- 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=21761 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #90914|0 |1 is obsolete| | --- Comment #14 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 95947 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=95947&action=edit Bug 21761: Call set + store as update Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- 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=21761 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |martin.renvoize@ptfs-europe |ity.org |.com --- Comment #15 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Jonathan Druart from comment #12)
To investigate:
+ $itemtype = $builder->build_object( { class => 'Koha::ItemTypes' }); + $itemtype->update({ notforloan => "" });
These two lines will warn "Argument "" isn't numeric in numeric eq (==)"
I did not manage to remove this warning. However it highlights a misuse: setting a string to an integer field. As we are going to deal in ->store I think that's fine. -- 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=21761 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |BLOCKED --- Comment #16 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=21761 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Failed QA --- Comment #17 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Patrons.t fails on set_password set_password contains an update (which was formerly DBIx, and now no longer), so it is now routed back to sub store which starts another txn_do store contains a get_from_storage for the password, and here we end up in some transaction problem with wrong data -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21761 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=21761 Jonathan Druart <jonathan.druart@bugs.koha-community.org> 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=21761 --- Comment #18 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 96136 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=96136&action=edit Bug 21761: Do not call $self->store in ->set_password -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21761 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Needs Signoff CC| |joonas.kylmala@helsinki.fi --- Comment #19 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- The latest patch is not signed-off yet so marking this as "Needs Signoff". -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21761 --- Comment #20 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Joonas Kylmälä from comment #19)
The latest patch is not signed-off yet so marking this as "Needs Signoff".
When the first patch has been already signed off we do not necessarily ask for a second signoff. In this case I am expecting a ping pong with Marcel and so do not need to involve another tester. However another eyeball/thoughts from QA is more than welcome :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21761 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off --- Comment #21 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Joonas Kylmälä from comment #19)
The latest patch is not signed-off yet so marking this as "Needs Signoff".
Hi Joonas: Please check the assigned QA contact and if there was recent activity as was here. In that case you can pick another? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21761 --- Comment #22 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (In reply to Marcel de Rooy from comment #21)
Hi Joonas: Please check the assigned QA contact and if there was recent activity as was here. In that case you can pick another?
Thanks, I didn't realize I could check that. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21761 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch 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=21761 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #95947|0 |1 is obsolete| | --- Comment #23 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 96154 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=96154&action=edit Bug 21761: Call set + store as update Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Removed a double comma. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21761 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #96136|0 |1 is obsolete| | --- Comment #24 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 96155 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=96155&action=edit Bug 21761: Do not call $self->store in ->set_password 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=21761 --- Comment #25 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Really glad to see this one move, thanks to all involved... The code is looking polished and good now, Pushing. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21761 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |19.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=21761 --- Comment #26 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=21761 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|19.05.00 |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=21761 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|rel_20_05_target, | |RM_priority | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21761 Joy Nelson <joy@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |joy@bywatersolutions.com Status|Pushed to master |RESOLVED Resolution|--- |FIXED --- Comment #27 from Joy Nelson <joy@bywatersolutions.com> --- Not backported to 19.11.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21761 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=25303 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org