[Koha-bugs] [Bug 21073] Improve plugin performance

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073

--- Comment #70 from Tomás Cohen Arazi <tomascohen at gmail.com> ---
(In reply to Martin Renvoize from comment #49)
> Comment on attachment 89801 [details] [review]
> Bug 21073: Improve plugin performance
> 
> Review of attachment 89801 [details] [review]:
> -----------------------------------------------------------------
> 
> A few, hopefully minor, bits of feedback.. Failing QA whilst I await a reply.
> 
> ::: Koha/Plugins.pm
> @@ +73,5 @@
> >      my $method = $params->{method};
> >      my $req_metadata = $params->{metadata} // {};
> >  
> > +    my $dbh = C4::Context->dbh;
> > +    my $plugin_classes = $dbh->selectcol_arrayref('SELECT DISTINCT(plugin_class) FROM plugin_methods');
> 
> Why do we mix old dbh calls with dbic calls in this new code?

Fixed!

> Also, can we not skip a series of DB calls that are found in the loop by
> filtering on $method if it exists before running through the loop?

Fixed!

> @@ +119,4 @@
> >  
> >              my $plugin = $plugin_class->new({ enable_plugins => $self->{'enable_plugins'} });
> >  
> > +            Koha::Plugins::Methods->search({ plugin_class => $plugin_class })->delete();
> 
> Can't make my mind up as to whether we should really rebuilding the whole db
> table with every/any install/upgrade of any plugin...

I agree with this uncomfortable feeling,. I've added a note about why we do
this in the POD. Hopefully once this is in master and we start using it, we
realize there are simple ways to deal with faulty scenarios. I volunteer to
work on it if we find out.

> ::: Koha/Plugins/Handler.pm
> @@ +63,5 @@
> >      my $params        = $args->{'params'};
> >  
> > +    my $has_method = Koha::Plugins::Methods->search({ plugin_class => $plugin_class, plugin_method => $plugin_method })->count();
> > +    if ( $has_method ) {
> > +        load $plugin_class;
> 
> Is it ever possible for 'load' to fail here and if so should we catch and
> warn about it?.. We seem to have effectively removed a warning from the
> prior code.. I'm wondering if we may ever get a case where the plugin exists
> in the DB but has been deleted from the filesystem.

Before this patchset, we really queried the FS for plugins, and we could only
find existing plugins! With this way of not doing so, we might have scenarios
in which the DB contains references to (manually) deleted plugins. See above
for discussion about this. I vote for this approach until things settle a bit.

> @@ +68,2 @@
> >          my $plugin = $plugin_class->new( { cgi => $cgi, enable_plugins => $args->{'enable_plugins'} } );
> > +        my @return = $plugin->$plugin_method( $params );
> 
> The above line is never referenced right?

It was a bug! ->$method was being called twice even! This is probably some
rebasing problem.

> ::: plugins/plugins-upload.pl
> @@ +87,4 @@
> >                  $template->param( ERRORS => [ \%errors ] );
> >                  output_html_with_http_headers $input, $cookie, $template->output;
> >                  exit;
> > +            } else {
> 
> 'unless else' is a weird looking construct.. as we're calling 'exit' in the
> unless block we can just call the 'Koha::Plugins->new()->InstallPlugins'
> method below the block and not in it's own else block.

Fixed!

-- 
You are receiving this mail because:
You are watching all bug changes.