Jonthan, I love these ideas and definitely think they would be major improvements to Koha::Object(s). I would like to toss something else in as well. A while back I submitted bug 15759 as a concept: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15759 This bug does some pretty awesome things imo: 1) it let's us use a Koha::Object as if it is a hashref! So, if we have the following: my $patron = Koha::Patrons->find( $borrowernumber ); the following are functionally equivilent: my $surname = $patron->surname(); my $surname = $patron->{surname}; So are these: $patron->surname( $surname ); $paron->{surname} = $surname; 2) It prevents bugs One problem right now is you may accidentally use $patron->{surname} when you should have used $patron->surname(). Perl does not report any errors if you do this so the bug can be hard to track down. Because of the above this is no longer an issue! 3) Most importantly, this let's us drop a Koha::Object into a script where we are already using a hashref with no further changes! Any scripts that already use hashrefs will now be able to use Koha::Objects instead! There is a downside, we need to go through the existing Koha::Object(s) classes and change any references to $self where it is treated as a hashref and update them. So a line like this: $self->{_result} = .... to this $$self->{_result} = .... It is a trivial change though that could easily be done by a script at the time the bug would be pushed to master. What does everyone think? Would this be a worthwhile improvement? Kyle <https://secure2.convio.net/cffh/site/Donation2?df_id=1395&FR_ID=4715&PROXY_ID=2706639&PROXY_TYPE=20&1395.donation=form1&s_src=CHORUS&s_subsrc=CHAADOEB> http://www.kylehall.info ByWater Solutions ( http://bywatersolutions.com ) Meadville Public Library ( http://www.meadvillelibrary.org ) Crawford County Federated Library System ( http://www.ccfls.org ) On Sun, Jun 26, 2016 at 6:04 AM, Jonathan Druart < jonathan.druart@bugs.koha-community.org> wrote:
Hi devs,
At the KohaCon16 some of us have briefly talked about the limitations of Koha::Object[s] we encounter when implementing new modules. Koha::Object[s] has been pushed to master less than 18 months ago and we already have more than 30 classes using it. It permits to uniformise our way to code, use powerful objects and is very easy for refactoring. I have been moving code from C4 to the Koha namespace and it's very handy to just have to write a concise module and automatically have the basic methods (CRUD and more).
However it seems that we need to clearly define what we need in a near future and adapt it quickly if it's needed.
= Rooms for Improvement = There are 3 main rooms for improvement: - DBIx::Class basic methods - DBIx::Class relationship accesses - Koha::Objects vs DBIx::Class objects
== DBIx::Class basic methods == If you want to access a method provided by DBIx::Class, like ->store, ->delete, ->is_changed, we need to create a silly method which just calls the same method on the DBIx::Class object. For instance, let's take Koha::Objects->is_changed sub is_changed { my ( $self, @columns ) = @_; return $self->_result()->is_changed(@columns); }
Possible solution: It would be possible to modify our AUTOLOAD method and check if it's a DBIX::Class method. If it is a valid one, let's call it, otherwise carp something.
== DBIx::Class relationships == From a Koha::Objects, the relationships provided by DBIx::Class will not be automatically generated. For instance, from a virtual shelf, you will not have access to the items on the shelf.
You will have to create a method which will call the DBIx::Class relevant relationship. sub get_contents { my ( $self ) = @_; my $contents = $self->{_result}->virtualshelfcontents; return $contents; } This piece of code adds a get_contents method to Koha::Virtualshelf and call the virtualshelfcontents relationship created by DBIx::Class. Note that this is finally good here, it is better to call have a get_contents method than virtualshelfcontents. But we may prefer to redefine it in the schema file (Koha::Schema::Result::Virtualshelve).
Possible solution: Same as previously, it would be possible to modify our AUTOLOAD method and check if it's a valid relationship.
== Koha::Objects vs DBIX::Class objects == We'd like to always manipulate the same kind of objects, to avoid confusion when reading the code. At the moment we already manipulate 2 different entities: hashref from C4 and objects from Koha::Object. It's confusing and not so easy to manage in the code. To add flexibility we have added a Koha::Object->unblessed method to transform the object to an hashref, it's not ideal but it ease the transition. Anyway, from a Koha::Object if you want to retrieve data linked to this object, you would like to get a Koha::Object object base, not a DBIx::Class object base. If we continue with the previous example, we will need to modify get_contents like this: sub get_contents { my ( $self ) = @_; my $rs = $self->{_result}->virtualshelfcontents; my $contents = Koha::Virtualshelfcontents->_new_from_dbic( $rs ); return $contents; } Basically ->new_from_dbic will just create an empty Koha::Virtualshelfcontents object and set the internal resultset. No query or extra processing should be done here.
I do not know if the changes suggested (and already implemented in some areas and patches in the queue) is consistent. It's not very elegant to read, but it looks safe on the performance side, despite the encapsulation problem we could see at first glance.
= Concerns = == API == Some people raised concerns about the new API to lean. But finally we stick on DBIx::Class method names, so it should not be to hard to learn for new devs. And... the modules in the Koha namespace are much more easier to read/understand than the legacy code in C4.
== ResultSet == It seems that writing the following is not obvious for everybody: # Create and store my $biblio = Koha::Biblio->new( { title => "a title", [other => $data] } ); $biblio->store;
# Retrieve data my $biblio = Koha::Biblios->find( $biblionumber ); my $biblio = Koha::Biblios->search( { title => "a title" });
# Access relationship my $items = $biblio->items; warn ref( $items ); # Koha::Items warn ref( $items->first ); # Koha::Item
This looks very easy to read for me, feel free to suggest something else.
= A complete example =
At the moment, the more complete example in master (and 16.05) is the Koha::Virtualshelf and Koha::Virtualshelves modules. It includes several search methods, overwrite of the store method and some complex methods as well as complete test coverage (t/db_dependent/Virtualshelves.t)
= Alternatives = If you think you can provide an alternative or an idea to improve/replace our Koha::Object[s] implementation, do not hesitate, I would be glad to review or even implement it!
If you want to discuss one of these point, add a new one, or have a specific question, please answer this email. We could talk about that during the next dev meeting as well.
Cheers, Jonathan _______________________________________________ Koha-devel mailing list Koha-devel@lists.koha-community.org http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/