Bug 21761 - Koha::Object supports passing through 'update' which means we can side step 'set' + 'store'
Summary: Koha::Object supports passing through 'update' which means we can side step '...
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P1 - high critical (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 23185 23187
  Show dependency treegraph
 
Reported: 2018-11-05 11:43 UTC by Martin Renvoize
Modified: 2019-06-25 22:54 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 21761: Try to overload Koha::Object->update to handle incorrect values (6.75 KB, patch)
2019-03-04 17:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21761: Call set + store as update (1.13 KB, patch)
2019-06-21 15:48 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2018-11-05 11:43:45 UTC
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.
Comment 1 Jonathan Druart 2018-11-06 15:44:02 UTC
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.
Comment 2 Marcel de Rooy 2018-11-06 15:47:48 UTC
We should replace the remaining DBIx code fragments scattered here and there..
Comment 3 Tomás Cohen Arazi 2019-02-01 11:34:26 UTC
I agree we should either remove it from AUTOLOAD or write our own as a wrapper on top of the class's ->store.
Comment 4 Jonathan Druart 2019-03-04 17:00:15 UTC
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.
Comment 5 Martin Renvoize 2019-06-21 15:48:48 UTC
Created attachment 90914 [details] [review]
Bug 21761: Call set + store as update
Comment 6 Martin Renvoize 2019-06-21 15:49:58 UTC
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.
Comment 7 Jonathan Druart 2019-06-22 21:24:34 UTC
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.
Comment 8 Jonathan Druart 2019-06-22 21:33:19 UTC
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.
Comment 9 Martin Renvoize 2019-06-24 07:31:49 UTC
(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 ;)
Comment 10 Martin Renvoize 2019-06-24 07:34:10 UTC
(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?
Comment 11 Jonathan Druart 2019-06-25 22:52:42 UTC
(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.
Comment 12 Jonathan Druart 2019-06-25 22:54:00 UTC
To investigate:

+    $itemtype = $builder->build_object( { class => 'Koha::ItemTypes' });
+    $itemtype->update({ notforloan => "" });

These two lines will warn "Argument "" isn't numeric in numeric eq (==)"