Bug 20657 - Globally enable PRE_CHOMP and POST_CHOMP in Template Toolkit configuration
Summary: Globally enable PRE_CHOMP and POST_CHOMP in Template Toolkit configuration
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-04-25 14:44 UTC by Owen Leonard
Modified: 2024-03-17 23:21 UTC (History)
7 users (show)

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


Attachments
Bug 20657: Enable PRE_CHOMP, POST_CHOMP and TRIM by default it TT (773 bytes, patch)
2023-03-14 14:48 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20657: Remove explicit chomp in patronfields.inc (10.43 KB, patch)
2024-03-15 13:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20657: Explicit + in class (947 bytes, patch)
2024-03-15 14:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20657: Explicit + for TT vars in sentences (1.97 KB, patch)
2024-03-15 14:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20657: Add non-breaking space in alphabet list (979 bytes, patch)
2024-03-15 14:08 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2018-04-25 14:44:14 UTC
As it currently stands, the HTML output by Template Toolkit includes whitespace anywhere there was a template tag. For example, this is a portion of the generated source of the staff client home page:

 1
 2
 3
 4 <!DOCTYPE html>
 5 <!-- TEMPLATE FILE: intranet-main.tt -->
 6
 7
 8
 9
10 <html lang="en">
11 <head>
12
13 <title>Koha staff client</title>

Developer tools report: 26.69 KB / 5.83 KB transferred

In contrast, this what the same content looks like with PRE_CHOMP and  POST_CHOMP enabled:

1 <!DOCTYPE html>
2 <!-- TEMPLATE FILE: intranet-main.tt -->
3 <html lang="en"><head>
4 <title>Koha staff client</title>

Developer tools report: 23.29 KB / 5.65 KB transferred

In the samples I looked at this change did not make the HTML source significantly more difficult to read.
Comment 1 Jonathan Druart 2022-06-08 07:11:16 UTC
I've suggested that yesterday:

diff --git a/C4/Templates.pm b/C4/Templates.pm
index 789495f3fae..7f7b36e3dc6 100644
--- a/C4/Templates.pm
+++ b/C4/Templates.pm
@@ -72,6 +72,9 @@ sub new {
             INCLUDE_PATH => \@includes,
             FILTERS => {},
             ENCODING => 'UTF-8',
+            PRE_CHOMP => 3,
+            POST_CHOMP => 3,
+            TRIM => 1,
         }
     ) or die Template->error();
     my $self = {
@@ -126,6 +129,8 @@ sub output {
     binmode( STDOUT, ":encoding(UTF-8)" );
     $template->process( $self->filename, $vars, \$data )
       || die "Template process failed: ", $template->error();
+
+    $data =~ tr/ //s;
     return $data;
 }
Comment 2 Julian Maurice 2022-06-08 07:21:54 UTC
+    $data =~ tr/ //s;

This would squash *every* sequences of space into a single space, right ? I'm not sure if it's appropriate for every template (sometimes spaces have meaning, like in some MARC fields, or we want them, like in <pre></pre> blocks, ...)
Comment 3 Jonathan Druart 2022-06-08 09:15:24 UTC
(In reply to Julian Maurice from comment #2)
> +    $data =~ tr/ //s;
> 
> This would squash *every* sequences of space into a single space, right ?
> I'm not sure if it's appropriate for every template (sometimes spaces have
> meaning, like in some MARC fields, or we want them, like in <pre></pre>
> blocks, ...)

Yes, agreed, we should go with pre/post chomp first.
Comment 4 Tomás Cohen Arazi 2023-03-14 14:48:56 UTC
Created attachment 148167 [details] [review]
Bug 20657: Enable PRE_CHOMP, POST_CHOMP and TRIM by default it TT

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 Tomás Cohen Arazi 2023-03-14 14:49:23 UTC
Submitting to get the ball rolling.
Comment 6 Tomás Cohen Arazi 2023-03-14 14:50:59 UTC
For reference to the values picked by Jonathan on his proposal:

http://template-toolkit.org/docs/manual/Config.html#section_PRE_CHOMP_POST_CHOMP
Comment 7 Nick Clemens (kidclamp) 2023-03-15 12:07:23 UTC
Two issues I noticed on a quick test:
1 - Holdings tab on staff details is collapsed after this, you have to click to open/view
2 - Messages with spaces are affected by this:
    In transit fromCentervilletoSpringfieldsince03/09/2023
Comment 8 Tomás Cohen Arazi 2023-03-15 13:10:22 UTC
(In reply to Nick Clemens from comment #7)
> Two issues I noticed on a quick test:
> 1 - Holdings tab on staff details is collapsed after this, you have to click
> to open/view
> 2 - Messages with spaces are affected by this:
>     In transit fromCentervilletoSpringfieldsince03/09/2023

Yes, we would need to use this syntax on special cases [%+ +%] or so
Comment 9 Martin Renvoize 2023-03-15 13:12:33 UTC
I faced some challenges with chomping in bug 31028.. this patch kind of moves the problem really.

We've historically not been very good at being explicit about chomping flags in our templates and I've often found myself adding '-' to the opening or closing tags of template markers.  This patch changes the default from 'no chomp' to 'chomp always'.. I think this is an improvement given our general use.. but we will likely need to explicitly disable the chomping in a number of cases to keep intended whitespace as Nick spotted.  It also might be nice to clean up all uses of `-` as that's now effectively the detault.
Comment 10 Martin Renvoize 2023-03-15 13:13:56 UTC
I think this is probably a case of push early and fix occurences we spot in the wild quickly after.. it's going to be hard to test all pages to fix things inline here.
Comment 11 Martin Renvoize 2023-03-15 13:22:08 UTC
We do, however, also allow the use of our includes within notices.. we could end up "fixing" includes for this and breaking notices that use them.. perhaps we should also make this change in C4::Letters?
Comment 12 Katrin Fischer 2023-03-15 22:31:58 UTC
The translation process also removes spaces in some cases, so we'd need to test really well using translated templates too. I agree with Martin about the early push.
Comment 13 Martin Renvoize 2023-03-16 11:59:28 UTC
Perhaps this is another one to go for before hackfest and use some hackfest time to work through tidying up.?
Comment 14 Tomás Cohen Arazi 2023-03-16 12:07:32 UTC
(In reply to Martin Renvoize from comment #13)
> Perhaps this is another one to go for before hackfest and use some hackfest
> time to work through tidying up.?

Should we change this setting for notices?
Comment 15 Lucas Gass 2023-09-15 16:55:37 UTC
Is this ready for testing now?
Comment 16 Jonathan Druart 2024-03-15 13:52:29 UTC
Created attachment 163235 [details] [review]
Bug 20657: Remove explicit chomp in patronfields.inc
Comment 17 Jonathan Druart 2024-03-15 14:07:57 UTC
Created attachment 163239 [details] [review]
Bug 20657: Explicit + in class

We need a script to catch those ones
Comment 18 Jonathan Druart 2024-03-15 14:07:59 UTC
Created attachment 163240 [details] [review]
Bug 20657: Explicit + for TT vars in sentences

If inside div and span?
Comment 19 Jonathan Druart 2024-03-15 14:08:02 UTC
Created attachment 163241 [details] [review]
Bug 20657: Add non-breaking space in alphabet list
Comment 20 Jonathan Druart 2024-03-15 14:08:15 UTC
We are clearly not ready for that.
Comment 21 Jonathan Druart 2024-03-15 14:16:10 UTC
I will address the original issue (comment 0) on bug 36333.