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

[Jonathan Druart]

For the record:
The AUTOLOAD is on bug 17091 (Add AUTOLOAD to Koha::Objects)
and the DBIC vs Koha::Object returned value for Koha::Virtualshelf
methods is fixed on bug 17094 (Methods of Koha::Object[s]-based
classed should not return DBIx::Class objects)

2016-06-26 11:04 GMT+01:00 Jonathan Druart
<jonathan.druart at bugs.koha-community.org>:
> 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