Bug 7387 - Add Template::Toolkit plugin to allow caching of includes
Summary: Add Template::Toolkit plugin to allow caching of includes
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P1 - high new feature (vote)
Assignee: Chris Cormack
QA Contact: Elliott Davis
URL:
Keywords:
Depends on: 929 7248
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-29 19:28 UTC by Chris Cormack
Modified: 2014-05-26 21:04 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 7387 : Adding a caching plugin for Template::Toolkit (4.85 KB, patch)
2011-12-29 20:26 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 7387 : Adding a caching plugin for Template::Toolkit (4.92 KB, patch)
2012-01-28 15:08 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 7387 : Adding a caching plugin for Template::Toolkit (4.20 KB, patch)
2013-01-02 03:32 UTC, Chris Cormack
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7387 : Adding a caching plugin for Template::Toolkit (4.36 KB, patch)
2013-02-19 01:37 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Follow-up Bug 7387 - Add Template::Toolkit plugin to allow caching of includes (1.64 KB, patch)
2013-02-19 01:37 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 7387 : Adding a caching plugin for Template::Toolkit (4.42 KB, patch)
2013-03-05 18:24 UTC, Elliott Davis
Details | Diff | Splinter Review
Follow-up Bug 7387 - Add Template::Toolkit plugin to allow caching of includes (1.70 KB, patch)
2013-03-05 18:25 UTC, Elliott Davis
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2011-12-29 19:28:17 UTC

    
Comment 1 Chris Cormack 2011-12-29 20:26:51 UTC Comment hidden (obsolete)
Comment 2 Jared Camins-Esakov 2012-01-28 15:08:36 UTC Comment hidden (obsolete)
Comment 3 Paul Poulain 2012-02-03 10:43:12 UTC
Setting this one to "in discussion", as it's linked to bug 7248

QA comment: chris, there is no explanation of the positive effect of this. What would be the next step if this patch is included ? could we do that with all includes ? more explanation welcomed !
Comment 4 Paul Poulain 2012-03-30 10:24:38 UTC
I remove the "in discussion" status. This patch is very interesting, but bug 7248 does not apply anymore, so this patch can't apply until 7248 has been rewritten.
Comment 5 Jonathan Druart 2012-06-12 12:57:02 UTC
Bug 7248 is pushed, I switch this one to "Signed off"
Comment 6 Ian Walls 2012-06-15 18:13:59 UTC
Mostly just adding the new plugin, which appears to be functional.  Example of usage in intranet-main, to get the doc-head-open.inc for 6 minutes.  Should be an easy enough thing to revert if we later find some deep dark nastiness, but I can't see any right now, and will mark Passed QA.

Note that 3-way merge is required... git bz is not applying for me here directly.
Comment 7 Paul Poulain 2012-06-20 19:59:10 UTC
(In reply to comment #3)
> QA comment: chris, there is no explanation of the positive effect of this.
> What would be the next step if this patch is included ? could we do that
> with all includes ? more explanation welcomed !

Chris, before pushing this patch, i'd like to understand what will be the next step. Should all templates be switched to:
-[% INCLUDE 'doc-head-open.inc' %]
+[% USE cache = Cache %]
+[% cache.inc(
+    'template' => 'doc-head-open.inc',
+    'keys' => {'dochead' => 'dochead'},
+    'ttl' => 360
+    )
+%]

?
(if that's the case, then I'll add this to my next RM newsletter & send a mail to koha-devel about that)
Do you already have a list of other .inc we could cache too ?
Comment 8 Chris Cormack 2012-06-20 22:25:22 UTC
(In reply to comment #7)
> (In reply to comment #3)
> > QA comment: chris, there is no explanation of the positive effect of this.
> > What would be the next step if this patch is included ? could we do that
> > with all includes ? more explanation welcomed !
> 
> Chris, before pushing this patch, i'd like to understand what will be the
> next step. Should all templates be switched to:
> -[% INCLUDE 'doc-head-open.inc' %]
> +[% USE cache = Cache %]
> +[% cache.inc(
> +    'template' => 'doc-head-open.inc',
> +    'keys' => {'dochead' => 'dochead'},
> +    'ttl' => 360
> +    )
> +%]
> 
> ?
> (if that's the case, then I'll add this to my next RM newsletter & send a
> mail to koha-devel about that)
> Do you already have a list of other .inc we could cache too ?

Nope d, in fact caching that one won't win us much at all it's fast to render anyway. But looking into caching things that require computation, would be good. This patch is just to enable people to cache includes when they find ones, or even better create includes and cache them.
Comment 9 Paul Poulain 2012-06-27 13:42:27 UTC
(In reply to comment #8)
> Nope d, in fact caching that one won't win us much at all it's fast to
> render anyway. But looking into caching things that require computation,
> would be good. This patch is just to enable people to cache includes when
> they find ones, or even better create includes and cache them.

mmm... I'm not sure this patch should be pushed as is : if it's a good idea, the 
-[% INCLUDE 'doc-head-open.inc' %]
+[% USE cache = Cache %]
+[% cache.inc(
+    'template' => 'doc-head-open.inc',
+    'keys' => {'dochead' => 'dochead'},
+    'ttl' => 360
+    )
+%]
is useless, so should be removed, and a more useful example used ?

QA comment: isn't 
{'cache_servers' => C4::Context->config('memcached_servers') }

as memcache is the default server for Koha::Cache ?

marking failed QA for now
Comment 10 Chris Cormack 2013-01-02 03:32:11 UTC Comment hidden (obsolete)
Comment 11 Bernardo Gonzalez Kriegel 2013-02-19 01:37:07 UTC Comment hidden (obsolete)
Comment 12 Bernardo Gonzalez Kriegel 2013-02-19 01:37:29 UTC Comment hidden (obsolete)
Comment 13 Elliott Davis 2013-03-05 18:24:59 UTC
Created attachment 15892 [details] [review]
Bug 7387 : Adding a caching plugin for Template::Toolkit

To use this you need to do something like

-[% INCLUDE 'doc-head-open.inc' %]
+[% USE cache = Cache %]
+[% cache.inc(
+    'template' => 'doc-head-open.inc',
+    'keys' => {'dochead' => 'dochead'},
+    'ttl' => 360
+    )
+%]

Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Comment: Works. Tested modifying as described opac-main.tt
Follow up to fix tabulations.

Signed-off-by: Elliott Davis <elliott@bywatersolions.com>
Comment 14 Elliott Davis 2013-03-05 18:25:13 UTC
Created attachment 15893 [details] [review]
Follow-up Bug 7387 - Add Template::Toolkit plugin to allow caching of includes

Fixes tabulations.

Signed-off-by: Elliott Davis <elliott@bywatersolions.com>
Comment 15 Jared Camins-Esakov 2013-03-07 15:59:27 UTC
This patch has been pushed to master.