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.
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.
We should replace the remaining DBIx code fragments scattered here and there..
I agree we should either remove it from AUTOLOAD or write our own as a wrapper on top of the class's ->store.
Created attachment 86017 [details] [review] 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.
Created attachment 90914 [details] [review] Bug 21761: Call set + store as update
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.
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.
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.
(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 ;)
(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?
(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.
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 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?
Created attachment 95947 [details] [review] Bug 21761: Call set + store as update Signed-off-by: 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.
Looking here
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
Created attachment 96136 [details] [review] Bug 21761: Do not call $self->store in ->set_password
The latest patch is not signed-off yet so marking this as "Needs Signoff".
(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 :)
(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?
(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.
Created attachment 96154 [details] [review] 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.
Created attachment 96155 [details] [review] Bug 21761: Do not call $self->store in ->set_password Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Really glad to see this one move, thanks to all involved... The code is looking polished and good now, Pushing.
Nice work everyone! Pushed to master for 20.05
Not backported to 19.11.x