Summary: | General fix for plugins breaking database transactions | ||
---|---|---|---|
Product: | Koha | Reporter: | Philip Orr <philip.orr> |
Component: | Architecture, internals, and plumbing | Assignee: | Paul Derscheid <paul.derscheid> |
Status: | Failed QA --- | QA Contact: | Testopia <testopia> |
Severity: | major | ||
Priority: | P5 - low | CC: | david, dcook, kyle, martin.renvoize, nick, paul.derscheid |
Version: | unspecified | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Circulation function: | |||
Attachments: |
Bug 38384: Trigger plugin modules loading as soon as possible in Koha
koha-plugin-fancyplugin from KohaAdvent Test case (script) to reproduce the breaking database transaction Bug 38384: Inline plugin loading into C4::Context's BEGIN block Bug 38384: Prevent initialization failure by checking for existence of plugin_data first |
Description
Philip Orr
2024-11-07 09:56:55 UTC
We had discussed that at the hackfest and it has caused great trouble for some plugin users. We should also consider adding a warning to documentation, even the advent calendar. Created attachment 174150 [details] [review] Bug 38384: Trigger plugin modules loading as soon as possible in Koha I think this may solve the issue. I don't have a specific test plan to move this forward though. Is this ready for testing, Kyle? And thanks! The problem is that this fix creates a circular dependency that just breaks. We actually need a way to move the logic within 'get_enabled_plugins' into the BEGIN block of C4::Context w/o depending on Koha::Plugins at all. To test (preliminary): 1) Get the koha-plugin-fancyplugin from KohaAdvent. 2) Install it 3) Use test_transaction.pl and observe that due to the reinitialization, the transaction breaks and the biblionumber is returned(!) but after that we can't find the corresponding biblio. Then: 4) Find a way to implement comment #5 5) Apply your patch 6) See that the biblio is found by Koha::Biblios->find Created attachment 174264 [details]
koha-plugin-fancyplugin from KohaAdvent
Created attachment 174265 [details]
Test case (script) to reproduce the breaking database transaction
Using Koha::Cache::Memory::Lite in C4::Context's BEGIN should be find, though. Created attachment 174280 [details] [review] Bug 38384: Inline plugin loading into C4::Context's BEGIN block Not too sure on that latest patch. I essentially only translated the call to Koha::Plugins->get_enabled_plugins to an inlined version without dependencies on other local modules. This is however very ugly, I am just curious what others think. Thoughts? Created attachment 174282 [details] [review] Bug 38384: Prevent initialization failure by checking for existence of plugin_data first I don't think we should be using XML::Simple here, even for a simple use case. Worth exploring other fast alternatives though... Btw, you should be able to use Koha::Database instead of DBI directly there, since it's already lightweight (and uses the same modules, so it's same-same but with less duplicated code). (In reply to David Cook from comment #13) > Btw, you should be able to use Koha::Database instead of DBI directly there, > since it's already lightweight (and uses the same modules, so it's same-same > but with less duplicated code). For instance, look at bug 37657 where C4::Context->dbh is replaced by Koha::Database->dbh for big speed gain. We have "TableExists" in C4::Installer - it doesn't need a different syntax per engine, could we do the same here? Select all form the table and check if any warning (In reply to Nick Clemens (kidclamp) from comment #15) > We have "TableExists" in C4::Installer - it doesn't need a different syntax > per engine, could we do the same here? Select all form the table and check > if any warning Did you comment on the right bug here, Nick? @David I think Nick means the "TableExists" here: https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=174282&action=diff&headers=0#a/C4/Context.pm_sec3 |