Looks good Chris H, only thing missing is the change to 
 installer/data/mysql/sysprefs.sql

Have signed off, but leaving it in needs signoff as I think we need a signoff from outside Catalyst.

Hi Chris H., can you please add a description and test plan to this bug? As is I have no idea what it's supposed to do and how to test it. Thx! :)

QA Comment/Questions: 
This patch adds a syspref usecache. I would opt for a more specific name. Note that the word usecache is already used in several javascripts. Something like usememcached for instance?

If such a pref is added, I would also expect to see it being used. But as I understand, the current test is still if $ENV{'MEMCACHED_SERVERS'} is filled in C4/Context. The value of this pref is never tested?

The modules moved to Koha still include references in comments to C4. Could you also eliminate those?

Chris C pointed me to the patches you wrote under 7249 which use the pref. So that question got some clarification.
Although that was the most important one, I still think renaming it would be better. Should not be too hard with an intelligent(!) Replace All ;)
And the C4 mentions ..

Another comment while browsing through 7249:
If we start using your pref, it would be better to eliminate here the old way of testing via $ENV. Make it uniform again.
If you do that, we get rid of a "preference" coming via koha-httpd file. Although it would be nice to seek for some concensus on the list, especially with those who worked on this before..

I like the idea of having a system preference to turn it off and on - this will make testing really easy too.

I'm uncomfortable with those patches and would like some more explanations
* the memcache information are in httpd.conf, right ? So what could be the use of a "usecache" syspref ? just be easier to switch on/off use of caching ?
*  some things like C4/Languages.pm use memcache as well. Should they rely on this Cache.pm or is it something different ?

(not changing the status, I just want to understand)

