[Bug 11998] New: Syspref caching issues
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Bug ID: 11998 Summary: Syspref caching issues Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: gmcharlt@gmail.com Reporter: robin@catalyst.net.nz QA Contact: testopia@bugs.koha-community.org Sysprefs are cached in process, which is a problem when you have multiple plack workers. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Blocks| |7172 Assignee|gmcharlt@gmail.com |robin@catalyst.net.nz -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #1 from Robin Sheat <robin@catalyst.net.nz> --- Created attachment 26575 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=26575&action=edit Bug 11998 - timeout for syspref caching This adds a 30 second timeout for syspref caching so that when run under plack, you don't end up with different workers having different ideas of what the sysprefs are for too long. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #2 from Robin Sheat <robin@catalyst.net.nz> --- There will be some followups to this: improve the way sysprefs are saved by admin/systempreferences.pl, and to enable storing of sysprefs in memcache in preference to the in-process cache, if it's available. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@biblibre.co | |m -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #3 from Robin Sheat <robin@catalyst.net.nz> --- Created attachment 26621 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=26621&action=edit Bug 11998 - move most syspref db access into Context.pm There was a lot of direct database access in systempreferences.pl. This moves a lot of it into C4::Context so that it'll interact with caching better, and to centralise common functions and logging in one place. Test plan: * Make sure that creating, editing, deleting local use preferences works * Make sure that other system preferences still function normally -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Jacek Ablewicz <abl@biblos.pk.edu.pl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |abl@biblos.pk.edu.pl -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #4 from Robin Sheat <robin@catalyst.net.nz> --- Created attachment 26714 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=26714&action=edit Bug 11998 - sysprefs in memcached The caching mechanism has been abstracted a little bit, and now supports memcached for storing sysprefs. These should be correctly stored and cleared so caching will work between processes. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #5 from Robin Sheat <robin@catalyst.net.nz> --- These patches will need to be treated as a series, they depend on the previous ones. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #6 from Robin Sheat <robin@catalyst.net.nz> --- Created attachment 26716 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=26716&action=edit Bug 11998 - add test case, fix issue found Adds a test case with improved coverage of the caching functionality. Fixes a bug that was uncovered by writing the test case. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #26716|0 |1 is obsolete| | --- Comment #7 from Robin Sheat <robin@catalyst.net.nz> --- Created attachment 26717 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=26717&action=edit Bug 11998 - add test case, fix issue found Adds a test case with improved coverage of the caching functionality. Fixes a bug that was uncovered by writing the test case. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #8 from M. Tompsett <mtompset@hotmail.com> --- Comment on attachment 26621 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=26621 Bug 11998 - move most syspref db access into Context.pm Review of attachment 26621: --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=11998&attachment=26621) ----------------------------------------------------------------- ::: C4/Context.pm @@ +645,2 @@
+ $value = 0 if ( $db_type && $db_type eq 'YesNo' && $value eq '' );
What if $value = undef? The eq check will blow up. Should it not be !$value instead? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #9 from M. Tompsett <mtompset@hotmail.com> --- Comment on attachment 26714 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=26714 Bug 11998 - sysprefs in memcached Review of attachment 26714: --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=11998&attachment=26714) ----------------------------------------------------------------- ::: C4/Context.pm @@ +740,4 @@
my $sth = $dbh->prepare("DELETE FROM systempreferences WHERE variable=?"); my $res = $sth->execute($var); if ($res) { + C4::Context->set_preference($var, undef);
Isn't this supposed to be a _cache_preference call? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #10 from M. Tompsett <mtompset@hotmail.com> --- Comment on attachment 26717 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=26717 Bug 11998 - add test case, fix issue found Review of attachment 26717: --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=11998&attachment=26717) ----------------------------------------------------------------- ::: C4/Context.pm @@ +744,4 @@
my $sth = $dbh->prepare("DELETE FROM systempreferences WHERE variable=?"); my $res = $sth->execute($var); if ($res) { + _cache_preference($var, undef);
Ah, there's the fix. :) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #11 from Robin Sheat <robin@catalyst.net.nz> --- (In reply to M. Tompsett from comment #8)
+ $value = 0 if ( $db_type && $db_type eq 'YesNo' && $value eq '' );
What if $value = undef? The eq check will blow up. Should it not be !$value instead?
That was in the original code, and hadn't blown up that I've seen before. It probably be !$value though. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #26717|0 |1 is obsolete| | --- Comment #12 from Robin Sheat <robin@catalyst.net.nz> --- Created attachment 27382 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=27382&action=edit Bug 11998 - add test case, fix issue found Adds a test case with improved coverage of the caching functionality. Fixes a bug that was uncovered by writing the test case. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |major -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Magnus Enger <magnus@enger.priv.no> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |magnus@enger.priv.no Blocks| |12173 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |chris@bigballofwax.co.nz --- Comment #13 from Chris Cormack <chris@bigballofwax.co.nz> --- Can someone please sign this off, ... (I can't as its from catalyst) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #14 from David Cook <dcook@prosentient.com.au> --- I don't use Plack but here's a *bump* anyway... -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Jared Camins-Esakov <jcamins@cpbibliography.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #15 from Jared Camins-Esakov <jcamins@cpbibliography.com> --- Is there a test plan for this? I can recreate the problem, but not consistently, and it's very difficult to prove the absence of problems with non-deterministic symptoms. Also, is there a reason you're using memcached directly rather than Koha::Cache? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #16 from Robin Sheat <robin@catalyst.net.nz> --- It's hard to test because whether you get the problem is dependent on what plack worker you end up hitting, so it's inherently non-deterministic (or at least, pretty darn hard to predict.) As for Koha::Cache, I wrote this before I wrote Koha::Cache. Didn't think to go back and convert it. I might do that. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Viktor Sarge <viktor.sarge@regionhalland.se> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |viktor.sarge@regionhalland. | |se -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #17 from Robin Sheat <robin@catalyst.net.nz> --- Created attachment 31801 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=31801&action=edit Bug 11998 - make syspref caching use Koha::Cache This modifies the caching system in C4::Context to use Koha::Cache instead of a custom implementation. It should be applied on top of the other syspref caching patches. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Medium patch -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #31801|0 |1 is obsolete| | --- Comment #18 from Robin Sheat <robin@catalyst.net.nz> --- Created attachment 31802 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=31802&action=edit Bug 11998 - make syspref caching use Koha::Cache This modifies the caching system in C4::Context to use Koha::Cache instead of a custom implementation. It should be applied on top of the other syspref caching patches. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |bgkriegel@gmail.com --- Comment #19 from Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> --- Robin, there are some errors/warnings reported by qa script This appears on C4/Context.pm and then on every file that loads it FAIL C4/Context.pm OK pod OK forbidden patterns FAIL valid Name "Cache::RemovalStrategy::FIELDS" used only once: possible typo Constant subroutine C4::Context::CONFIG_FNAME redefined Name "Tie::Hash::FIELDS" used only once: possible typo Name "Cache::RemovalStrategy::LRU::FIELDS" used only once: possible typo FAIL critic # Variables::ProhibitConditionalDeclarations: Got 1 violation(s). Can't find the reason. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Brendan Gallagher <brendan@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Brendan Gallagher <brendan@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #26575|0 |1 is obsolete| | --- Comment #20 from Brendan Gallagher <brendan@bywatersolutions.com> --- Created attachment 32870 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32870&action=edit Bug 11998 - timeout for syspref caching This adds a 30 second timeout for syspref caching so that when run under plack, you don't end up with different workers having different ideas of what the sysprefs are for too long. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Brendan Gallagher <brendan@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #26621|0 |1 is obsolete| | --- Comment #21 from Brendan Gallagher <brendan@bywatersolutions.com> --- Created attachment 32871 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32871&action=edit Bug 11998 - move most syspref db access into Context.pm There was a lot of direct database access in systempreferences.pl. This moves a lot of it into C4::Context so that it'll interact with caching better, and to centralise common functions and logging in one place. Test plan: * Make sure that creating, editing, deleting local use preferences works * Make sure that other system preferences still function normally Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Brendan Gallagher <brendan@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #26714|0 |1 is obsolete| | --- Comment #22 from Brendan Gallagher <brendan@bywatersolutions.com> --- Created attachment 32872 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32872&action=edit Bug 11998 - sysprefs in memcached The caching mechanism has been abstracted a little bit, and now supports memcached for storing sysprefs. These should be correctly stored and cleared so caching will work between processes. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Brendan Gallagher <brendan@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #27382|0 |1 is obsolete| | --- Comment #23 from Brendan Gallagher <brendan@bywatersolutions.com> --- Created attachment 32873 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32873&action=edit Bug 11998 - add test case, fix issue found Adds a test case with improved coverage of the caching functionality. Fixes a bug that was uncovered by writing the test case. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Brendan Gallagher <brendan@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #31802|0 |1 is obsolete| | --- Comment #24 from Brendan Gallagher <brendan@bywatersolutions.com> --- Created attachment 32874 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32874&action=edit Bug 11998 - make syspref caching use Koha::Cache This modifies the caching system in C4::Context to use Koha::Cache instead of a custom implementation. It should be applied on top of the other syspref caching patches. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com --- Comment #25 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- QA Comment: Should these db action in Context.pm use the dbic variation now days? Other than that, the code looks good to me. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #26 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- QA Comment: I also get qa script failure. FAIL C4/Context.pm FAIL critic # Variables::ProhibitConditionalDeclarations: Got 1 violation(s). -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #27 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- I also get all Bernardo's failures too.. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #28 from Robin Sheat <robin@catalyst.net.nz> --- Is it really coming from Koha::Cache? I haven't had a chance to look yet, but I don't really understand those messages, and without understanding they'll be hard to fix. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #29 from Jonathan Druart <jonathan.druart@biblibre.com> --- (In reply to Robin Sheat from comment #28)
Is it really coming from Koha::Cache? I haven't had a chance to look yet, but I don't really understand those messages, and without understanding they'll be hard to fix.
koha@koha-qa:~/src (BZ11998⚡)% perl -wc C4/Context.pm Subroutine handle_errors redefined at C4/Context.pm line 28. Constant subroutine C4::Context::CONFIG_FNAME redefined at /usr/share/perl/5.20/constant.pm line 156. Subroutine KOHAVERSION redefined at C4/Context.pm line 214. Subroutine final_linear_version redefined at C4/Context.pm line 234. Subroutine read_config_file redefined at C4/Context.pm line 263. Subroutine ismemcached redefined at C4/Context.pm line 279. Subroutine memcached redefined at C4/Context.pm line 290. Subroutine db_scheme2dbi redefined at C4/Context.pm line 309. Subroutine import redefined at C4/Context.pm line 314. Subroutine new redefined at C4/Context.pm line 353. Subroutine set_context redefined at C4/Context.pm line 429. Subroutine restore_context redefined at C4/Context.pm line 468. Subroutine _common_config redefined at C4/Context.pm line 499. Subroutine config redefined at C4/Context.pm line 512. Subroutine zebraconfig redefined at C4/Context.pm line 515. Subroutine ModZebrations redefined at C4/Context.pm line 518. Subroutine preference redefined at C4/Context.pm line 540. Subroutine boolean_preference redefined at C4/Context.pm line 567. Subroutine enable_syspref_cache redefined at C4/Context.pm line 583. Subroutine disable_syspref_cache redefined at C4/Context.pm line 597. Subroutine clear_syspref_cache redefined at C4/Context.pm line 613. Subroutine set_preference redefined at C4/Context.pm line 628. Subroutine delete_preference redefined at C4/Context.pm line 676. Subroutine AUTOLOAD redefined at C4/Context.pm line 703. Subroutine Zconn redefined at C4/Context.pm line 725. Subroutine _new_Zconn redefined at C4/Context.pm line 751. Subroutine _new_dbh redefined at C4/Context.pm line 813. Subroutine dbh redefined at C4/Context.pm line 878. Subroutine new_dbh redefined at C4/Context.pm line 912. Subroutine set_dbh redefined at C4/Context.pm line 937. Subroutine restore_dbh redefined at C4/Context.pm line 960. Subroutine queryparser redefined at C4/Context.pm line 984. Subroutine _new_queryparser redefined at C4/Context.pm line 1004. Subroutine marcfromkohafield redefined at C4/Context.pm line 1036. Subroutine _new_marcfromkohafield redefined at C4/Context.pm line 1052. Subroutine stopwords redefined at C4/Context.pm line 1077. Subroutine _new_stopwords redefined at C4/Context.pm line 1093. Subroutine userenv redefined at C4/Context.pm line 1117. Subroutine set_userenv redefined at C4/Context.pm line 1139. Subroutine set_shelves_userenv redefined at C4/Context.pm line 1161. Subroutine get_shelves_userenv redefined at C4/Context.pm line 1169. Subroutine _new_userenv redefined at C4/Context.pm line 1196. Subroutine _unset_userenv redefined at C4/Context.pm line 1213. Subroutine get_versions redefined at C4/Context.pm line 1230. Subroutine tz redefined at C4/Context.pm line 1256. Subroutine IsSuperLibrarian redefined at C4/Context.pm line 1271. Subroutine interface redefined at C4/Context.pm line 1295. Name "Tie::Hash::FIELDS" used only once: possible typo at /usr/share/perl/5.20/fields.pm line 135. Name "Cache::RemovalStrategy::LRU::FIELDS" used only once: possible typo at /usr/share/perl/5.20/fields.pm line 135. Name "Cache::RemovalStrategy::FIELDS" used only once: possible typo at /usr/share/perl/5.20/fields.pm line 135. C4/Context.pm syntax OK koha@koha-qa:~/src (BZ11998⚡)% perlcritic C4/Context.pm Variable declared in conditional statement at line 544, column 5. Declare variables outside of the condition. (Severity: 5) On master: koha@koha-qa:~/src (master↓⚡)% perl -wc C4/Context.pm C4/Context.pm syntax OK koha@koha-qa:~/src (master↓⚡)% perlcritic C4/Context.pm C4/Context.pm source OK The perlcritic error is easy to fix, I can provide a followup, but I don't understand the warnings neither. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #30 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 33183 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33183&action=edit Bug 11998: QA follow-up - fix the perlcritic error % perlcritic C4/Context.pm Variable declared in conditional statement at line 544, column 5. Declare variables outside of the condition. (Severity: 5) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #31 from Robin Sheat <robin@catalyst.net.nz> --- I bet it's a circular dependency. This might be problematic. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #32 from Robin Sheat <robin@catalyst.net.nz> --- (In reply to Jonathan Druart from comment #30)
Created attachment 33183 [details] [review] Bug 11998: QA follow-up - fix the perlcritic error
% perlcritic C4/Context.pm Variable declared in conditional statement at line 544, column 5. Declare variables outside of the condition. (Severity: 5)
So I was wondering what purpose this change solves, turns out it can be important: <grantm> my $var if condition is a messy construct that doesn't do quite what you expect <grantm> if condition is true, then $var is created in the current scope and gets cleaned up when the scope ends <grantm> but if condition is not true, then $var does not get created at runtime and instead, the $var that was inferred at compile time is used instead <grantm> it ends up being a file-scoped global that persists a value <Ned> ahh <Ned> that's a bit of a nasty side-effect :p <grantm> actually, it's not a file-scoped global <grantm> it's a lexical and the sub is a closure over that lexical <grantm> the thing about 'my' is that it has compile-time and run-time effects and in this instance the run-time effects only get to happen if condition is true <grantm> historical note: this bug/feature was how people used to do state variables before the 'state' keyword existed <Ned> heh <grantm> specifically: my $state_variable = 0 if 0; <Ned> there should be a word for that <Ned> "fug" perhaps <grantm> it's certainly fugly -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #33 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Is it possible that we strip the C4::Log circular dependency out for now? We could even think of a proper Koha::SystemPreferences class to move this functions to in a short term (post-release). But we definitely need the bugfix, and logging syspref changes could wait if needed (it is actually logged by the /svc/config/systempref.... service). -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #34 from Robin Sheat <robin@catalyst.net.nz> --- Created attachment 33447 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33447&action=edit Bug 11998 - fix compile warnings This creates a simplified logging system that can be used by C4::Context. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #35 from Robin Sheat <robin@catalyst.net.nz> --- (In reply to Robin Sheat from comment #34)
Created attachment 33447 [details] [review] Bug 11998 - fix compile warnings
This creates a simplified logging system that can be used by C4::Context.
Ignore that, there's a bug in it. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #36 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Robin Sheat from comment #35)
(In reply to Robin Sheat from comment #34)
Created attachment 33447 [details] [review] [review] Bug 11998 - fix compile warnings
This creates a simplified logging system that can be used by C4::Context.
Ignore that, there's a bug in it.
Robin, just strip it out. We'll properly implement it as you wanted, later. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #33447|0 |1 is obsolete| | --- Comment #37 from Robin Sheat <robin@catalyst.net.nz> --- Created attachment 33670 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33670&action=edit Bug 11998 - fix compile warnings This creates a simplified logging system that can be used by C4::Context. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #38 from Robin Sheat <robin@catalyst.net.nz> --- Created attachment 33671 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33671&action=edit Bug 11998 - remove logging from places where it shouldn't be This prevents duplicate action_log entries when a preference is updated. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #39 from Robin Sheat <robin@catalyst.net.nz> --- OK, implemented in a good-enough-for-now sorta way that improves things overall. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #40 from Tomás Cohen Arazi <tomascohen@gmail.com> --- I don't understand why we would add a transitional logging facility we don't need if we can just remove the logging introduced by this patches, that is not needed. It seems straightforward to think we should have a Koha::SystemPreferences class soon that can do the loggin properly without introducing circular dependencies. That is out of the scope of this bug, for sure. Also, are we sure we need global variables? I was just about to post counter patches (attributed to Robin) that just remove the introduction of logaction here, but I thought it would be better to just write this lines here. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #41 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 33685 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33685&action=edit Bug 11998: (RM followup) avoid adding an unneeded workaround This patch aims to make my point explicit on how I think we should handle this bug. It removes the addition of a new logging facility, removes the introduction of logging on C4::Context (out if the scope of the bug, and already solved by the svc script, in the wrong place, I know). I'm still concerned about the addition of global variables in favour of a more OO design. I should note I'm not a Plack user so I KNOW I might not be getting a point on that very subject, so: Feel free to discuss and/or obsolete it. Best regards Tomas -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #42 from Tomás Cohen Arazi <tomascohen@gmail.com> --- BTW, please take a look at t/db_dependent/Context.t failing with this patches. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #43 from Robin Sheat <robin@catalyst.net.nz> --- (In reply to Tomás Cohen Arazi from comment #40)
I don't understand why we would add a transitional logging facility we don't need if we can just remove the logging introduced by this patches, that is not needed.
It seems straightforward to think we should have a Koha::SystemPreferences class soon that can do the loggin properly without introducing circular dependencies. That is out of the scope of this bug, for sure.
Also, are we sure we need global variables?
I was just about to post counter patches (attributed to Robin) that just remove the introduction of logaction here, but I thought it would be better to just write this lines here.
Well my thinking was: 1) Logging the same thing in multiple places is bad and wrong and should be removed whenever the chance arises. 2) C4::Context should have the facility to do logging 3) This centralises things which makes it easier to implement an even better Koha::SystemPreferences module later. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #44 from Robin Sheat <robin@catalyst.net.nz> --- (In reply to Tomás Cohen Arazi from comment #41)
I'm still concerned about the addition of global variables in favour of a more OO design.
What global variable do you mean? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #45 from Robin Sheat <robin@catalyst.net.nz> --- I forgot to add: I don't really care about these details :) I'd rather see it in sooner rather than later, the issues now are very much on the level of being fixable later, whereas the patch is actually fixing a real bug. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #46 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Comment on attachment 32874 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32874 Bug 11998 - make syspref caching use Koha::Cache
+my $syspref_cache = Koha::Cache->get_instance(); +my $use_syspref_cache = 1;
I was talking about this, which I thought could be problematic for Plack, but as I said I'm all ears on Plack. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #47 from Robin Sheat <robin@catalyst.net.nz> --- (In reply to Tomás Cohen Arazi from comment #46)
+my $syspref_cache = Koha::Cache->get_instance(); +my $use_syspref_cache = 1;
I was talking about this, which I thought could be problematic for Plack, but as I said I'm all ears on Plack.
This is fine, the first case because it's a cache and so it's allowed to be on instance per thread, that's the whole point of it. The second because it's manipulating the cache setting. In general, I think it's only used by test cases and possibly by scripts running standalone. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #33671|0 |1 is obsolete| | --- Comment #48 from Robin Sheat <robin@catalyst.net.nz> --- Created attachment 33724 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33724&action=edit Bug 11998 - remove logging from places where it shouldn't be This prevents duplicate action_log entries when a preference is updated. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #49 from Robin Sheat <robin@catalyst.net.nz> --- Created attachment 33725 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33725&action=edit Bug 11998 - fix test case so it's testing the cache properly -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #50 from Jonathan Druart <jonathan.druart@biblibre.com> --- (In reply to Robin Sheat from comment #48)
Created attachment 33724 [details] [review] Bug 11998 - remove logging from places where it shouldn't be
This prevents duplicate action_log entries when a preference is updated.
But... Is there at least one left now? I don't see where the log is done after this patch. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #51 from Jonathan Druart <jonathan.druart@biblibre.com> --- After all these patches applied, there is an unused variable $logtype in C4::Context::set_preference. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #52 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 33746 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33746&action=edit Bug 11998: Add Koha::Config::SystemPreference This (dirty) patch extract the syspref set and delete routine into a module. It fixes the circular dependency and the logging problem. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #53 from Jonathan Druart <jonathan.druart@biblibre.com> --- (In reply to Jonathan Druart from comment #52)
Created attachment 33746 [details] [review] Bug 11998: Add Koha::Config::SystemPreference
I hope this patch won't add more confusion. Feel free to obsolete it if not needed. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #54 from Robin Sheat <robin@catalyst.net.nz> --- (In reply to Jonathan Druart from comment #50)
But... Is there at least one left now? I don't see where the log is done after this patch.
Oh, it wasn't clear: mine were an alternative to Tomás's patch. If you put them both together, you have no logging. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #55 from Robin Sheat <robin@catalyst.net.nz> --- (In reply to Jonathan Druart from comment #52)
Created attachment 33746 [details] [review] Bug 11998: Add Koha::Config::SystemPreference
This (dirty) patch extract the syspref set and delete routine into a module. It fixes the circular dependency and the logging problem.
That's probably the better solution. Though, the caching should happen inside the module too. Eventually this means we could move away from using C4::Context->preference, but for now we maintain a compatibility shim. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #56 from Jonathan Druart <jonathan.druart@biblibre.com> --- (In reply to Robin Sheat from comment #55)
(In reply to Jonathan Druart from comment #52)
Created attachment 33746 [details] [review] [review] Bug 11998: Add Koha::Config::SystemPreference
This (dirty) patch extract the syspref set and delete routine into a module. It fixes the circular dependency and the logging problem.
That's probably the better solution. Though, the caching should happen inside the module too. Eventually this means we could move away from using C4::Context->preference, but for now we maintain a compatibility shim.
The idea was to have a quick and safe patch for 3.18. The SystemPreference module will have to be rewritten in a OO way. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #57 from Robin Sheat <robin@catalyst.net.nz> --- (In reply to Jonathan Druart from comment #56)
The idea was to have a quick and safe patch for 3.18. The SystemPreference module will have to be rewritten in a OO way.
fair enough. I'd add comments to that effect. Especially about the caching, given a an small amount of technical debt is being added. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #58 from Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> --- Seems to be many people interested in this, but it's not clear (for me) how to test. Besides that, koha-qa still complains koha-qa -c 11 testing 11 commit(s) (applied to b2649cb 'Bug 12750: koha-create should be able') fatal: ambiguous argument 'C4/LogStatic.pm': unknown revision or path not in the working tree. Use '--' to separate paths from revisions FAIL C4/Context.pm FAIL Koha/Config/SystemPreference.pm FAIL t/db_dependent/Context.t FAIL admin/preferences.pl OK C4/LogStatic.pm FAIL t/db_dependent/Circulation.t FAIL t/db_dependent/check_sysprefs.t FAIL t/db_dependent/sysprefs.t FAIL admin/systempreferences.pl -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #33685|0 |1 is obsolete| | --- Comment #59 from Robin Sheat <robin@catalyst.net.nz> --- Comment on attachment 33685 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33685 Bug 11998: (RM followup) avoid adding an unneeded workaround Marking Tomás' patch obsolete, as things are being refactored more cleanly. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #60 from Robin Sheat <robin@catalyst.net.nz> --- Created attachment 33829 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33829&action=edit Bug 11998 - tidy up the Koha::Config::SystemPreferences module a bit -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #61 from Robin Sheat <robin@catalyst.net.nz> --- Added docs and a copyright header to Koha::Config::SystemPreference, because nothing should ever get in without those. OK C4/Context.pm OK Koha/Config/SystemPreference.pm OK t/db_dependent/Context.t OK admin/preferences.pl OK C4/LogStatic.pm OK t/db_dependent/Circulation.t OK t/db_dependent/check_sysprefs.t OK t/db_dependent/sysprefs.t OK admin/systempreferences.pl OK C4/Circulation.pm OK circ/returns.pl OK t/db_dependent/Circulation/Returns.t -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #62 from Jonathan Druart <jonathan.druart@biblibre.com> --- Robin, I still got the compilation error: % more Koha/Pouet.pm package Koha::Pouet; use Koha::Database; 1; % perl -wc Koha/Pouet.pm Name "Cache::RemovalStrategy::FIELDS" used only once: possible typo at /usr/share/perl/5.20/fields.pm line 135. Name "Cache::RemovalStrategy::LRU::FIELDS" used only once: possible typo at /usr/share/perl/5.20/fields.pm line 135. Name "Tie::Hash::FIELDS" used only once: possible typo at /usr/share/perl/5.20/fields.pm line 135. Koha/Pouet.pm syntax OK Actually, it comes from the use Koha::Cache in C4::Context introduced by this patch set. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #63 from Robin Sheat <robin@catalyst.net.nz> --- Adding the Koha::Config::SystemPreferences module doesn't actually solve this problem, it just makes it harder to find. We still have a circular dependency on C4::Context. I'm going to see if I can fix that for a bit. If not, going to roll it all back to my static logger. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #64 from Robin Sheat <robin@catalyst.net.nz> --- Created attachment 33940 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33940&action=edit Bug 11998 - fix some compile warnings This lazy-loads C4::Context and some other modules so that they can be be safely used by C4::Context without introducing problematic circular dependencies. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #65 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- So... I'm a tad confused, are we back at a point where I can test these patches for signoff..? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #66 from Jacek Ablewicz <abl@biblos.pk.edu.pl> --- (In reply to Robin Sheat from comment #64)
Created attachment 33940 [details] [review] Bug 11998 - fix some compile warnings
This lazy-loads C4::Context and some other modules so that they can be be safely used by C4::Context without introducing problematic circular dependencies.
This approach may be somehow problematic IMO.. Looks like it relies on the unwritten assumption that 'use C4::Context' would be always used somewhere beforehand at least once - because if not, usually there will be no default/already existing context to rely upon. Currently, initial context is most often created & stored as default in the import() function in C4::Context (and typically only there ?). Take this part for example: --- a/Koha/Database.pm +++ a/Koha/Database.pm @@ -34,7 +34,6 @@ Koha::Database use Modern::Perl; use Carp; -use C4::Context; use Koha::Schema; use base qw(Class::Accessor); @@ -46,6 +45,7 @@ __PACKAGE__->mk_accessors(qw( )); # returns it. sub _new_schema { + require C4::Context; my $context = C4::Context->new(); with it applied, some simple code chunks like that: use Koha::Database; my $scnt = Koha::Database->new()->schema()->resultset('Subscription')->count(); would fail ("... Can't call method "config" on unblessed reference at /home/koha/devkohaclone/C4/Context.pm line 780"). It's possible that this particular issue is being addressed in some other patch[es] belonging to this bug report, but it's hard to say for sure, as the whole patch set does not apply on the current master at the moment. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de Status|Needs Signoff |Patch doesn't apply -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Dobrica Pavlinusic <dpavlin@rot13.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dpavlin@rot13.org -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #67 from Dobrica Pavlinusic <dpavlin@rot13.org> --- In plack discussion we decided to use this patch as starting point for plack sysprefs problems, but it doesn't apply for me: Applying: Bug 11998 - move most syspref db access into Context.pm fatal: sha1 information is lacking or useless (C4/Context.pm). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 11998 - move most syspref db access into Context.pm This one is quite large, so I'm reluctant to apply it by hand. Robin, can you please rebase it? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #32870|0 |1 is obsolete| | --- Comment #68 from Robin Sheat <robin@catalyst.net.nz> --- Created attachment 36523 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=36523&action=edit Bug 11998 - timeout for syspref caching This adds a 30 second timeout for syspref caching so that when run under plack, you don't end up with different workers having different ideas of what the sysprefs are for too long. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #32871|0 |1 is obsolete| | --- Comment #69 from Robin Sheat <robin@catalyst.net.nz> --- Created attachment 36524 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=36524&action=edit Bug 11998 - move most syspref db access into Context.pm There was a lot of direct database access in systempreferences.pl. This moves a lot of it into C4::Context so that it'll interact with caching better, and to centralise common functions and logging in one place. Test plan: * Make sure that creating, editing, deleting local use preferences works * Make sure that other system preferences still function normally Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #32872|0 |1 is obsolete| | --- Comment #70 from Robin Sheat <robin@catalyst.net.nz> --- Created attachment 36525 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=36525&action=edit Bug 11998 - sysprefs in memcached The caching mechanism has been abstracted a little bit, and now supports memcached for storing sysprefs. These should be correctly stored and cleared so caching will work between processes. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #32873|0 |1 is obsolete| | --- Comment #71 from Robin Sheat <robin@catalyst.net.nz> --- Created attachment 36526 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=36526&action=edit Bug 11998 - add test case, fix issue found Adds a test case with improved coverage of the caching functionality. Fixes a bug that was uncovered by writing the test case. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #32874|0 |1 is obsolete| | --- Comment #72 from Robin Sheat <robin@catalyst.net.nz> --- Created attachment 36527 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=36527&action=edit Bug 11998 - make syspref caching use Koha::Cache This modifies the caching system in C4::Context to use Koha::Cache instead of a custom implementation. It should be applied on top of the other syspref caching patches. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #33183|0 |1 is obsolete| | --- Comment #73 from Robin Sheat <robin@catalyst.net.nz> --- Created attachment 36528 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=36528&action=edit Bug 11998: QA follow-up - fix the perlcritic error % perlcritic C4/Context.pm Variable declared in conditional statement at line 544, column 5. Declare variables outside of the condition. (Severity: 5) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #33670|0 |1 is obsolete| | --- Comment #74 from Robin Sheat <robin@catalyst.net.nz> --- Created attachment 36529 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=36529&action=edit Bug 11998 - fix compile warnings This creates a simplified logging system that can be used by C4::Context. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #33724|0 |1 is obsolete| | --- Comment #75 from Robin Sheat <robin@catalyst.net.nz> --- Created attachment 36530 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=36530&action=edit Bug 11998 - remove logging from places where it shouldn't be This prevents duplicate action_log entries when a preference is updated. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #33725|0 |1 is obsolete| | --- Comment #76 from Robin Sheat <robin@catalyst.net.nz> --- Created attachment 36531 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=36531&action=edit Bug 11998 - fix test case so it's testing the cache properly -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #33746|0 |1 is obsolete| | --- Comment #77 from Robin Sheat <robin@catalyst.net.nz> --- Created attachment 36532 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=36532&action=edit Bug 11998: Add Koha::Config::SystemPreference This (dirty) patch extract the syspref set and delete routine into a module. It fixes the circular dependency and the logging problem. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #33829|0 |1 is obsolete| | --- Comment #78 from Robin Sheat <robin@catalyst.net.nz> --- Created attachment 36533 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=36533&action=edit Bug 11998 - tidy up the Koha::Config::SystemPreferences module a bit -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #33940|0 |1 is obsolete| | --- Comment #79 from Robin Sheat <robin@catalyst.net.nz> --- Created attachment 36534 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=36534&action=edit Bug 11998 - fix some compile warnings This lazy-loads C4::Context and some other modules so that they can be be safely used by C4::Context without introducing problematic circular dependencies. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Dobrica Pavlinusic <dpavlin@rot13.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=13791 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #80 from Dobrica Pavlinusic <dpavlin@rot13.org> --- Created attachment 36571 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=36571&action=edit Bug 11998 - Syspref caching issues fix delete_preference method and make t/db_dependent/sysprefs.t pass again -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Brendan Gallagher <brendan@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Brendan Gallagher <brendan@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #36523|0 |1 is obsolete| | --- Comment #81 from Brendan Gallagher <brendan@bywatersolutions.com> --- Created attachment 36583 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=36583&action=edit Bug 11998 - timeout for syspref caching This adds a 30 second timeout for syspref caching so that when run under plack, you don't end up with different workers having different ideas of what the sysprefs are for too long. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Brendan Gallagher <brendan@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #36524|0 |1 is obsolete| | --- Comment #82 from Brendan Gallagher <brendan@bywatersolutions.com> --- Created attachment 36584 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=36584&action=edit Bug 11998 - move most syspref db access into Context.pm There was a lot of direct database access in systempreferences.pl. This moves a lot of it into C4::Context so that it'll interact with caching better, and to centralise common functions and logging in one place. Test plan: * Make sure that creating, editing, deleting local use preferences works * Make sure that other system preferences still function normally Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Brendan Gallagher <brendan@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #36525|0 |1 is obsolete| | --- Comment #83 from Brendan Gallagher <brendan@bywatersolutions.com> --- Created attachment 36585 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=36585&action=edit Bug 11998 - sysprefs in memcached The caching mechanism has been abstracted a little bit, and now supports memcached for storing sysprefs. These should be correctly stored and cleared so caching will work between processes. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Brendan Gallagher <brendan@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #36526|0 |1 is obsolete| | --- Comment #84 from Brendan Gallagher <brendan@bywatersolutions.com> --- Created attachment 36586 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=36586&action=edit Bug 11998 - add test case, fix issue found Adds a test case with improved coverage of the caching functionality. Fixes a bug that was uncovered by writing the test case. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Brendan Gallagher <brendan@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #36527|0 |1 is obsolete| | --- Comment #85 from Brendan Gallagher <brendan@bywatersolutions.com> --- Created attachment 36587 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=36587&action=edit Bug 11998 - make syspref caching use Koha::Cache This modifies the caching system in C4::Context to use Koha::Cache instead of a custom implementation. It should be applied on top of the other syspref caching patches. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Brendan Gallagher <brendan@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #36528|0 |1 is obsolete| | --- Comment #86 from Brendan Gallagher <brendan@bywatersolutions.com> --- Created attachment 36588 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=36588&action=edit Bug 11998: QA follow-up - fix the perlcritic error % perlcritic C4/Context.pm Variable declared in conditional statement at line 544, column 5. Declare variables outside of the condition. (Severity: 5) Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Brendan Gallagher <brendan@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #36529|0 |1 is obsolete| | --- Comment #87 from Brendan Gallagher <brendan@bywatersolutions.com> --- Created attachment 36589 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=36589&action=edit Bug 11998 - fix compile warnings This creates a simplified logging system that can be used by C4::Context. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Brendan Gallagher <brendan@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #36530|0 |1 is obsolete| | --- Comment #88 from Brendan Gallagher <brendan@bywatersolutions.com> --- Created attachment 36590 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=36590&action=edit Bug 11998 - remove logging from places where it shouldn't be This prevents duplicate action_log entries when a preference is updated. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Brendan Gallagher <brendan@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #36531|0 |1 is obsolete| | --- Comment #89 from Brendan Gallagher <brendan@bywatersolutions.com> --- Created attachment 36591 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=36591&action=edit Bug 11998 - fix test case so it's testing the cache properly Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Brendan Gallagher <brendan@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #36532|0 |1 is obsolete| | --- Comment #90 from Brendan Gallagher <brendan@bywatersolutions.com> --- Created attachment 36592 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=36592&action=edit Bug 11998: Add Koha::Config::SystemPreference This (dirty) patch extract the syspref set and delete routine into a module. It fixes the circular dependency and the logging problem. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Brendan Gallagher <brendan@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #36533|0 |1 is obsolete| | --- Comment #91 from Brendan Gallagher <brendan@bywatersolutions.com> --- Created attachment 36593 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=36593&action=edit Bug 11998 - tidy up the Koha::Config::SystemPreferences module a bit Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Brendan Gallagher <brendan@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #36534|0 |1 is obsolete| | --- Comment #92 from Brendan Gallagher <brendan@bywatersolutions.com> --- Created attachment 36594 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=36594&action=edit Bug 11998 - fix some compile warnings This lazy-loads C4::Context and some other modules so that they can be be safely used by C4::Context without introducing problematic circular dependencies. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Brendan Gallagher <brendan@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #36571|0 |1 is obsolete| | --- Comment #93 from Brendan Gallagher <brendan@bywatersolutions.com> --- Created attachment 36595 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=36595&action=edit Bug 11998 - Syspref caching issues fix delete_preference method and make t/db_dependent/sysprefs.t pass again Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=13805 --- Comment #94 from Jonathan Druart <jonathan.druart@biblibre.com> --- Could be "solved" by bug 13805. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=13967 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Patch doesn't apply --- Comment #95 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- We need to get moving on this, but there are conflicts in Context.pm. Please rebase! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |josef.moravec@gmail.com -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 --- Comment #96 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- I am not sure about the current state of this - are the current patches still valid? -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org