Great idea!

However, with patch I get in staff client:
Software error:

Template process failed: plugin error - Stash: plugin not found at /usr/share/kohaclone/C4/Templates.pm line 121.

Found:
Template::Plugin::Stash (1.006) 	Not Installed 

However: If somebody enables the system preference DumpTemplateVarsIntranet without first checking the Perl modules on the About page, he is locked out and can not set back the preference, beause the template process fails on every page.

Maybe this should be handled more gracefully, e.g. by checking if the perl module is installed before using it. Best way would be to write a warning as HTML comment instead of the template variables.

(In reply to Marc Véron from comment #4)
> Found:
> Template::Plugin::Stash (1.006) 	Not Installed 
> 
> However: If somebody enables the system preference DumpTemplateVarsIntranet
> without first checking the Perl modules on the About page, he is locked out
> and can not set back the preference, beause the template process fails on
> every page.
> 
> Maybe this should be handled more gracefully, e.g. by checking if the perl
> module is installed before using it. Best way would be to write a warning as
> HTML comment instead of the template variables.

I got the same error -- Template::Stash is part of template toolkit, but  Template::Plugin::Stash is not.

If what I wrote in my comment is true, then I would fail this. But since I don't have the time to check, I'm signing off, in hopes that QA will have time to confirm one way or another.

Comment on attachment 37981 [details] [review]
Bug 13948 - Add ability to dump template toolkit variables to html comment

Review of attachment 37981 [details] [review]:
-----------------------------------------------------------------

Did you check the availability of Template::Plugin::Stash?
I am on jessie, and don't find it packaged.
The module does not seem to have been updated for a while, 2004...
http://search.cpan.org/~podmaster/Template-Plugin-Stash-1.006/lib/Template/Plugin/Stash.pm

::: C4/Installer/PerlDependencies.pm
@@ +533,5 @@
> +        'min_ver'  => '2.22',
> +      },
> +    'Template::Plugin::Stash' => {
> +        'usage'    => 'Debugging',
> +        'required' => '1',

I would say it's not required.

::: installer/data/mysql/updatedatabase.pl
@@ +10028,5 @@
>  
> +$DBversion = "XXX";
> +if ( CheckVersion($DBversion) ) {
> +    my $rs = Koha::Database->new()->schema()->resultset('Systempreference');
> +    $rs->find_or_create(

Did we decide on this way to insert sysprefs?
Now we have a way to prevent conflicts on this file it would be bad not to use it.

(In reply to Kyle M Hall from comment #10)
> Created attachment 39097 [details] [review] [review]
> Bug 13948 [QA Followup] - Make dependency optional

This doesn't answer the implied question from what I wrote:
"Additionally:
 http://www.template-toolkit.org/docs/modules/Template/Plugin/index.html
 does not contain Stash. I suspect this was how it was
 introduced initially by TT."

Do you even need to mention it? Is not stash already part of TT?

(In reply to M. Tompsett from comment #11)
> (In reply to Kyle M Hall from comment #10)
> > Created attachment 39097 [details] [review] [review] [review]
> > Bug 13948 [QA Followup] - Make dependency optional
> 
> This doesn't answer the implied question from what I wrote:
> "Additionally:
>  http://www.template-toolkit.org/docs/modules/Template/Plugin/index.html
>  does not contain Stash. I suspect this was how it was
>  introduced initially by TT."
> 
> Do you even need to mention it? Is not stash already part of TT?

It appears that while Template::Stash is part of TT, Template::Plugin::Stash is not part of TT, so we do need to have it as a separate dependency.

(In reply to Kyle M Hall from comment #12)
> It appears that while Template::Stash is part of TT, Template::Plugin::Stash
> is not part of TT, so we do need to have it as a separate dependency.

Okay. Also, I didn't like the explosion in that bad configuration case. I fixed it. Could you test it, Kyle? If it works, let's set this signed off. In the mean time, I'll put it back to Needs Signoff.

I am not sure it's a good idea to test dependencies on TT side.

(In reply to Jonathan Druart from comment #18)
> I am not sure it's a good idea to test dependencies on TT side.

Can you explain why? It seems like a good solution here, especially considering  that this feature would rarely if ever be used in production.

(In reply to Kyle M Hall from comment #19)
> (In reply to Jonathan Druart from comment #18)
> > I am not sure it's a good idea to test dependencies on TT side.
> 
> Can you explain why? It seems like a good solution here, especially
> considering  that this feature would rarely if ever be used in production.

We have a script to know when deps are missing (and it's also displayed on the about page). With this patch, we will have checks in pm, pl and tt files.

As you said, this feature should only be enabled on a dev env, and a developer will know what to do if the missing deps error appears.

But it's not a big deal and I don't want to block this enh for this reason.

(In reply to Jonathan Druart from comment #20)
> We have a script to know when deps are missing (and it's also displayed on
> the about page). With this patch, we will have checks in pm, pl and tt files.

Which you can't get to, if you enable it and don't catch the error.
Who regularly visits the about page before changing system preferences?


> As you said, this feature should only be enabled on a dev env, and a
> developer will know what to do if the missing deps error appears.

It should be noted that sometimes the person playing with this in a development environment is not a developer. We have librarians with no linux and/or development experience trying to use Koha. A cleaner catch is a good thing for them.


> But it's not a big deal and I don't want to block this enh for this reason.

Thanks. :)

Robin,
This patch set introduces a new deps Template::Plugin::Stash.
It looks quite easy to package but I would like to get your opinion to be sure.

It looks pretty straightforward. There are some unrelated technical issues stopping me doing it right now, hopefully I'll figure them out and get back to it tomorrow.

(In reply to Jonathan Druart from comment #25)
> Robin,
> This patch set introduces a new deps Template::Plugin::Stash.
> It looks quite easy to package but I would like to get your opinion to be
> sure.

Jonathan, that wasn't even a challenge ;) libtemplate-plugin-stash-perl is in the Koha repo now.

Also, if it's working right this second: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=786742

Created attachment 39995 [details] [review]
Bug 13948: Add ability to dump template toolkit variables to html comment

It would be incredibly helpful if we could easily enable Koha to dump
all Template Toolkit variables to a comment for debugging purposes.

Test Plan:
1) Apply this patch
2) Run updatedatabase
3) Enable the new system preferences DumpTemplateVarsIntranet and
   DumpTemplateVarsOpac
4) Load a page in the staff intranet, view the html source
5) Note the template toolkit variables are embedded in an html comment
6) Load a page in the opac, view the html source
7) Note the template toolkit variable are embedded in an html comment

NOTE: I had to cpan2deb Template::Plugin::Stash to test.
      This is not optimal. Additionally:
      http://www.template-toolkit.org/docs/modules/Template/Plugin/index.html
      does not contain Stash. I suspect this was how it was
      introduced initially by TT.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org>

Created attachment 39996 [details] [review]
Bug 13948 [QA Followup] - Make dependency optional

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org>

Created attachment 39997 [details] [review]
Bug 13948: Prevent explosion when Template::Plugin::Stash not installed

TEST PLAN
---------
1) Install first two patches
2) do not install, or uninstall Template::Plugin::Stash
3) Upgrade to make sure system preference is added.
4) Set the system preference to turn it on for Staff and OPAC
5) Refresh staff -- kaboom
6) Load OPAC -- kaboom
7) Apply this patch
8) Reload staff and OPAC
   -- nice HTML comment about what is wrong.
9) run koha qa test tools.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Neat, runs well. Tested with/without sysprefs and Template::Plugin::Stash
No koha-qa errors

Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org>

Patches pushed to master.

Thanks Kyle!