Example, instead of this: return Koha::Patrons->find( $self->guarantorid() ); We should do this: return Koha::Patron->_new_from_dbic($self->_result->guarantor); This allows us to use prefetch and perform a single query as opposed to multiple queries
Yes, and no. You can only do that if you know the patron has a guarantor, which is not always true. You must do: sub guarantor { my ( $self ) = @_; my $guarantor_rs = $self->_result->guarantorid; return unless $guarantor_rs; return Koha::Patron->_new_from_dbic($guarantor_rs); } I am definitely in favor of that, we should have a guideline about it IMO.
(In reply to Jonathan Druart from comment #1) > Yes, and no. > > You can only do that if you know the patron has a guarantor, which is not > always true. > > You must do: > sub guarantor { > my ( $self ) = @_; > my $guarantor_rs = $self->_result->guarantorid; > return unless $guarantor_rs; > return Koha::Patron->_new_from_dbic($guarantor_rs); > } > > I am definitely in favor of that, we should have a guideline about it IMO. The pattern should cover the one-to-many use case as well. And yes, the idea is to propose a coding guideline!
We have already everything in our code: Koha::Item->biblio 93 sub biblio { 94 my ( $self ) = @_; 95 my $biblio_rs = $self->_result->biblio; 96 return Koha::Biblio->_new_from_dbic( $biblio_rs ); 97 } Koha::Item->checkout 121 sub checkout { 122 my ( $self ) = @_; 123 my $checkout_rs = $self->_result->issue; 124 return unless $checkout_rs; 125 return Koha::Checkout->_new_from_dbic( $checkout_rs ); 126 } Koha::Item->current_holds 271 sub current_holds { 272 my ( $self ) = @_; 273 my $attributes = { order_by => 'priority' }; 274 my $dtf = Koha::Database->new->schema->storage->datetime_parser; 275 my $params = { 276 itemnumber => $self->itemnumber, 277 suspend => 0, 278 -or => [ 279 reservedate => { '<=' => $dtf->format_date(dt_from_string) }, 280 waitingdate => { '!=' => undef }, 281 ], 282 }; 283 my $hold_rs = $self->_result->reserves->search( $params, $attributes ); 284 return Koha::Holds->_new_from_dbic($hold_rs); 285 }
Do you still want to propose a coding guideline here, maybe for next dev meeting?
(In reply to Katrin Fischer from comment #4) > Do you still want to propose a coding guideline here, maybe for next dev > meeting? I believe we should.
I have added it to the next meetings agenda, but as I am no expert for this topic it would be nice if someone could write up a proposal here.
DRAFT: Within PERL15 under Koha::Object Koha::Object relationship accessors should be written to allow 'prefetch' For example one should use: sub guarantor { my $self = shift; my $guarantor_rs = $self->_result->guarantorid; return unless $guarantor_rs; return Koha::Patron->_new_from_dbic($gaurantor_rs); } One should not use: sub guarantor { my $self = shift; return Koha::Patrons->find( $self->guarantorid() ); }
I've started this off.. but there's a huge number of classes to go through :(
This has me wondering.. we're often renaming relationships within Koha::Objects (understandably as often our db derived ones are horrible because of poor initial db design). Should this guideline also introduce a requirement to match the relation name in the dbic class up to the relation name in the Koha::Object? It wouldn't be hard to add relations with such names and would lead to cleaner, easier to guess, prefetch strings.
(In reply to Martin Renvoize from comment #9) > This has me wondering.. we're often renaming relationships within > Koha::Objects (understandably as often our db derived ones are horrible > because of poor initial db design). Should this guideline also introduce a > requirement to match the relation name in the dbic class up to the relation > name in the Koha::Object? It wouldn't be hard to add relations with such > names and would lead to cleaner, easier to guess, prefetch strings. I'm all for renaming relations like you say. I've tried myself to (a) migrate some methods into matching this guideline and (b) use it in a real life scenario and can say the only uncomfortable step was (b), due to the relation naming issues. See bug 22696.
We could also use 'skip_relationships' in our dbicdump script to prevent outputting superfluous relationships given we can't/won't use them until we've manually written code in the corresponding Koha::Object class.. this way we would end up being much more explicit about what relationships were building (and as such reduce the load time/run time memory use form building massive dbic object full of unreachable expensive relationship accessors. That.. or we could start using the dbic recommended way of doing db work and go from schema to database rather than the other way around [U+1F609] and ditch the script entirely.
(In reply to Martin Renvoize from comment #11) > We could also use 'skip_relationships' in our dbicdump script to prevent > outputting superfluous relationships given we can't/won't use them until > we've manually written code in the corresponding Koha::Object class.. this > way we would end up being much more explicit about what relationships were > building (and as such reduce the load time/run time memory use form building > massive dbic object full of unreachable expensive relationship accessors. I agree with this, but it would require us first finish setting our custom relationships. At least the ones we use. > That.. or we could start using the dbic recommended way of doing db work and > go from schema to database rather than the other way around [U+1F609] and ditch the > script entirely. That's not gonna happen unless someone takes care of it, and it is not a simple task, changing the community workflow and upgrade scripts.
Honest question: Why do this: sub guarantor { my $self = shift; my $guarantor_rs = $self->_result->guarantorid; return unless $guarantor_rs; return Koha::Patron->_new_from_dbic($gaurantor_rs); } Instead of this: sub guarantor { my $self = shift; my $guarantor; my $guarantor_rs = $self->_result->guarantorid; if ($guarantor_rs){ $guarantor = Koha::Patron->_new_from_dbic($gaurantor_rs); } return $guarantor; } I admit that the latter has 3 more lines than the former, but the latter is way easier to read, easier to debug, and easier to add error-handling. Actually, that latter should probably double-check the output of Koha::Patron->_new_from_dbic() before returning. I've noticed a lot of Koha code directly returning the output of a method call, and it drives me crazy. We should do more data validation. It'll mean fewer bugs and easier maintenance.
(In reply to David Cook from comment #13) > I've noticed a lot of Koha code directly returning the output of a method > call, and it drives me crazy. We should do more data validation. It'll mean > fewer bugs and easier maintenance. What is there to check in this particular situation ? _new_from_dbic either dies or return a blessed object. If it dies then Koha::Patron::guarantor should die too, if it returns a blessed object, then there is nothing to check.
(In reply to Julian Maurice from comment #14) > What is there to check in this particular situation ? _new_from_dbic either > dies or return a blessed object. If it dies then Koha::Patron::guarantor > should die too, if it returns a blessed object, then there is nothing to > check. That's an interesting perspective. If it dies, it's probably going to bring down that entire Plack worker. That seems problematic to me. We could try to trap the error in the caller, but in practice that is rare in Koha. That being said, even if it doesn't die, we need to handle a non-fatal exception too. Maybe it is better to die in the grand scheme of things, as it'll more easily show code bugs and bad data. If it doesn't die, what happens if you want to add a check to that object? Maybe you only want to return the guarantor under certain conditions. It probably makes sense to add that check here rather than in every caller of this method. To do that, you'd have to refactor to use the syntax I described anyway. Or maybe you're doing some debugging because the guarantor being returned isn't correct. You can't add any logging in this code unless you refactor to the syntax I described.
(In reply to David Cook from comment #15) > If it doesn't die, what happens if you want to add a check to that object? > Maybe you only want to return the guarantor under certain conditions. It > probably makes sense to add that check here rather than in every caller of > this method. To do that, you'd have to refactor to use the syntax I > described anyway. Or maybe you're doing some debugging because the guarantor > being returned isn't correct. You can't add any logging in this code unless > you refactor to the syntax I described. In my opinion "maybe we will want to add something here" is not a good reason to refactor code. If that "maybe" ever comes true, then you can refactor the code while making the change. No need to do it now. Note that I also tend to prefer your version, but I'm happy with both. If it ain't broke don't fix it ;)
(In reply to Julian Maurice from comment #16) > In my opinion "maybe we will want to add something here" is not a good > reason to refactor code. If that "maybe" ever comes true, then you can > refactor the code while making the change. No need to do it now. Given the number of legacy projects I've maintained over the years, we'll just have to agree to disagree on that one ;). > Note that I also tend to prefer your version, but I'm happy with both. > If it ain't broke don't fix it ;) True enough. There are better hills to die on heh.
Koha::Object is trapping DBIC exceptions and converting them into Koha::Exception* and we are shifting into using them everywhere. They get properly propagated if not catch locally, so I guess there will be places in which we could catch exceptions. But those would be specific cases.
We lost our way here. I updated PERL15 and marked it a defacto rule. Is anyone interested in moving this on for 21.05? I will help them as much as I can. Just let me know!