[Koha-devel] Koha::Object[s] improvements - call for discussions

[Jonathan Druart]

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