<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<!--[if !mso]><style>v\:* {behavior:url(#default#VML);}
o\:* {behavior:url(#default#VML);}
w\:* {behavior:url(#default#VML);}
.shape {behavior:url(#default#VML);}
</style><![endif]--><style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman",serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
span.E-mailStijl17
{mso-style-type:personal-reply;
font-family:"Arial",sans-serif;
color:black;}
.MsoChpDefault
{mso-style-type:export-only;
font-family:"Calibri",sans-serif;
mso-fareast-language:EN-US;}
@page WordSection1
{size:612.0pt 792.0pt;
margin:72.0pt 72.0pt 72.0pt 72.0pt;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="NL" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial",sans-serif;color:black;mso-fareast-language:EN-US">Not sure if we want
</span><span lang="EN-US">$patron->{surname} = $surname;<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">If it goes via the Koha::Object, you have control. If you allow mangling the hash outside your object code, what could happen?
<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial",sans-serif;color:black;mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">Van:</span></b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> koha-devel-bounces@lists.koha-community.org [mailto:koha-devel-bounces@lists.koha-community.org]
<b>Namens </b>Kyle Hall<br>
<b>Verzonden:</b> zondag 26 juni 2016 13:06<br>
<b>Aan:</b> Jonathan Druart <jonathan.druart@bugs.koha-community.org><br>
<b>CC:</b> koha-devel@lists.koha-community.org<br>
<b>Onderwerp:</b> Re: [Koha-devel] Koha::Object[s] improvements - call for discussions<o:p></o:p></span></p>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal">Jonthan, I love these ideas and definitely think they would be major improvements to Koha::Object(s).<o:p></o:p></p>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">I would like to toss something else in as well. A while back I submitted bug 15759 as a concept: <a href="https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15759">https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15759</a><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">This bug does some pretty awesome things imo:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">1) it let's us use a Koha::Object as if it is a hashref!<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">So, if we have the following:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">my $patron = Koha::Patrons->find( $borrowernumber );<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">the following are functionally equivilent:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">my $surname = $patron->surname();<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">my $surname = $patron->{surname};<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">So are these:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">$patron->surname( $surname );<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">$paron->{surname} = $surname;<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">2) It prevents bugs<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">One problem right now is you may accidentally use $patron->{surname} when you should have used $patron->surname().<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">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!<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">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!<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Any scripts that already use hashrefs will now be able to use Koha::Objects instead!<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">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:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">$self->{_result} = ....<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">to this<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">$$self->{_result} = ....<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">It is a trivial change though that could easily be done by a script at the time the bug would be pushed to master.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">What does everyone think? Would this be a worthwhile improvement?<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Kyle<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
<div>
<p class="MsoNormal"><br clear="all">
<o:p></o:p></p>
<div>
<div>
<div>
<div>
<div>
<div>
<p class="MsoNormal"><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"><span style="text-decoration:none"><img border="0" id="_x0000_i1025" src="http://widgets.ch-or.us/badge/convio/cff/4715/2706639"></span></a><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal"><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> )<o:p></o:p></p>
</div>
</div>
</div>
</div>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal">On Sun, Jun 26, 2016 at 6:04 AM, Jonathan Druart <<a href="mailto:jonathan.druart@bugs.koha-community.org" target="_blank">jonathan.druart@bugs.koha-community.org</a>> wrote:<o:p></o:p></p>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0cm 0cm 0cm 6.0pt;margin-left:4.8pt;margin-right:0cm">
<p class="MsoNormal">Hi devs,<br>
<br>
At the KohaCon16 some of us have briefly talked about the 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 and<br>
we already have more than 30 classes using it.<br>
It permits to uniformise our way to code, use powerful objects and is<br>
very easy for refactoring. I have been moving code from C4 to the Koha<br>
namespace and it's very handy to just have to write a concise 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 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 ->store,<br>
->delete, ->is_changed, we need to create a silly method which 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 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 will<br>
not be automatically generated.<br>
For instance, from a virtual shelf, you will not have access to 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 Koha::Virtualshelf<br>
and call the virtualshelfcontents relationship created by 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 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 from<br>
C4 and objects from Koha::Object. It's confusing and not so easy to<br>
manage in the code. To add flexibility we have added a<br>
Koha::Object->unblessed method to transform the object to an 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 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. 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 performance<br>
side, despite the encapsulation problem we could see at first 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 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 => $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 something else.<br>
<br>
= A complete example =<br>
<br>
At the moment, the more complete example in master (and 16.05) is the<br>
Koha::Virtualshelf and Koha::Virtualshelves modules. It includes<br>
several search methods, overwrite of the store method and some 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 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 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" target="_blank">http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel</a><br>
website : <a href="http://www.koha-community.org/" target="_blank">http://www.koha-community.org/</a><br>
git : <a href="http://git.koha-community.org/" target="_blank">http://git.koha-community.org/</a><br>
bugs : <a href="http://bugs.koha-community.org/" target="_blank">http://bugs.koha-community.org/</a><o:p></o:p></p>
</blockquote>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
</body>
</html>