https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21159
Tomás Cohen Arazi <[email protected]> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |[email protected] --- Comment #87 from Tomás Cohen Arazi <[email protected]> --- I'm pushing this one. I appreciate the effort to refactor things in a good way and thanks for that. I have some remarks, though. Those I feel deserve to be treated on their own bugs but I'll post here for now. 1. I think the method name should be changed to something like `location_update_trigger`. I hope it is clear the why the name change proposal. 2. I'm not sure this method should internally call ->store. Calling it triggers a series of things like logging, and maybe others. It feels like the caller should _do all the changes to the item they need and then finally save_. 3. I don't feel like the return value is cool or either documented enough. I recognize this is inherited from the original code in the C4 namespace. Moving to Koha:: should be part of cleaning and improving, so more thought is needed here. At a very first sight, I'd say this shouldn't be calling ->store implicitly, and should be returning $self for method chainability. If we need to return some messages for the caller to be aware of, we already have a standard way to do it, using the Koha::Object::Message class, which is encapsulated inside Koha::Object which Koha::Item inherits from (e.g. see Koha::Object->add_message). -- You are receiving this mail because: You are watching all bug changes. _______________________________________________ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
