<div dir="ltr">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.<div><br></div><div>Kyle</div></div><div class="gmail_extra"><br clear="all"><div><div class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div><a href="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" target="_blank"><img src="http://widgets.ch-or.us/badge/convio/cff/4715/2706639"></a><br></div><div><br></div><div><a href="http://www.kylehall.info" target="_blank">http://www.kylehall.info</a><br>ByWater Solutions ( <a href="http://bywatersolutions.com" target="_blank">http://bywatersolutions.com</a> )<br>Meadville Public Library ( <a href="http://www.meadvillelibrary.org" target="_blank">http://www.meadvillelibrary.org</a> )<br>Crawford County Federated Library System ( <a href="http://www.ccfls.org" target="_blank">http://www.ccfls.org</a> )<br></div></div></div></div></div></div>
<br><div class="gmail_quote">On Thu, Jul 7, 2016 at 7:25 AM, Alex Sassmannshausen <span dir="ltr"><<a href="mailto:alex.sassmannshausen@gmail.com" target="_blank">alex.sassmannshausen@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hello<br>
<br>
Throwing in my 2¢ here too…<br>
<span class=""><br>
Kyle Hall writes:<br>
<br>
> Jonthan, I love these ideas and definitely think they would be major<br>
> improvements to Koha::Object(s).<br>
<br>
</span>Agreed. My reservations I expressed at the hackfest were around us<br>
losing the great documentation that is available with dbic by having<br>
Koha::Objects wrappers around dbic. However, with that caveat in mind,<br>
I think your suggested improvements seem solid. Especially as more dbic<br>
documentation would become applicable through the transparent proxying<br>
of dbic relationships and methods.<br>
<span class=""><br>
> I would like to toss something else in as well. A while back I<br>
> submitted bug 15759 as a concept:<br>
> <a href="https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15759" rel="noreferrer" target="_blank">https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15759</a><br>
<br>
</span>I will comment this on the bug too, but, to make it part of this email<br>
thread:<br>
<br>
I have some reservations about this approach.<br>
<span class=""><br>
> 1) it let's us use a Koha::Object as if it is a hashref!<br>
><br>
> So, if we have the following:<br>
><br>
> my $patron = Koha::Patrons->find( $borrowernumber );<br>
><br>
> the following are functionally equivilent:<br>
><br>
> my $surname = $patron->surname();<br>
><br>
> my $surname = $patron->{surname};<br>
><br>
> So are these:<br>
><br>
> $patron->surname( $surname );<br>
><br>
> $paron->{surname} = $surname;<br>
<br>
</span>I fear this approach of intermingling hashrefs and objects will make<br>
reading code harder, as it blurs boundaries between what should be<br>
discrete entities: there is the object that represents a patron and then<br>
there is a hashref that represents a patron. Both are valid<br>
representations, but they are different things.<br>
<br>
To aid code legibility, it makes sense IMHO, to be clear about what we<br>
are using. The proposed change, while convenient, would obscure.<br>
<span class=""><br>
> 2) It prevents bugs<br>
><br>
> One problem right now is you may accidentally use $patron->{surname}<br>
> when you should have used $patron->surname().<br>
><br>
> Perl does not report any errors if you do this so the bug can be hard<br>
> to track down. Because of the above this is no longer an issue!<br>
<br>
</span>I agree that silently continuing with an undef value is problematic. I<br>
think if at all possible I would prefer to fix this problem though.<br>
<br>
I think if a programmer mistakes an object for a hashref then that is an<br>
mistake that the programmer needs to be aware of, rather than the<br>
program silently correcting that mistake…<br>
<span class=""><br>
> 3) Most importantly, this let's us drop a Koha::Object into a script<br>
> where we are already using a hashref with no further changes!<br>
><br>
> Any scripts that already use hashrefs will now be able to use<br>
> Koha::Objects instead!<br>
<br>
</span>I agree that it would help with the conversion to Koha::Objects, but I<br>
think the cost would be to high: I think we'd end up making code that is<br>
already tricky to understand even harder to understand, and would tempt<br>
us to continue accumulating technical debt when we need to be reducing<br>
that.<br>
<br>
I appreciate all the work done by Jonathan (and others?) on C4<br>
refactoring, in the process of shifting to Koha objects, and I think<br>
that's the right way to continue.<br>
Of course, I appreciate your work on proposing this change too — it's<br>
way easier to sit on the sidelines and critique, like I am doing here<br>
;-)<br>
<br>
> Kyle<br>
<br>
Best wishes,<br>
<br>
Alex<br>
<br>
><br>
> *<br>
<div class="HOEnZb"><div class="h5">><br>
> <a href="http://www.kylehall.info" rel="noreferrer" target="_blank">http://www.kylehall.info</a><br>
> ByWater Solutions ( <a href="http://bywatersolutions.com" rel="noreferrer" target="_blank">http://bywatersolutions.com</a> )<br>
> Meadville Public Library ( <a href="http://www.meadvillelibrary.org" rel="noreferrer" target="_blank">http://www.meadvillelibrary.org</a> )<br>
> Crawford County Federated Library System ( <a href="http://www.ccfls.org" rel="noreferrer" target="_blank">http://www.ccfls.org</a> )<br>
><br>
> On Sun, Jun 26, 2016 at 6:04 AM, Jonathan Druart<br>
> <<a href="mailto:jonathan.druart@bugs.koha-community.org">jonathan.druart@bugs.koha-community.org</a>> wrote:<br>
><br>
> Hi devs,<br>
><br>
> At the KohaCon16 some of us have briefly talked about the<br>
> limitations<br>
> of Koha::Object[s] we encounter when implementing new modules.<br>
> Koha::Object[s] has been pushed to master less than 18 months ago<br>
> and<br>
> we already have more than 30 classes using it.<br>
> It permits to uniformise our way to code, use powerful objects and<br>
> is<br>
> very easy for refactoring. I have been moving code from C4 to the<br>
> Koha<br>
> namespace and it's very handy to just have to write a concise<br>
> module<br>
> and automatically have the basic methods (CRUD and more).<br>
><br>
> However it seems that we need to clearly define what we need in a<br>
> near<br>
> future and adapt it quickly if it's needed.<br>
><br>
> = Rooms for Improvement =<br>
> There are 3 main rooms for improvement:<br>
> - DBIx::Class basic methods<br>
> - DBIx::Class relationship accesses<br>
> - Koha::Objects vs DBIx::Class objects<br>
><br>
> == DBIx::Class basic methods ==<br>
> If you want to access a method provided by DBIx::Class, like -<br>
> >store,<br>
> ->delete, ->is_changed, we need to create a silly method which<br>
> just<br>
> calls the same method on the DBIx::Class object.<br>
> For instance, let's take Koha::Objects->is_changed<br>
> sub is_changed {<br>
> my ( $self, @columns ) = @_;<br>
> return $self->_result()->is_changed(@columns);<br>
> }<br>
><br>
> Possible solution:<br>
> It would be possible to modify our AUTOLOAD method and check if<br>
> it's a<br>
> DBIX::Class method.<br>
> If it is a valid one, let's call it, otherwise carp something.<br>
><br>
> == DBIx::Class relationships ==<br>
> From a Koha::Objects, the relationships provided by DBIx::Class<br>
> will<br>
> not be automatically generated.<br>
> For instance, from a virtual shelf, you will not have access to<br>
> the<br>
> items on the shelf.<br>
><br>
> You will have to create a method which will call the DBIx::Class<br>
> relevant relationship.<br>
> sub get_contents {<br>
> my ( $self ) = @_;<br>
> my $contents = $self->{_result}->virtualshelfcontents;<br>
> return $contents;<br>
> }<br>
> This piece of code adds a get_contents method to<br>
> Koha::Virtualshelf<br>
> and call the virtualshelfcontents relationship created by<br>
> DBIx::Class.<br>
> Note that this is finally good here, it is better to call have a<br>
> get_contents method than virtualshelfcontents.<br>
> But we may prefer to redefine it in the schema file<br>
> (Koha::Schema::Result::Virtualshelve).<br>
><br>
> Possible solution:<br>
> Same as previously, it would be possible to modify our AUTOLOAD<br>
> method<br>
> and check if it's a valid relationship.<br>
><br>
> == Koha::Objects vs DBIX::Class objects ==<br>
> We'd like to always manipulate the same kind of objects, to avoid<br>
> confusion when reading the code.<br>
> At the moment we already manipulate 2 different entities: hashref<br>
> from<br>
> C4 and objects from Koha::Object. It's confusing and not so easy<br>
> to<br>
> manage in the code. To add flexibility we have added a<br>
> Koha::Object->unblessed method to transform the object to an<br>
> hashref,<br>
> it's not ideal but it ease the transition.<br>
> Anyway, from a Koha::Object if you want to retrieve data linked to<br>
> this object, you would like to get a Koha::Object object base, not<br>
> a<br>
> DBIx::Class object base.<br>
> If we continue with the previous example, we will need to modify<br>
> get_contents like this:<br>
> sub get_contents {<br>
> my ( $self ) = @_;<br>
> my $rs = $self->{_result}->virtualshelfcontents;<br>
> my $contents = Koha::Virtualshelfcontents->_new_from_dbic( $rs );<br>
> return $contents;<br>
> }<br>
> Basically ->new_from_dbic will just create an empty<br>
> Koha::Virtualshelfcontents object and set the internal resultset.<br>
> No<br>
> query or extra processing should be done here.<br>
><br>
> I do not know if the changes suggested (and already implemented in<br>
> some areas and patches in the queue) is consistent.<br>
> It's not very elegant to read, but it looks safe on the<br>
> performance<br>
> side, despite the encapsulation problem we could see at first<br>
> glance.<br>
><br>
> = Concerns =<br>
> == API ==<br>
> Some people raised concerns about the new API to lean.<br>
> But finally we stick on DBIx::Class method names, so it should not<br>
> be<br>
> to hard to learn for new devs.<br>
> And... the modules in the Koha namespace are much more easier to<br>
> read/understand than the legacy code in C4.<br>
><br>
> == ResultSet ==<br>
> It seems that writing the following is not obvious for everybody:<br>
> # Create and store<br>
> my $biblio = Koha::Biblio->new( { title => "a title", [other =><br>
> $data] } );<br>
> $biblio->store;<br>
><br>
> # Retrieve data<br>
> my $biblio = Koha::Biblios->find( $biblionumber );<br>
> my $biblio = Koha::Biblios->search( { title => "a title" });<br>
><br>
> # Access relationship<br>
> my $items = $biblio->items;<br>
> warn ref( $items ); # Koha::Items<br>
> warn ref( $items->first ); # Koha::Item<br>
><br>
> This looks very easy to read for me, feel free to suggest<br>
> something else.<br>
><br>
> = A complete example =<br>
><br>
> At the moment, the more complete example in master (and 16.05) is<br>
> the<br>
> Koha::Virtualshelf and Koha::Virtualshelves modules. It includes<br>
> several search methods, overwrite of the store method and some<br>
> complex<br>
> methods as well as complete test coverage<br>
> (t/db_dependent/Virtualshelves.t)<br>
><br>
> = Alternatives =<br>
> If you think you can provide an alternative or an idea to<br>
> improve/replace our Koha::Object[s] implementation, do not<br>
> hesitate, I<br>
> would be glad to review or even implement it!<br>
><br>
> If you want to discuss one of these point, add a new one, or have<br>
> a<br>
> specific question, please answer this email.<br>
> We could talk about that during the next dev meeting as well.<br>
><br>
> Cheers,<br>
> Jonathan<br>
> _______________________________________________<br>
> Koha-devel mailing list<br>
> <a href="mailto:Koha-devel@lists.koha-community.org">Koha-devel@lists.koha-community.org</a><br>
> <a href="http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel" rel="noreferrer" target="_blank">http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel</a><br>
><br>
> website : <a href="http://www.koha-community.org/" rel="noreferrer" target="_blank">http://www.koha-community.org/</a><br>
> git : <a href="http://git.koha-community.org/" rel="noreferrer" target="_blank">http://git.koha-community.org/</a><br>
> bugs : <a href="http://bugs.koha-community.org/" rel="noreferrer" target="_blank">http://bugs.koha-community.org/</a><br>
><br>
><br>
<br>
_______________________________________________<br>
Koha-devel mailing list<br>
<a href="mailto:Koha-devel@lists.koha-community.org">Koha-devel@lists.koha-community.org</a><br>
<a href="http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel" rel="noreferrer" target="_blank">http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel</a><br>
website : <a href="http://www.koha-community.org/" rel="noreferrer" target="_blank">http://www.koha-community.org/</a><br>
git : <a href="http://git.koha-community.org/" rel="noreferrer" target="_blank">http://git.koha-community.org/</a><br>
bugs : <a href="http://bugs.koha-community.org/" rel="noreferrer" target="_blank">http://bugs.koha-community.org/</a></div></div></blockquote></div><br></div>