[Koha-devel] Koha::Object[s] improvements - call for discussions
Alex Sassmannshausen
alex.sassmannshausen at gmail.com
Thu Jul 7 13:25:48 CEST 2016
Hello
Throwing in my 2¢ here too…
Kyle Hall writes:
> Jonthan, I love these ideas and definitely think they would be major
> improvements to Koha::Object(s).
Agreed. My reservations I expressed at the hackfest were around us
losing the great documentation that is available with dbic by having
Koha::Objects wrappers around dbic. However, with that caveat in mind,
I think your suggested improvements seem solid. Especially as more dbic
documentation would become applicable through the transparent proxying
of dbic relationships and methods.
> 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
I will comment this on the bug too, but, to make it part of this email
thread:
I have some reservations about this approach.
> 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;
I fear this approach of intermingling hashrefs and objects will make
reading code harder, as it blurs boundaries between what should be
discrete entities: there is the object that represents a patron and then
there is a hashref that represents a patron. Both are valid
representations, but they are different things.
To aid code legibility, it makes sense IMHO, to be clear about what we
are using. The proposed change, while convenient, would obscure.
> 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!
I agree that silently continuing with an undef value is problematic. I
think if at all possible I would prefer to fix this problem though.
I think if a programmer mistakes an object for a hashref then that is an
mistake that the programmer needs to be aware of, rather than the
program silently correcting that mistake…
> 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!
I agree that it would help with the conversion to Koha::Objects, but I
think the cost would be to high: I think we'd end up making code that is
already tricky to understand even harder to understand, and would tempt
us to continue accumulating technical debt when we need to be reducing
that.
I appreciate all the work done by Jonathan (and others?) on C4
refactoring, in the process of shifting to Koha objects, and I think
that's the right way to continue.
Of course, I appreciate your work on proposing this change too — it's
way easier to sit on the sidelines and critique, like I am doing here
;-)
> Kyle
Best wishes,
Alex
>
> *
>
> 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/
>
>
More information about the Koha-devel
mailing list