Bug 13948 - Add ability to dump template toolkit variables to html comment
Summary: Add ability to dump template toolkit variables to html comment
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-02 15:02 UTC by Kyle M Hall
Modified: 2016-12-05 21:22 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 13948 - Add ability to dump template toolkit variables to html comment (9.80 KB, patch)
2015-04-02 15:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13948 - Add ability to dump template toolkit variables to html comment (10.07 KB, patch)
2015-04-02 15:35 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13948 - Add ability to dump template toolkit variables to html comment (10.45 KB, patch)
2015-04-16 17:33 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 13948 - Add ability to dump template toolkit variables to html comment (10.71 KB, patch)
2015-05-12 16:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13948 [QA Followup] - Make dependency optional (768 bytes, patch)
2015-05-12 16:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13948: Prevent explosion when Template::Plugin::Stash not installed (3.11 KB, patch)
2015-05-15 04:52 UTC, Mark Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 13948: Add ability to dump template toolkit variables to html comment (10.83 KB, patch)
2015-05-16 19:47 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 13948 [QA Followup] - Make dependency optional (891 bytes, patch)
2015-05-16 19:47 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 13948: Prevent explosion when Template::Plugin::Stash not installed (3.33 KB, patch)
2015-05-16 19:48 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 13948: Add ability to dump template toolkit variables to html comment (10.83 KB, patch)
2015-05-24 12:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13948 [QA Followup] - Make dependency optional (892 bytes, patch)
2015-05-24 12:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13948: Prevent explosion when Template::Plugin::Stash not installed (3.33 KB, patch)
2015-05-24 12:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13948: Add ability to dump template toolkit variables to html comment (11.18 KB, patch)
2015-06-08 13:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13948: Add ability to dump template toolkit variables to html comment (11.18 KB, patch)
2015-06-08 13:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13948 [QA Followup] - Make dependency optional (900 bytes, patch)
2015-06-08 13:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13948: Prevent explosion when Template::Plugin::Stash not installed (3.33 KB, patch)
2015-06-08 13:53 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2015-04-02 15:02:07 UTC
It would be incredibly helpful if we could easily enable Koha to dump all Template Toolkit variables to a comment for debugging purposes.
Comment 1 Kyle M Hall 2015-04-02 15:34:03 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2015-04-02 15:35:34 UTC Comment hidden (obsolete)
Comment 3 Marc Véron 2015-04-02 17:31:20 UTC
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.
Comment 4 Marc Véron 2015-04-02 18:13:41 UTC
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.
Comment 5 Barton Chittenden 2015-04-03 21:01:00 UTC
(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.
Comment 6 Mark Tompsett 2015-04-16 17:33:12 UTC Comment hidden (obsolete)
Comment 7 Mark Tompsett 2015-04-16 17:35:27 UTC
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 8 Jonathan Druart 2015-04-20 11:27:40 UTC
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.
Comment 9 Kyle M Hall 2015-05-12 16:43:09 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2015-05-12 16:43:16 UTC Comment hidden (obsolete)
Comment 11 Mark Tompsett 2015-05-12 17:14:49 UTC
(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?
Comment 12 Kyle M Hall 2015-05-14 10:57:49 UTC
(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.
Comment 13 Mark Tompsett 2015-05-15 04:52:39 UTC Comment hidden (obsolete)
Comment 14 Mark Tompsett 2015-05-15 04:55:08 UTC
(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.
Comment 15 Bernardo Gonzalez Kriegel 2015-05-16 19:47:51 UTC Comment hidden (obsolete)
Comment 16 Bernardo Gonzalez Kriegel 2015-05-16 19:47:58 UTC Comment hidden (obsolete)
Comment 17 Bernardo Gonzalez Kriegel 2015-05-16 19:48:05 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2015-05-18 09:22:18 UTC
I am not sure it's a good idea to test dependencies on TT side.
Comment 19 Kyle M Hall 2015-05-18 10:54:58 UTC
(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.
Comment 20 Jonathan Druart 2015-05-18 11:11:51 UTC
(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.
Comment 21 Mark Tompsett 2015-05-18 11:37:23 UTC
(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. :)
Comment 22 Jonathan Druart 2015-05-24 12:56:47 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2015-05-24 12:56:51 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2015-05-24 12:56:55 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2015-05-24 12:58:22 UTC
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.
Comment 26 Robin Sheat 2015-05-24 23:45:11 UTC
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.
Comment 27 Robin Sheat 2015-05-25 06:05:25 UTC
(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
Comment 28 Kyle M Hall 2015-06-08 13:42:10 UTC Comment hidden (obsolete)
Comment 29 Kyle M Hall 2015-06-08 13:52:30 UTC
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>
Comment 30 Kyle M Hall 2015-06-08 13:53:28 UTC
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>
Comment 31 Kyle M Hall 2015-06-08 13:53:45 UTC
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>
Comment 32 Tomás Cohen Arazi 2015-07-28 13:39:14 UTC
Patches pushed to master.

Thanks Kyle!