(In reply to comment #18)
> I'm uncomfortable with those patches and would like some more explanations
> * the memcache information are in httpd.conf, right ? So what could be the use
> of a "usecache" syspref ? just be easier to switch on/off use of caching ?
> *  some things like C4/Languages.pm use memcache as well. Should they rely on
> this Cache.pm or is it something different ?

Paul, could you also look at comment 12? We would not like to have multiple ways of doing the same.

+1 for a system preference to turn off/on memcache.

(In reply to comment #19)
> Paul, could you also look at comment 12? We would not like to have multiple
> ways of doing the same.

About comment 12 :
If we use syspref to get memcache server informations, it mean we can't cache the syspref (as we need the cache server information to get the cache address)
Isn't this a good reason to keep memcache server in httpd.conf ? what is/are the side effect if any of the $ENV ?

(In reply to comment #21)
> (In reply to comment #19)
> > Paul, could you also look at comment 12? We would not like to have multiple
> > ways of doing the same.
> 
> About comment 12 :
> If we use syspref to get memcache server informations, it mean we can't cache
> the syspref (as we need the cache server information to get the cache address)
> Isn't this a good reason to keep memcache server in httpd.conf ? what is/are
> the side effect if any of the $ENV ?

So there's a special case for the UseCache syspref. The side effect of keeping the memcache configuration in $ENV is that we have to explain to non-technical librarians how to adjust their caching settings if they run into problems. This seems like a significant disadvantage, for no real benefit.

Also, regarding the name of the syspref, I think UseCache is better than UseMemcached. One day we may have more than one cache technology available, at which point it could be changed from boolean to a choice, and UseCache = On remapped to UseCache = memcached.

(In reply to comment #22)
> > Isn't this a good reason to keep memcache server in httpd.conf ? what is/are
> > the side effect if any of the $ENV ?
> 
> So there's a special case for the UseCache syspref. The side effect of keeping
> the memcache configuration in $ENV is that we have to explain to non-technical
> librarians how to adjust their caching settings if they run into problems. This
> seems like a significant disadvantage, for no real benefit.
I think that if a librarian run into problems, the thing that must be done is emptying the cache, that must be done on command line. That's not something for anyone.
Setting the cache server is also something highly technical, and sysprefs should be dedicated to library/user-oriented things.
This setting is the same as for the LDAP = that's in koha.conf, not in syspref.
I think we should have as a rule that :
* technical parameters are in koha.xml or httpd.conf
* librarian parameters are in syspreferences


> Also, regarding the name of the syspref, I think UseCache is better than
> UseMemcached. One day we may have more than one cache technology available, at
> which point it could be changed from boolean to a choice, and UseCache = On
> remapped to UseCache = memcached.
agreed

I have no objection to the setting being in koha-conf.xml, if you feel there is no way to specify cache settings in system preferences (I'd argue that LDAP should also be specified in system preferences, and that it's oversight that it still requires editing configuration files manually). koha-conf is a place for *Koha* configuration settings. The Apache config is not.

I think we must discuss of where to put those files.

I plan to send mails to koha-devel about all "in discussion" bugs, will do that tomorrow

(In reply to comment #24)
> I have no objection to the setting being in koha-conf.xml, if you feel there is
> no way to specify cache settings in system preferences (I'd argue that LDAP
> should also be specified in system preferences, and that it's oversight that it
> still requires editing configuration files manually). koha-conf is a place for
> *Koha* configuration settings. The Apache config is not.

First, I just noticed this bug. I hope it is not late to step in.

I agree caching should be handled in a uniform and consistent way. The only reason to put memcached server(s) settings inside the Apache config is to be able to cache de koha-conf.xml file itself. This has been discussed on bug 6193. Otherwise, it can be put anywhere (koha-conf.xml or db).

There's also a second patch in bug 6193, that reuses those settings read from ENV, and the C4::Context->memcached object itself (and thus removes lots of calls to sysprefs in several koha libraries too).

Regarding the 'special-non-cached' syspref usecache, it could be tested before setting the memcached stuff in C4::Context and thus provide a way to disable caching for the librarians.

I sent an email to the list to discuss caching, as we have two different ways of dealing with it right now, and not a proper solution to cache invalidation, which actually annoys several librarians here (think of languages, changes on frameworks, etc). I hope we can discuss it on the list soon.

Following discussion & decision on koha-devel, the current patch won't be applied : we have decided to set caching at apache configuration level, bug 6193 has been applied and provide this feature/level of caching.

So the "usecache" syspref is now irrelevent (we use cache BEFORE reading syspref)
Using Koha/Cache is probably a good idea though, but the patch must be rewritten completly imo.

For now, all caching is made with the following code:

eval {
    if (C4::Context->ismemcached) {
        require Memoize::Memcached;
        import Memoize::Memcached qw(memoize_memcached);
        memoize_memcached('getTranslatedLanguages', memcached => C4::Context->memcached);
}

->ismemcached answer 1 if memcache is ON
C4::Context->memcached returns the handle of the memcache server

Some ideas behing this patch are great and should be reintroduced:
* use a central/unique Caching place = will let us change the caching system if we want, it's something we can't do with current system
* the cache flushing is something we must add, definetly.

So you're more than welcomed to rewrite your patch !

I would really like to see this feature in 3.8. I hope it can be fixed and will be happy to test again.

Created attachment 9360 [details] [review]
Bug 7248 Added caching support and and moved Caching into Koha namespace

Unit test for Koha/Cache.pm (which includes Koha/Cache/Memcached.pm)

Note that in order to test Koha::Cache you must export the environment variable
MEMCACHED_SERVERS. For example:
$ export MEMCACHED_SERVERS=127.0.0.1:11211

Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>

passing QA - passes perlcritic OK, and both tests pass OK

i've squashed the two patches into one, 
applied to 142c62081fc15bbd994e737eef55e29b61a6d730

Looking at how this caching works in bug 7249, I think this part of the code is wrong: 

+if ($usecache) {
+    require Koha::Cache;
+    Koha::Cache->import();
+    $cache = Koha::Cache->new(
+        {
+            'cache_type'    => 'memcached',
+            'cache_servers' => $ENV{'MEMCACHED_SERVERS'}
+        }
+    );
+    my $namespace = $ENV{'MEMCACHED_NAMESPACE'} || 'koha';
+    my $page = $cache->get_from_cache("$namespace:intranet:report:$report");
+    if ($page) {
+        print $query->header;
+        print $page;
+        exit;
+    }
+}

=> 'memcached' is hardcoded in the file that use caching, the caching mechanism must be generic.

The caching API must be something like:
use Koha::Cache;
my $cache_server = Koha::Cache->new();
if $cache_server( my $page = $cache->get_from_cache("intranet:report:$report");)

so it won't depend on the caching system & caching configuration.

I'll provide a patch for that ASAP (maybe today)

Hi Paul,
I understand what you mean, but couldn't you still push this? I see some reasons:
- adding new reports is currently totally broken in master
- I am quite sure we have more references for memcache that need to be cleaned up
- Koha does only support memcache right now, so it can be pushed without breaking anything
- the change you suggest could be done as a follow-up and perhaps do more clean-up in other places at the same time

(wait an hour or 2 before testing this patch, i'm working on t/Cache.t and cleaning other instances of caching)

If anyone can sign-off my 3 follow-ups...

there's only one missing, to remove ismemcache from Context.pm and Auth.pm, will do that once those have been pushed.

Then, i'll write a wiki page about caching

Created attachment 9571 [details] [review]
Bug 7248 follow-up (alternative)

This patch introduces some new features for caching system:
* the type of caching is retrieved from ENV variable in httpd configuration if not forced when the ->new() is called.
* if $ENV{DEBUG} is ON, you'll have feedback when something is set or read from cache
* the Koha::Cache->is_cache_active is now available and will return 1 if there is a caching system available. It's a replacement for C4::Context->ismemcached
* fixes the mistake for debug/compress_threshold and expire_time parameters

The 2 report web service modules have been updated to use this new API

Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>

Followup breaks out requirement of Koha::Cache to a universal use, then makes use of the new subroutine.  Also centralizes the cache params into Koha::Cache->new().  Also adds a pre-emptive check for ENV variables indicating caching; if not present, will continue to fall back to other methods.  Some additional warns if DEBUG is active.

All in all, looks okay, and applies cleanly (provided you apply the baseline patch first).  Marking Passed QA.

There have been no further reports of problems so I am marking this bug resolved.