[Koha-devel] Koha::Object[s] improvements - call for discussions
Kyle Hall
kyle.m.hall at gmail.com
Sun Jun 26 13:05:58 CEST 2016
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 at 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 at 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/
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.koha-community.org/pipermail/koha-devel/attachments/20160626/33edc22e/attachment-0001.html>
More information about the Koha-devel
mailing list