[Bug 13019] New: Add base classes on which to build Koha objects
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Bug ID: 13019 Summary: Add base classes on which to build Koha objects Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: gmcharlt@gmail.com Reporter: kyle@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org The idea behind this is to have a pair of base classes on which to build our new generation of Koha objects. Koha::Object is a base class, which in it's most basic form, is to represent a row in a table. For example, Koha::Borrower inherits from Koha::Object. So too could Koha::Biblio and Koha::Item for example. Koha::Objects is to represent a way to fetch and manipulate sets of objects. For example, Koha::Borrowers has a method to get a Koha::Borrower object by id and a method to search for an get a list of Koha::Borrower objects. Right now Koha::Objects has only the essentials but can easily be extended and those enhancements will be passed down to all the child classes based on it. By using these classes as a base, we will add consistency to our code, allow us to keep our code DRY, reduce bugs, and encapsulate our database access among other benefits. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|gmcharlt@gmail.com |kyle@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 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.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 --- Comment #1 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 31964 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=31964&action=edit Bug 13019 - Add base classes on which to build Koha objects The idea behind this is to have a pair of base classes on which to build our new generation of Koha objects. Koha::Object is a base class, which in it's most basic form, is to represent a row in a table. For example, Koha::Borrower inherits from Koha::Object. So too could Koha::Biblio and Koha::Item for example. Koha::Objects is to represent a way to fetch and manipulate sets of objects. For example, Koha::Borrowers has a method to get a Koha::Borrower object by id and a method to search for an get a list of Koha::Borrower objects. Right now Koha::Objects has only the essentials but can easily be extended and those enhancements will be passed down to all the child classes based on it. By using these classes as a base, we will add consistency to our code, allow us to keep our code DRY, reduce bugs, and encapsulate our database access among other benefits. Test Plan: 1) Apply this patch 2) prove t/Object.t t/db_dependent/Object.t t/db_dependent/Objects.t -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |12892 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@biblibre.co | |m --- Comment #2 from Jonathan Druart <jonathan.druart@biblibre.com> --- First thoughts (already sent on #koha): 1/ Type and ObjectClass could be attributes, instead of methods 2/ I really would like to see method name without capital letters (and underscore '_' to separate words) 3/ "Bad setter returns undef" => not sure about that. Bad setter should croak 4/ What about using Class::Accessor? 5/ t/db_dependent/Objects.t should be t/db_dependent/Borrowers.t -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 --- Comment #3 from Kyle M Hall <kyle@bywatersolutions.com> ---
1/ Type and ObjectClass could be attributes, instead of methods
Could write a followup? I tried to do that but had issues with getting the base class to know about the package variable. I was properly implementing it wrong.
2/ I really would like to see method name without capital letters (and underscore '_' to separate words)
I have no strong preference here as long as we stick to one convention. My only concern would be a convention conflict where a table has the same name as a field in the table. That would lead to ambiguity, but I'm not sure if we even have an instance of this. For example, let's say a table has the field borrower which is actually a borrowernumber. I'd like to have $row->borrower return the borrowernumber, but $row->Borrower return the Koha::Borrower object. That way we know when we are getting back a scalar, and when we are getting back an Object. It would be silly to fetch the Koha::Borrower in a case where we just need the borrowernumber, which we would already have.
3/ "Bad setter returns undef" => not sure about that. Bad setter should croak
Agreed.
4/ What about using Class::Accessor?
I don't see the advantage to this. Can you give an example. This implementation uses a DBIC object as a place to store the data, so there is no need to have separate properties. Please note the this should not conflict this unit testing on a database-less system. t/Object.t should demonstrate this.
5/ t/db_dependent/Objects.t should be t/db_dependent/Borrowers.t
I can see your point, but the tests are really testing the base classes, and not the Borrower classes per se. However, I have no strong opinion so that's fine by me. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 --- Comment #4 from Jonathan Druart <jonathan.druart@biblibre.com> --- I tried to make Koha::Borrower extends Koha::Schema::Result::Borrower, but I get an error. The idea was to remove the AUTOLOAD and avoid the IsStored, IsChanged, Id, etc. methods. [the error was: Unable to perform storage-dependent operations with a detached result source (source '_unnamed_' is not associated with a schema)] I suppose I tried to do something very bad :) So, some additional remarks on the patch: - "result" and "columns" should be _result and _columns to avoid collisions later, don't you think? - Are you sure it is useful to test if the row is stored and has been changed (Store method)? Not sure about the perf, maybe DBIC does that (?). - IsStored should be InStorage or completely removed, if useless. - Delete could call delete in an eval, to avoid a call to in_storage - what about 2 constructors? new and new_from_rs? The ResultSet should not be create if not used later - what about a _result method? It would return ->{_result} if already exists, otherwise create it. All occurrences to ->{result} will be replaced by ->_result -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 --- Comment #5 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #4)
I tried to make Koha::Borrower extends Koha::Schema::Result::Borrower, but I get an error. The idea was to remove the AUTOLOAD and avoid the IsStored, IsChanged, Id, etc. methods.
[the error was: Unable to perform storage-dependent operations with a detached result source (source '_unnamed_' is not associated with a schema)] I suppose I tried to do something very bad :)
So, some additional remarks on the patch:
- "result" and "columns" should be _result and _columns to avoid collisions later, don't you think?
Good idea
- Are you sure it is useful to test if the row is stored and has been changed (Store method)? Not sure about the perf, maybe DBIC does that (?).
Yeah, I'd have to check the DBIC code, but that seems entirely likely.
- IsStored should be InStorage or completely removed, if useless.
Again, your probably right. I can't see a use for this right now and we can always add an InStorage method at a later date if it *is* needed for something.
- Delete could call delete in an eval, to avoid a call to in_storage
Do you think that will improve efficiency? in_storage has already been calculated and is just a dbic property. It seems like using eval would take more time.
- what about 2 constructors? new and new_from_rs? The ResultSet should not be create if not used later
For Koha::Object? That's a good idea.
- what about a _result method? It would return ->{_result} if already exists, otherwise create it. All occurrences to ->{result} will be replaced by ->_result
Also a good idea. I'll see if I can't make some time next week to implement these changes. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #31964|0 |1 is obsolete| | --- Comment #6 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 32004 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32004&action=edit Bug 13019 - Add base classes on which to build Koha objects The idea behind this is to have a pair of base classes on which to build our new generation of Koha objects. Koha::Object is a base class, which in it's most basic form, is to represent a row in a table. For example, Koha::Borrower inherits from Koha::Object. So too could Koha::Biblio and Koha::Item for example. Koha::Objects is to represent a way to fetch and manipulate sets of objects. For example, Koha::Borrowers has a method to get a Koha::Borrower object by id and a method to search for an get a list of Koha::Borrower objects. Right now Koha::Objects has only the essentials but can easily be extended and those enhancements will be passed down to all the child classes based on it. By using these classes as a base, we will add consistency to our code, allow us to keep our code DRY, reduce bugs, and encapsulate our database access among other benefits. Test Plan: 1) Apply this patch 2) prove t/Object.t t/db_dependent/Object.t t/db_dependent/Objects.t -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 --- Comment #7 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #4)
- "result" and "columns" should be _result and _columns to avoid collisions later, don't you think?
Done!
- Are you sure it is useful to test if the row is stored and has been changed (Store method)? Not sure about the perf, maybe DBIC does that (?).
DBIC does indeed do that already. In fact, DBIC will only update changed columns and skipped those that are unchanged.
- IsStored should be InStorage or completely removed, if useless.
Changed to InStorage
- Delete could call delete in an eval, to avoid a call to in_storage
This one I did not implement, in_storage is really just a property to I believe it is quicker than using eval. We can always run some tests to see which is really faster.
- what about 2 constructors? new and new_from_rs? The ResultSet should not be create if not used later
Done!
- what about a _result method? It would return ->{_result} if already exists, otherwise create it. All occurrences to ->{result} will be replaced by ->_result
Done! Also done for _columns as well since it depends on _result -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #32004|0 |1 is obsolete| | --- Comment #8 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 32007 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32007&action=edit Bug 13019 - Add base classes on which to build Koha objects The idea behind this is to have a pair of base classes on which to build our new generation of Koha objects. Koha::Object is a base class, which in it's most basic form, is to represent a row in a table. For example, Koha::Borrower inherits from Koha::Object. So too could Koha::Biblio and Koha::Item for example. Koha::Objects is to represent a way to fetch and manipulate sets of objects. For example, Koha::Borrowers has a method to get a Koha::Borrower object by id and a method to search for an get a list of Koha::Borrower objects. Right now Koha::Objects has only the essentials but can easily be extended and those enhancements will be passed down to all the child classes based on it. By using these classes as a base, we will add consistency to our code, allow us to keep our code DRY, reduce bugs, and encapsulate our database access among other benefits. Test Plan: 1) Apply this patch 2) prove t/Object.t t/db_dependent/Object.t t/db_dependent/Objects.t -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #32007|0 |1 is obsolete| | --- Comment #9 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 32008 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32008&action=edit Bug 13019 - Add base classes on which to build Koha objects The idea behind this is to have a pair of base classes on which to build our new generation of Koha objects. Koha::Object is a base class, which in it's most basic form, is to represent a row in a table. For example, Koha::Borrower inherits from Koha::Object. So too could Koha::Biblio and Koha::Item for example. Koha::Objects is to represent a way to fetch and manipulate sets of objects. For example, Koha::Borrowers has a method to get a Koha::Borrower object by id and a method to search for an get a list of Koha::Borrower objects. Right now Koha::Objects has only the essentials but can easily be extended and those enhancements will be passed down to all the child classes based on it. By using these classes as a base, we will add consistency to our code, allow us to keep our code DRY, reduce bugs, and encapsulate our database access among other benefits. Test Plan: 1) Apply this patch 2) prove t/Object.t t/db_dependent/Object.t t/db_dependent/Objects.t -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #32008|0 |1 is obsolete| | --- Comment #10 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 32009 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32009&action=edit Bug 13019 - Add base classes on which to build Koha objects The idea behind this is to have a pair of base classes on which to build our new generation of Koha objects. Koha::Object is a base class, which in it's most basic form, is to represent a row in a table. For example, Koha::Borrower inherits from Koha::Object. So too could Koha::Biblio and Koha::Item for example. Koha::Objects is to represent a way to fetch and manipulate sets of objects. For example, Koha::Borrowers has a method to get a Koha::Borrower object by id and a method to search for an get a list of Koha::Borrower objects. Right now Koha::Objects has only the essentials but can easily be extended and those enhancements will be passed down to all the child classes based on it. By using these classes as a base, we will add consistency to our code, allow us to keep our code DRY, reduce bugs, and encapsulate our database access among other benefits. Test Plan: 1) Apply this patch 2) prove t/Object.t t/db_dependent/Object.t t/db_dependent/Objects.t -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #32009|0 |1 is obsolete| | --- Comment #11 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 32042 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32042&action=edit Bug 13019 - Add base classes on which to build Koha objects The idea behind this is to have a pair of base classes on which to build our new generation of Koha objects. Koha::Object is a base class, which in it's most basic form, is to represent a row in a table. For example, Koha::Borrower inherits from Koha::Object. So too could Koha::Biblio and Koha::Item for example. Koha::Objects is to represent a way to fetch and manipulate sets of objects. For example, Koha::Borrowers has a method to get a Koha::Borrower object by id and a method to search for an get a list of Koha::Borrower objects. Right now Koha::Objects has only the essentials but can easily be extended and those enhancements will be passed down to all the child classes based on it. By using these classes as a base, we will add consistency to our code, allow us to keep our code DRY, reduce bugs, and encapsulate our database access among other benefits. Test Plan: 1) Apply this patch 2) prove t/Object.t t/db_dependent/Object.t t/db_dependent/Objects.t -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #32042|0 |1 is obsolete| | --- Comment #12 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 32043 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32043&action=edit Bug 13019 - Add base classes on which to build Koha objects The idea behind this is to have a pair of base classes on which to build our new generation of Koha objects. Koha::Object is a base class, which in it's most basic form, is to represent a row in a table. For example, Koha::Borrower inherits from Koha::Object. So too could Koha::Biblio and Koha::Item for example. Koha::Objects is to represent a way to fetch and manipulate sets of objects. For example, Koha::Borrowers has a method to get a Koha::Borrower object by id and a method to search for an get a list of Koha::Borrower objects. Right now Koha::Objects has only the essentials but can easily be extended and those enhancements will be passed down to all the child classes based on it. By using these classes as a base, we will add consistency to our code, allow us to keep our code DRY, reduce bugs, and encapsulate our database access among other benefits. Test Plan: 1) Apply this patch 2) prove t/Object.t t/db_dependent/Object.t t/db_dependent/Objects.t -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #32043|0 |1 is obsolete| | --- Comment #13 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 32045 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32045&action=edit Bug 13019 - Add base classes on which to build Koha objects The idea behind this is to have a pair of base classes on which to build our new generation of Koha objects. Koha::Object is a base class, which in it's most basic form, is to represent a row in a table. For example, Koha::Borrower inherits from Koha::Object. So too could Koha::Biblio and Koha::Item for example. Koha::Objects is to represent a way to fetch and manipulate sets of objects. For example, Koha::Borrowers has a method to get a Koha::Borrower object by id and a method to search for an get a list of Koha::Borrower objects. Right now Koha::Objects has only the essentials but can easily be extended and those enhancements will be passed down to all the child classes based on it. By using these classes as a base, we will add consistency to our code, allow us to keep our code DRY, reduce bugs, and encapsulate our database access among other benefits. Test Plan: 1) Apply this patch 2) prove t/Object.t t/db_dependent/Object.t t/db_dependent/Objects.t -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #32045|0 |1 is obsolete| | --- Comment #14 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 32056 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32056&action=edit Bug 13019 - Add base classes on which to build Koha objects The idea behind this is to have a pair of base classes on which to build our new generation of Koha objects. Koha::Object is a base class, which in it's most basic form, is to represent a row in a table. For example, Koha::Borrower inherits from Koha::Object. So too could Koha::Biblio and Koha::Item for example. Koha::Objects is to represent a way to fetch and manipulate sets of objects. For example, Koha::Borrowers has a method to get a Koha::Borrower object by id and a method to search for an get a list of Koha::Borrower objects. Right now Koha::Objects has only the essentials but can easily be extended and those enhancements will be passed down to all the child classes based on it. By using these classes as a base, we will add consistency to our code, allow us to keep our code DRY, reduce bugs, and encapsulate our database access among other benefits. Test Plan: 1) Apply this patch 2) prove t/Object.t t/db_dependent/Object.t t/db_dependent/Objects.t -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #32056|0 |1 is obsolete| | --- Comment #15 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 32059 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32059&action=edit Bug 13019 - Add base classes on which to build Koha objects The idea behind this is to have a pair of base classes on which to build our new generation of Koha objects. Koha::Object is a base class, which in it's most basic form, is to represent a row in a table. For example, Koha::Borrower inherits from Koha::Object. So too could Koha::Biblio and Koha::Item for example. Koha::Objects is to represent a way to fetch and manipulate sets of objects. For example, Koha::Borrowers has a method to get a Koha::Borrower object by id and a method to search for an get a list of Koha::Borrower objects. Right now Koha::Objects has only the essentials but can easily be extended and those enhancements will be passed down to all the child classes based on it. By using these classes as a base, we will add consistency to our code, allow us to keep our code DRY, reduce bugs, and encapsulate our database access among other benefits. Test Plan: 1) Apply this patch 2) prove t/Object.t t/db_dependent/Object.t t/db_dependent/Objects.t -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #32059|0 |1 is obsolete| | --- Comment #16 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 32065 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32065&action=edit Bug 13019 - Add base classes on which to build Koha objects The idea behind this is to have a pair of base classes on which to build our new generation of Koha objects. Koha::Object is a base class, which in it's most basic form, is to represent a row in a table. For example, Koha::Borrower inherits from Koha::Object. So too could Koha::Biblio and Koha::Item for example. Koha::Objects is to represent a way to fetch and manipulate sets of objects. For example, Koha::Borrowers has a method to get a Koha::Borrower object by id and a method to search for an get a list of Koha::Borrower objects. Right now Koha::Objects has only the essentials but can easily be extended and those enhancements will be passed down to all the child classes based on it. By using these classes as a base, we will add consistency to our code, allow us to keep our code DRY, reduce bugs, and encapsulate our database access among other benefits. Test Plan: 1) Apply this patch 2) prove t/Object.t t/db_dependent/Object.t t/db_dependent/Objects.t -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #32065|0 |1 is obsolete| | --- Comment #17 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 32068 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32068&action=edit Bug 13019 - Add base classes on which to build Koha objects The idea behind this is to have a pair of base classes on which to build our new generation of Koha objects. Koha::Object is a base class, which in it's most basic form, is to represent a row in a table. For example, Koha::Borrower inherits from Koha::Object. So too could Koha::Biblio and Koha::Item for example. Koha::Objects is to represent a way to fetch and manipulate sets of objects. For example, Koha::Borrowers has a method to get a Koha::Borrower object by id and a method to search for an get a list of Koha::Borrower objects. Right now Koha::Objects has only the essentials but can easily be extended and those enhancements will be passed down to all the child classes based on it. By using these classes as a base, we will add consistency to our code, allow us to keep our code DRY, reduce bugs, and encapsulate our database access among other benefits. Test Plan: 1) Apply this patch 2) prove t/Object.t t/db_dependent/Object.t t/db_dependent/Objects.t -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|unspecified |master -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com --- Comment #18 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- I have been playing around allot with this lately and my general feeling is that it's pretty workable already and a viable route to using proper objects in Koha. Whilst it has allot of further enhancements required to make it 'really' useful, it's in a good state as is for a first step as is. I'll happily QA if someone else wants to signoff.. or I can signoff if another of the QA team is happy expending the effort of getting to grips with it ;) My general thoughts as a whole follow: I still feel the objects kinda of lock away allot of the advantages of dbic behind an abstraction that sometimes detracts rather than enhances; but with our debates going on and on I'de rather see Koha moving forward than stagnating in debate territory. The only other option I see perhaps working is setting a bunch of coding rules/guidelines really pushing people in the direction of consistent objects. My real feeling after all this is that we're doing way too much logical work in our .pl's.. if it's logic based at all, it really belongs in a .pm where it can be fully tested ;-) ! (Though I'de love to see Services in PM's tested, and UI in Angular Tested as a future route.. I'll be posting up some demo code for such soon) This one is holding up bug 12892 now, so I'd really like to encourage some additional input, signoff and qa thought go into it asap :) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |10363 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 --- Comment #19 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Some questions after looking at the code: Clean code on itself, still needs more documentation. Is the idea now to allow DBIC methods ONLY in Koha::Object and Objects? This is currently not achieved if you need to pass in a resultset. Do we enforce its use in order to prevent chaos? This means creating a lot of new modules rightaway.. Having new and new_from_dbic was qualified above somewhere as a good idea. Why? I would merge them. Let new figure out what it receives. [If you want to encapsulate dbic btw, do not use that name when constructing your object.] Instead of passing the resultset, you could add a find method in the object class or implement the same via the new constructor. Something like: my $borrower= Koha::Borrower->find( $number ); or: my $borrower= Koha::Borrower->new( $number ); New c/should see that this is different from passing { number => .., name => ..} -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de Depends on| |11703 --- Comment #20 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- What is needed to get this moving again? Kyle, could you take a look at the last 2 comments? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #32068|0 |1 is obsolete| | --- Comment #21 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 33418 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33418&action=edit Bug 13019 - Add base classes on which to build Koha objects The idea behind this is to have a pair of base classes on which to build our new generation of Koha objects. Koha::Object is a base class, which in it's most basic form, is to represent a row in a table. For example, Koha::Borrower inherits from Koha::Object. So too could Koha::Biblio and Koha::Item for example. Koha::Objects is to represent a way to fetch and manipulate sets of objects. For example, Koha::Borrowers has a method to get a Koha::Borrower object by id and a method to search for an get a list of Koha::Borrower objects. Right now Koha::Objects has only the essentials but can easily be extended and those enhancements will be passed down to all the child classes based on it. By using these classes as a base, we will add consistency to our code, allow us to keep our code DRY, reduce bugs, and encapsulate our database access among other benefits. Test Plan: 1) Apply this patch 2) prove t/Object.t t/db_dependent/Object.t t/db_dependent/Objects.t Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> http://bugs.koha-community.org/show_bug.cgi?id=10363 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #33418|0 |1 is obsolete| | --- Comment #22 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 33419 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33419&action=edit Bug 13019 - Add base classes on which to build Koha objects The idea behind this is to have a pair of base classes on which to build our new generation of Koha objects. Koha::Object is a base class, which in it's most basic form, is to represent a row in a table. For example, Koha::Borrower inherits from Koha::Object. So too could Koha::Biblio and Koha::Item for example. Koha::Objects is to represent a way to fetch and manipulate sets of objects. For example, Koha::Borrowers has a method to get a Koha::Borrower object by id and a method to search for an get a list of Koha::Borrower objects. Right now Koha::Objects has only the essentials but can easily be extended and those enhancements will be passed down to all the child classes based on it. By using these classes as a base, we will add consistency to our code, allow us to keep our code DRY, reduce bugs, and encapsulate our database access among other benefits. Test Plan: 1) Apply this patch 2) prove t/Object.t t/db_dependent/Object.t t/db_dependent/Objects.t Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 --- Comment #23 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 33420 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33420&action=edit Bug 13019: (follow-up) Remove smartmatch operator This patch also adds 1 test. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 --- Comment #24 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #23) Jonathan, Any thoughts on comments 18 and 19? Marcel -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 --- Comment #25 from Jonathan Druart <jonathan.druart@biblibre.com> --- (In reply to M. de Rooy from comment #24)
(In reply to Jonathan Druart from comment #23) Jonathan, Any thoughts on comments 18 and 19?
As Martin said, this is the first step to get modules in Koha using DBIC with a common way. It will allow developers to easily instantiate objects in pl script. We already discussed about the abstraction layer and I agree with it. Moreover Kyle fixed/implemented my concerns (comments 2 and 4).
Is the idea now to allow DBIC methods ONLY in Koha::Object and Objects? This is currently not achieved if you need to pass in a resultset. Do we enforce its use in order to prevent chaos? This means creating a lot of new modules rightaway..
Yes of course. We will only add methods in Koha::Object[s] if needed and make sense. Create 1 module per class in the Koha namespace is the logical consequence of this patch, it's not a problem.
Having new and new_from_dbic was qualified above somewhere as a good idea. Why?
The first implementation of the constructor was uselessly complicated. I believe this will add readability. We already iterated several times with Kyle and other peoples involved, now it's the time to see something pushed and go further. It's always possible to improve this code later if it doesn't match our needs or cause problems. -- You are receiving this mail because: You are watching all bug changes.
Yes of course. We will only add methods in Koha::Object[s] if needed and make sense. This sounds like a misunderstanding. If it would be so obvious to not have DBIx code in a perl script (or unit test!), why does Borrower.t contain constructs
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 --- Comment #26 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #25) Please be assured that I appreciate your work and eagerness to get things into Koha. I also know that it can be somewhat frustrating to see things develop rather slow. Since this is meant to be a very fundamental object, I would not recommend hurry here btw. like: my $borrower = Koha::Database->new()->schema()->resultset('Borrower')->find( $borrowernumber ); Just replace it by something like Koha::Borrower->find( $borrowernumber ) or even extend new with similar functionality, as commented before. This would make new_from_dbic as external method (!! -- could be private though with an underscore) and passing resultsets superfluous. Leave the result set stuff in the objects. Also note that Koha::Borrowers->new()->find is not really elegant. Why not offer both Koha::Borrower->find and Koha::Borrowers->find without going through new? With some minimal changes in that direction, I would be inclined to pass qa.. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|12892 | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 --- Comment #27 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to M. de Rooy from comment #26)
(In reply to Jonathan Druart from comment #25) Please be assured that I appreciate your work and eagerness to get things into Koha. I also know that it can be somewhat frustrating to see things develop rather slow. Since this is meant to be a very fundamental object, I would not recommend hurry here btw.
I assure you, we haven't hurried ; ) This has taken months of discussion and proofs of concept so far!
Yes of course. We will only add methods in Koha::Object[s] if needed and make sense. This sounds like a misunderstanding. If it would be so obvious to not have DBIx code in a perl script (or unit test!), why does Borrower.t contain constructs like: my $borrower = Koha::Database->new()->schema()->resultset('Borrower')->find( $borrowernumber );
It's just a way to test that our objects are actually making changes to the database. We don't want to test our objects by using our objects!
Just replace it by something like Koha::Borrower->find( $borrowernumber ) or even extend new with similar functionality, as commented before. This would make new_from_dbic as external method (!! -- could be private though with an underscore) and passing resultsets superfluous. Leave the result set stuff in the objects.
new_from_dbic will probably never be used in actual Koha code, but it needs to be external for the objects to use. You should never see new_from_dbic used in a pl file.
Also note that Koha::Borrowers->new()->find is not really elegant. Why not offer both Koha::Borrower->find and Koha::Borrowers->find without going through new?
Right now those methods are not static. That's an improvement we can work on over time! It's at least more elegant than Koha::Database->new()->schema()->resultset('Borrower')->find( $borrowernumber ); don't you think? ; ) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 --- Comment #28 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 33797 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33797&action=edit Bug 13019 [QA Followup] - Allow find() and search() to be called as static methods -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 --- Comment #29 from Kyle M Hall <kyle@bywatersolutions.com> --- This latest followup should take care of this!
Also note that Koha::Borrowers->new()->find is not really elegant. Why not offer both Koha::Borrower->find and Koha::Borrowers->find without going through new?
With some minimal changes in that direction, I would be inclined to pass qa..
-- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |13030 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 --- Comment #30 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Kyle M Hall from comment #27)
It's just a way to test that our objects are actually making changes to the database. We don't want to test our objects by using our objects!
I think that would be the only legitimate excuse :) But in that case it should not be in Borrowers. However, since our object is just a very smaller wrapper around DBIC, we are actually comparing DBIC with DBIC. So no big deal. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 --- Comment #31 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 33840 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33840&action=edit Bug 13019: [QA Follow-up] Rename new_from_dbic and few typos Since new_from_dbic is not meant as a public method, this patch adds a prefix to the name of this internal routine. For the same reason I removed it from t/Borrower.t. Removed one use overload-line in Objects (not used). Resolved a few typos. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #33419|0 |1 is obsolete| | --- Comment #32 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 33841 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33841&action=edit Bug 13019 - Add base classes on which to build Koha objects The idea behind this is to have a pair of base classes on which to build our new generation of Koha objects. Koha::Object is a base class, which in it's most basic form, is to represent a row in a table. For example, Koha::Borrower inherits from Koha::Object. So too could Koha::Biblio and Koha::Item for example. Koha::Objects is to represent a way to fetch and manipulate sets of objects. For example, Koha::Borrowers has a method to get a Koha::Borrower object by id and a method to search for an get a list of Koha::Borrower objects. Right now Koha::Objects has only the essentials but can easily be extended and those enhancements will be passed down to all the child classes based on it. By using these classes as a base, we will add consistency to our code, allow us to keep our code DRY, reduce bugs, and encapsulate our database access among other benefits. Test Plan: 1) Apply this patch 2) prove t/Object.t t/db_dependent/Object.t t/db_dependent/Objects.t Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #33420|0 |1 is obsolete| | --- Comment #33 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 33842 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33842&action=edit Bug 13019: (follow-up) Remove smartmatch operator This patch also adds 1 test. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #33797|0 |1 is obsolete| | --- Comment #34 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 33843 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33843&action=edit Bug 13019 [QA Followup] - Allow find() and search() to be called as static methods Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #33840|0 |1 is obsolete| | --- Comment #35 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 33844 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33844&action=edit Bug 13019: [QA Follow-up] Rename new_from_dbic and few typos Since new_from_dbic is not meant as a public method, this patch adds a prefix to the name of this internal routine. For the same reason I removed it from t/Borrower.t. Removed one use overload-line in Objects (not used). Resolved a few typos. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 --- Comment #36 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 33845 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33845&action=edit Bug 13019: Follow-up for find in Koha::Object This patch includes the proposal to add find as method to Koha::Object too. In the form implemented it supports class and object method calls. The test Borrower.t includes an additional test for this change. Test plan: Run t/Borrower.t and t/db_dependent/Borrower.t and Borrowers.t. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 --- Comment #37 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Kyle: Before proceeding, I would like to know your opinion on the last patch. It is a suggestion to add find on Object.pm. What do you think? Still another question: Is the return encode( 'UTF-8', $value ) really necessary? I have my doubts.. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #33845|0 |1 is obsolete| | --- Comment #38 from Kyle M Hall <kyle@bywatersolutions.com> --- Comment on attachment 33845 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33845 Bug 13019: Follow-up for find in Koha::Object I highly recommend we don't include this patch. All logic related to getting one or more objects should stay in the Objects class. That way we have uniformity and no confusion over how to call find. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #33841|0 |1 is obsolete| | Attachment #33842|0 |1 is obsolete| | Attachment #33843|0 |1 is obsolete| | Attachment #33844|0 |1 is obsolete| | --- Comment #39 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 33846 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33846&action=edit [SIGNED-OFF] Bug 13019 - Add base classes on which to build Koha objects The idea behind this is to have a pair of base classes on which to build our new generation of Koha objects. Koha::Object is a base class, which in it's most basic form, is to represent a row in a table. For example, Koha::Borrower inherits from Koha::Object. So too could Koha::Biblio and Koha::Item for example. Koha::Objects is to represent a way to fetch and manipulate sets of objects. For example, Koha::Borrowers has a method to get a Koha::Borrower object by id and a method to search for an get a list of Koha::Borrower objects. Right now Koha::Objects has only the essentials but can easily be extended and those enhancements will be passed down to all the child classes based on it. By using these classes as a base, we will add consistency to our code, allow us to keep our code DRY, reduce bugs, and encapsulate our database access among other benefits. Test Plan: 1) Apply this patch 2) prove t/Object.t t/db_dependent/Object.t t/db_dependent/Objects.t Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> 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.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 --- Comment #40 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 33847 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33847&action=edit [SIGNED-OFF] Bug 13019: (follow-up) Remove smartmatch operator This patch also adds 1 test. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> 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.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 --- Comment #41 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 33848 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33848&action=edit [SIGNED-OFF] Bug 13019 [QA Followup] - Allow find() and search() to be called as static methods 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.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 --- Comment #42 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 33849 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33849&action=edit [SIGNED-OFF] Bug 13019: [QA Follow-up] Rename new_from_dbic and few typos Since new_from_dbic is not meant as a public method, this patch adds a prefix to the name of this internal routine. For the same reason I removed it from t/Borrower.t. Removed one use overload-line in Objects (not used). Resolved a few typos. 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.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 --- Comment #43 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 33850 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33850&action=edit Bug 13019 [QA Followup] - Remove use of encode -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #33846|[SIGNED-OFF] Bug 13019 - |Bug 13019 - Add base description|Add base classes on which |classes on which to build |to build Koha objects |Koha objects -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #33847|[SIGNED-OFF] Bug 13019: |Bug 13019: (follow-up) description|(follow-up) Remove |Remove smartmatch operator |smartmatch operator | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #33848|[SIGNED-OFF] Bug 13019 [QA |Bug 13019 [QA Followup] - description|Followup] - Allow find() |Allow find() and search() |and search() to be called |to be called as static |as static methods |methods -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #33849|[SIGNED-OFF] Bug 13019: [QA |Bug 13019: [QA Follow-up] description|Follow-up] Rename |Rename new_from_dbic and |new_from_dbic and few typos |few typos -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #33850|0 |1 is obsolete| | --- Comment #44 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 33852 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33852&action=edit Bug 13019 [QA Followup] - Remove use of encode Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #45 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- QA Comment: Some minor concerns have been addressed in the meantime. This topic waits for some time now and has been discussed at various times, but without too much progress. I think this set of patches is an interesting starting point now. The Borrower example submitted now is too small to really show the advantages/disadvantages of this approach. The code looks good. No complaints from qa tools. I think we should push this now and first build a object on it, used in real Koha code (not just some tests). After evaluating that "exercise", we can think of adapting the coding guidelines. Note that the object still need some further documentation, but I do not mind doing that in follow-ups later. Let's get this further now. Passed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 --- Comment #46 from Jonathan Druart <jonathan.druart@biblibre.com> --- (In reply to M. de Rooy from comment #45)
I think we should push this now and first build a object on it, used in real Koha code (not just some tests). After evaluating that "exercise", we can think of adapting the coding guidelines.
See bug 10363. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 --- Comment #47 from David Cook <dcook@prosentient.com.au> --- Comment on attachment 33846 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33846 Bug 13019 - Add base classes on which to build Koha objects Review of attachment 33846: --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=13019&attachment=33846) ----------------------------------------------------------------- ::: Koha/Object.pm @@ +136,5 @@
+=head3 $object->delete(); + +Removes the object from storage. + +Returns:
I can see returning true on success, but shouldn't we throw an exception if the deletion fails or if the object was never in storage? I would think it would make more sense to try a proper exception using die(), and then catch that when deleting an object using eval() or Try::Tiny (which I think is a prereq of DBIC anyway). Mind you, off the top of my head, I'm not sure what exceptions DBIC throws if a delete fails or if the object was never there to begin with... ::: Koha/Objects.pm @@ +187,5 @@
+ +sub _wrap { + my ( $self, @dbic_rows ) = @_; + + my @objects = map { $self->object_class()->new_from_dbic( $_ ) } @dbic_rows;
Mmm, I see... so you use Koha::Objects for fetching objects from the database... and it wraps the resultset with the applicable object based on Koha::Object... -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #48 from David Cook <dcook@prosentient.com.au> --- Looking over this again... I don't know if we're implementing Koha::Objects in the best way possible. After all, every Koha::Objects subclass is basically just going to consist of the following: 44 sub type { 45 return 'Borrower'; 46 } 47 48 sub object_class { 49 return 'Koha::Borrower'; 50 } That seems like unnecessary duplication in my mind. Why not follow the DBIC model, and go with something like the following? my $Borrowers= Koha::Objects->new('Borrower'); my @objects = $Borrowers->search({surname => 'Jones'}); I suppose the limitation of that is that you can't have custom methods for the group of objects, but would you need them? I imagine all the custom methods are going to be at the individual object level. -- I must admit, after looking through DBIC more and more, I'm less sure of the encapsulation method. I wonder how much functionality we'll use by encapsulating the DBIC objects in Koha objects. Yet, DBIC objects on their own don't have enough functionality to be all we need. Or do they? Maybe we should keep DBIC objects as data objects, and then have other modules/objects for applying business logic to those objects. In terms of CRUD, DBIC is fine the way it is. But let's think about a Koha::Biblio object... it could have methods for importing, exporting, filtering, extracting data from a Koha::Schema::Biblio object or a Koha::Schema::Deletedbiblio object. Koha::Biblio::new($biblionumber); or maybe Koha::Biblio::new($dbic_object); #not dissimilar to using inheritance, except that you can use Biblio or Deletedbiblio in this case my $output = Koha::Biblio::format("opac","marcxml"); #detail view my $output = Koha::Biblio::format("opac","isomarc"); #exporting as isomarc my $output = Koha::Biblio::format("intranet","marcxml"); #detail view my $output = Koha::Biblio::format("intranet","dc"); #detail view as dublin core -- In my latest OAI stuff, I have DBIC objects for OAI records and OAI repositories. I then have a Harvester.pm (which has methods for getting active repositories, querying the repositories, queueing the records, etc.) and an Importer.pm (gets the queued records, analyzes them, transforms metadata, calls the internal C4::Biblio API for adding records to the database). Biblio is actually pretty interesting... because you can get biblios in all sorts of ways. From Zebra, from MySQL, from Z39.50, from OAI, from importing through other tools. You should be able to feed ISOMARC or MARCXML into Koha::Biblio and have that take care of everything thereafter. Actually, Koha::Biblio might not even be the most appropriate name in that case. Maybe it should be Koha::Record, and that becomes the one and only access point for that record. I suppose though... if you were doing a DBIC search, you'd have to feed your result into Koha::Record before you could work on it... and you might be tempted to avoid Koha::Record. Whereas if you're using Koha::Records or Koha::Biblios and that returns the DBIC object wrapped in a Koha object... then you're forced to use that (unless you bypass Koha::Biblios and just use DBIC on its own). But what is a "biblio"... when do we use it... 1) We add biblios to the catalogue. We might do this by hand, by Z39.50, from OAI, or some other means. But basically, we have a metadata record in MARC format. 2) We search for biblios in the catalogue We query Zebra and it returns biblio records. We see a list of search results. 3) We examine individual records We click on the results and look at the records 4) We edit the metadata records We load the MARCXML into a form that can be edited by humans, and then save it again into the relational database (where it gets stored in biblioitems.marcxml, and processed into the biblio and biblioitems tables) 5) We export records (to Zebra, to browsers for download, via OAI, via Z39.50, etc) We output the MARCXML to other sources -- I'm sure there are other use cases of biblios, but the thing I continually notice is that most of these probably wouldn't need the DBIC object. Adding and editing would, as the MARCXML would need to be stored in biblioitems.marcxml, and the MARCXML would need to be parsed into the other database fields. Most of the `biblio` and `biblioitems` fields are calculated fields rather than data in their own right. -- That all being said... I like the idea of the code being more DRY. So maybe it does make sense to have a Koha::Object base class that defines methods for adding, modifying, and deleting database data. Hmm... but I'm not sure that can work either. Thinking about "Biblio" or "Borrower", when you delete one of these, you don't want to just nuke it from the database. You want to move it to the "Deletedbiblio" or "Deletedborrower" then nuke it. I suppose you could always have standard methods and then override them in these cases. Likewise for moving issues into old_issues, reserves into old_reserves, etc. We'd probably always want a single Acquisitions::Budgets object even though there are multiple tables. -- Ultimately... I don't think the approach of wrapping DBIC objects with Koha objects will work. I think it would work in some cases, but I think it wouldn't be flexible enough to accommodate all cases. I think they get a bit repetitive and automagical at times as well, which might be confusing. That said, maybe we should try them out a bit in the wild and see how it goes. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |In Discussion --- Comment #49 from David Cook <dcook@prosentient.com.au> --- Actually, I'm going to put this one to "in discussion", as I don't think it's ready to be pushed to master. I think we really should spend some more time thinking through how we want the next generation of Koha::* to progress. Perhaps this should find its way to the listserv as well, as I think it's a conversation that is applicable to more than just the 5 or 6 of us. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 --- Comment #50 from Jonathan Druart <jonathan.druart@biblibre.com> --- (In reply to David Cook from comment #49)
Actually, I'm going to put this one to "in discussion", as I don't think it's ready to be pushed to master. I think we really should spend some more time thinking through how we want the next generation of Koha::* to progress.
Perhaps this should find its way to the listserv as well, as I think it's a conversation that is applicable to more than just the 5 or 6 of us.
I personally don't want to discuss this implementation for the following reasons: 1/ We already have discussed (or at least tried) for years. 2/ We already have different proposals (At least from Kyle, Yohann and me). 3/ This code is functional and we will able to improve it according to our further needs. 4/ Everybody knows that putting a bug report to the 'in discussion' status without any other actions (mail on mailing list, discussion on #koha, etc) don't permit to move it forward. I strongly believe that the next step is to get the RM point of view. You can provide a patch on another report to improve the code and show us what could be better, that would be great :) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 --- Comment #51 from David Cook <dcook@prosentient.com.au> --- (In reply to Jonathan Druart from comment #50)
I personally don't want to discuss this implementation for the following reasons: 1/ We already have discussed (or at least tried) for years. 2/ We already have different proposals (At least from Kyle, Yohann and me). 3/ This code is functional and we will able to improve it according to our further needs. 4/ Everybody knows that putting a bug report to the 'in discussion' status without any other actions (mail on mailing list, discussion on #koha, etc) don't permit to move it forward.
I strongly believe that the next step is to get the RM point of view.
You can provide a patch on another report to improve the code and show us what could be better, that would be great :)
Fair enough. Maybe the next step is to get the RM point of view. While I do have ideas for what I think would be better code and a better proposal, I think it would need to be a more concerted team effort (admittedly, that has historically been extraordinarily difficult to achieve). As for individual work, I'm not willing to dedicate the time in my personal life at the moment, and I don't have the time to do it professionally, so I suppose it's not fair for me to be too critical if I can't provide any work myself. Feel free to switch it back to Passed QA. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Passed QA --- Comment #52 from Kyle M Hall <kyle@bywatersolutions.com> --- I agree with Jonathan. This has been discussed in great detail. This *has* been a team effort with much input from Jonathan and Martin. This is also the third iteration of this idea that I've proposed. It would be great if we could get input from every single person, but there will always be opposing opinions on how things should be done. At this point, I ( and probably others ) just want something, anything, we can get and use. This does not by any means prevent us from making improvements in the future! But we need to start somewhere. (In reply to David Cook from comment #51)
(In reply to Jonathan Druart from comment #50)
I personally don't want to discuss this implementation for the following reasons: 1/ We already have discussed (or at least tried) for years. 2/ We already have different proposals (At least from Kyle, Yohann and me). 3/ This code is functional and we will able to improve it according to our further needs. 4/ Everybody knows that putting a bug report to the 'in discussion' status without any other actions (mail on mailing list, discussion on #koha, etc) don't permit to move it forward.
I strongly believe that the next step is to get the RM point of view.
You can provide a patch on another report to improve the code and show us what could be better, that would be great :)
Fair enough. Maybe the next step is to get the RM point of view.
While I do have ideas for what I think would be better code and a better proposal, I think it would need to be a more concerted team effort (admittedly, that has historically been extraordinarily difficult to achieve). As for individual work, I'm not willing to dedicate the time in my personal life at the moment, and I don't have the time to do it professionally, so I suppose it's not fair for me to be too critical if I can't provide any work myself.
Feel free to switch it back to Passed QA.
-- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |11431 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 --- Comment #53 from David Cook <dcook@prosentient.com.au> --- (In reply to Kyle M Hall from comment #52)
I agree with Jonathan. This has been discussed in great detail. This *has* been a team effort with much input from Jonathan and Martin. This is also the third iteration of this idea that I've proposed. It would be great if we could get input from every single person, but there will always be opposing opinions on how things should be done. At this point, I ( and probably others ) just want something, anything, we can get and use. This does not by any means prevent us from making improvements in the future! But we need to start somewhere.
With deference to Jonathan and Martin, I was referring to the overarching community when I wrote "team". I know there has been some discussion here and on the lists, but I don't think there was ever any sort of consensus on the best way to move forward. That said, I know you have proposed a lot of different ideas, and kudos for that. I also know that the perfect is the enemy of the good. While I would like to avoid technical debt, I suppose it can't hurt to try out multiple ideas. Since the majority of code also comes from yourself and Jonathan, it probably makes sense to use a system that you both find useful. So long as Koha is working well for librarians and patrons, I can't really complain. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 --- Comment #54 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 35813 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=35813&action=edit Bug 13019 [QA Followup] - Allow chaining By returning the object itself instead of a boolean, we can chain methods together while retaining the exact same functionality. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 --- Comment #55 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 35814 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=35814&action=edit Bug 13019 [QA Followup] - Fix stale unit test -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #35814|0 |1 is obsolete| | --- Comment #56 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 35815 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=35815&action=edit Bug 13019 [QA Followup] - Fix stale unit test -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master CC| |tomascohen@gmail.com --- Comment #57 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Patchset pushed to master. Thanks Kyle, Jonathan and Marcel! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 --- Comment #58 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to Tomás Cohen Arazi from comment #57)
Patchset pushed to master.
Thanks Kyle, Jonathan and Marcel!
Documentation for using Koha::Object and Koha::Objects can be found on the community wiki ( http://wiki.koha-community.org/wiki/Koha_Objects ). -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 --- Comment #59 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 35980 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=35980&action=edit Store list of objects as needed -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #35980|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 --- Comment #60 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Kyle, maybe a new bug report? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |13719 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |13726 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |13738 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |olli-antti.kivilahti@jns.fi --- Comment #61 from Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> --- Just wanted to say that good job with this one Kyle! I just subclassed a subclass of one of your Koha::Objects :) Works like a charm! I updated the wiki page for that regard. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|13030 | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |14544 Referenced Bugs: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14544 [Bug 14544] Move the list related code to Koha::Virtualshelves -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |14616 Referenced Bugs: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14616 [Bug 14616] Introducing Koha::Object subclasses -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13019 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |15446 Referenced Bugs: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15446 [Bug 15446] Koha::Object[s]->type should be renamed to _type to avoid conflict with column name -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org