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.
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; }
+ $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, ...)
(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.
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>
Submitting to get the ball rolling.
For reference to the values picked by Jonathan on his proposal: http://template-toolkit.org/docs/manual/Config.html#section_PRE_CHOMP_POST_CHOMP
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
(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
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.
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.
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?
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.
Perhaps this is another one to go for before hackfest and use some hackfest time to work through tidying up.?
(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?
Is this ready for testing now?
Created attachment 163235 [details] [review] Bug 20657: Remove explicit chomp in patronfields.inc
Created attachment 163239 [details] [review] Bug 20657: Explicit + in class We need a script to catch those ones
Created attachment 163240 [details] [review] Bug 20657: Explicit + for TT vars in sentences If inside div and span?
Created attachment 163241 [details] [review] Bug 20657: Add non-breaking space in alphabet list
We are clearly not ready for that.
I will address the original issue (comment 0) on bug 36333.