Koha::Cache can be improved to fall back more reliably, to handle complex data types, and so on. This is necessary to allow caching of things properly under plack.
Created attachment 26891 [details] [review] Bug 12041 - improve Koha::Cache This makes Koha::Cache behave better by default. It will use memcached if available to do shared caching, if that's not available it will fall back to in-memory caching. It also allows for a singleton accessor to allow a single cache to be shared within a process. Test plan: * The t/Cache.t file runs successfully with and without the MEMCACHED_SERVERS envvar set (and memcached running in the environment.)
(In reply to Robin Sheat from comment #1) > Created attachment 26891 [details] [review] [review] > Bug 12041 - improve Koha::Cache > > This makes Koha::Cache behave better by default. It will use memcached > if available to do shared caching, if that's not available it will fall > back to in-memory caching. It also allows for a singleton accessor to > allow a single cache to be shared within a process. > > Test plan: > * The t/Cache.t file runs successfully with and without the > MEMCACHED_SERVERS envvar set (and memcached running in the > environment.) You will want to test that storing utf8 and retrieving it still works. If this change munches utf8 then it makes the caching unusable in a lot of situations.
Created attachment 26908 [details] [review] Bug 12041 - utf8 compatibility changes Switched to Cache::Memcached::Fast due to its better support for unicode, added to the test case so that we notice if something causes it to break.
Created attachment 26909 [details] [review] Bug 12041 - improve Koha::Cache This makes Koha::Cache behave better by default. It will use memcached if available to do shared caching, if that's not available it will fall back to in-memory caching. It also allows for a singleton accessor to allow a single cache to be shared within a process. Added tests to confirm UTF8-cleanness. Test plan: * The t/Cache.t file runs successfully with and without the MEMCACHED_SERVERS envvar set (and memcached running in the environment.)
Created attachment 26949 [details] [review] Bug 12041 - improve Koha::Cache This makes Koha::Cache behave better by default. It will use memcached if available to do shared caching, if that's not available it will fall back to in-memory caching. It also allows for a singleton accessor to allow a single cache to be shared within a process. * Added tests to confirm UTF8-cleanness. * Added minor fixups to stop warnings. Test plan: * The t/Cache.t file runs successfully with and without the MEMCACHED_SERVERS envvar set (and memcached running in the environment.)
Created attachment 27381 [details] [review] Bug 12041 - improve Koha::Cache This makes Koha::Cache behave better by default. It will use memcached if available to do shared caching, if that's not available it will fall back to in-memory caching. It also allows for a singleton accessor to allow a single cache to be shared within a process. * Added tests to confirm UTF8-cleanness. * Added minor fixups to stop warnings. Test plan: * The t/Cache.t file runs successfully with and without the MEMCACHED_SERVERS envvar set (and memcached running in the environment.)
Robin - *** ERROR: Spurious text after =cut =over on line 219 without closing =back (at head2) in file Koha/Cache.pm Pretty simple clean up. I tested and it's working, happy to signoff with the follow up for that - or I can ignore that and just tell the QA to follow up or the RM to followup when pushing. let me know what to do. -Brendan
Created attachment 28415 [details] [review] Bug 12041 - POD fixing followup This fixes a minor POD error detected, and also adds the Koha/ directory to the regular test case so that the in-repo tests can find these things.
Robin, All looks good but I don't understand something in the current code. In Koha/Cache.pm 458 unless ( lc( $args->{cache_type} ) eq 'null' ) { 459 $args->{'cache'} = $self; 460 $args->{'cache_type'} ||= $ENV{'CACHING_SYSTEM'}; 461 } Isn't it $self->{cache_type} instead of $args->{cache_type}?
(In reply to Jonathan Druart from comment #9) > Isn't it $self->{cache_type} instead of $args->{cache_type}? I don't think so, it's just figuring out the defaults and adding them to the $args hashref. That's then passed on to Koha::Cache::Object when it's tied.
Created attachment 28611 [details] [review] Bug 12041 - improve Koha::Cache This makes Koha::Cache behave better by default. It will use memcached if available to do shared caching, if that's not available it will fall back to in-memory caching. It also allows for a singleton accessor to allow a single cache to be shared within a process. * Added tests to confirm UTF8-cleanness. * Added minor fixups to stop warnings. Test plan: * The t/Cache.t file runs successfully with and without the MEMCACHED_SERVERS envvar set (and memcached running in the environment.) Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 28612 [details] [review] Bug 12041 - POD fixing followup This fixes a minor POD error detected, and also adds the Koha/ directory to the regular test case so that the in-repo tests can find these things. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 28613 [details] [review] Bug 12041: UT - Get rid of warnings Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Pushed to master. Thanks Robin!
This patch set changed the prototype of set_in_cache, but did not update the calls in t/Cache.t.