[Koha-devel] Koha::Object[s] improvements - call for discussions
Kyle Hall
kyle.m.hall at gmail.com
Thu Jul 7 14:26:31 CEST 2016
Thanks for the input. The bug started out as just a way to make the
improper use of objects as hashrefs trigger an error, but evolved when I
thought of this possibility. We can still make that happen, it still
requires the blessed hashref to be converted to a blessed scalar ref and
the class files will need updated. If this is generally considered a good
idea, I can submit a patch.
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 Thu, Jul 7, 2016 at 7:25 AM, Alex Sassmannshausen <
alex.sassmannshausen at gmail.com> wrote:
> 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/
> >
> >
>
> _______________________________________________
> 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/20160707/bdc5b7c8/attachment-0001.html>
More information about the Koha-devel
mailing list