<div dir="ltr">Jonthan, I love these ideas and definitely think they would be major improvements to Koha::Object(s).<div><br></div><div>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></div><div><br></div><div>This bug does some pretty awesome things imo:</div><div><br></div><div>1) it let's us use a Koha::Object as if it is a hashref!</div><div><br></div><div>So, if we have the following:</div><div><br></div><div>my $patron = Koha::Patrons->find( $borrowernumber );</div><div><br></div><div>the following are functionally equivilent:</div><div><br></div><div>my $surname = $patron->surname();</div><div>my $surname = $patron->{surname};</div><div><br></div><div>So are these:</div><div><br></div><div>$patron->surname( $surname );</div><div>$paron->{surname} = $surname;</div><div><br></div><div>2) It prevents bugs</div><div><br></div><div>One problem right now is you may accidentally use $patron->{surname} when you should have used $patron->surname().</div><div>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!</div><div><br></div><div>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!</div><div><br></div><div>Any scripts that already use hashrefs will now be able to use Koha::Objects instead!</div><div><br></div><div>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:</div><div>$self->{_result} = ....</div><div>to this</div><div>$$self->{_result} = ....</div><div><br></div><div>It is a trivial change though that could easily be done by a script at the time the bug would be pushed to master.</div><div><br></div><div>What does everyone think? Would this be a worthwhile improvement?</div><div><br></div><div>Kyle</div><div><br></div><div><br></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 Sun, Jun 26, 2016 at 6:04 AM, Jonathan Druart <span dir="ltr"><<a href="mailto:jonathan.druart@bugs.koha-community.org" target="_blank">jonathan.druart@bugs.koha-community.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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" 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><br>
</blockquote></div><br></div>