A new bug (Bug 8385) highlights a problem already known : In Koha, we forbid the following syntax in tt files: <option value="foo" [% IF ( option.selected ) %] selected="selected" [% END %]>bar</option> We prefer : [% IF ( option.selected ) %] <option value="foo" selected="selected">bar</option> [% ELSE %] <option value="foo">bar</option> [% END %] For Bug 8385, if I want to make tt_valid.t happy, I should replace 8 lines (with the same structure as previous) with 40 lines. These new lines get nowhere. Less readability (discutable), less concise, a lot of useless lines, etc. Why ? I don't know exactly. The translator tool crashes if this kind of syntax occurs. In fact, after few test, it seems the problem existed with HTML::Template, but I don't get through with Template::Toolkit. It would seem that we can introduce a IF statement to add attributes like selected, checked or disabled (for example). Given that the string (between IF and END) does not contain translatable strings. I will propose a patch (for discussion and massive test). It would be great if several testers could check that this patch does not valid a broken template or a template containing non translatable strings. Like as Frédéric Demians said me, it is a time bomb and we have to be sure the templates do not passed as valid if they are not.
Created attachment 10744 [details] [review] Bug 8407: make tt_valid less strict (for non translatable strings) Proposed patch
Chris, could you give your opinion here ? That did not work at all with HTML::Template::Pro, but it seems it work well with T::T (jonathan, I've switched to need signoff)
These seems to work ok, the template still passes the translated test. Before the patch perl xt/author/translatable-templates.t 1..2 ok 1 - opac templates are translatable ok 2 - intranet templates are translatable xt/tt_valid.t 2012-07-13 08:04:57 christopher pts/2 1..2 not ok 1 - TT syntax: not using TT directive within HTML tag # Failed test 'TT syntax: not using TT directive within HTML tag' # at xt/tt_valid.t line 55. # Files list: # intranet-tmpl/prog/en/modules/admin/searchengine/solr/indexes.tt: 132, 133, 134, 135, 136, 137, 141, 144 ok 2 - Token received same as original put on stack # Looks like you failed 1 test of 2. After the patch perl xt/tt_valid.t 2012-07-13 08:09:14 christopher pts/2 1..2 not ok 1 - TT syntax: not using TT directive within HTML tag # Failed test 'TT syntax: not using TT directive within HTML tag' # at xt/tt_valid.t line 60. # Files list: # intranet-tmpl/prog/en/modules/admin/searchengine/solr/indexes.tt: 141, 144 ok 2 - Token received same as original put on stack # Looks like you failed 1 test of 2. Dropped the errors in the tt down from 8 to 2. Looking at that file It appears to still be the checked="checked" lines it doesn't like. If we fix that up, Ill do the first signoff.
Hi Chris, I proposed a patch for Bug 8385. It fixes these 2 errors.
Created attachment 10808 [details] [review] Bug 8407: make tt_valid less strict (for non translatable strings) Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
I have signed off, but will leave it needs signoff, I would like to get it a bit more testing. It seems safe, but If someone could test creating and updating the .po files for another language as well as installing them. I only did english-nz
I tested breaking the templates and checking the output of the test. I think maybe the test needs should be modified to ignore the keywords disabled, checked and selected in a TT directive. Example: <th [% UNLESS ( selected ) %]title="TESTING"[% END %]>Date</th> This will pass the test. I am not sure, if it really breaks translations. In my tests "TESTING" didn't show up in the updated po files. I am not sure, if this is reason to fail this an if it really breaks something. Frederic, maybe you can take a look and give your opinion?
Created attachment 10870 [details] [review] Bug 8407: make tt_valid less strict (for non translatable strings) Hi Katrin, Good catch :) This one fails for the following lines: <th [% UNLESS ( selected ) %] title="TESTING" [% END %]>Date</th> <th [% UNLESS ( selected ) %]title="TESTING"[% END %]>Date</th> <th [% UNLESS ( selected ) %] selected="selected" [% END %]>Date</th> and passes: <th [% UNLESS ( selected ) %]selected="selected"[% END %]>Date</th> <th [% UNLESS ( selected ) %]disabled="disabled"[% END %]>Date</th> <th [% UNLESS ( selected ) %]checked="checked"[% END %]>Date</th> Switch back to needs signedoff
I raise the priority for this bug. Indeed, Jenkins is not happy currently. Either this patch corrects and does not introduce regression, or I will submit a patch for Bug 8385 (which will replace 6 lines with 6*5 [useless] lines). Please test and report any eventual problems.
Have signed off again, I am happy with it. It fixes the problems katrin found. Works with Maori and English-NZ Drops the fails to 1, which bug 8385 fixes
Created attachment 11139 [details] [review] Bug 8407: make tt_valid less strict (for non translatable strings) Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
chris, could you describe the exact tests you made. I did the following: in memberentrygen.tt, replaced [% IF ( categoryloo.categorycodeselected ) %] <option value="[% categoryloo.categorycode %]" selected="selected" data-typename="[% typeloo.typename %]">[% categoryloo.categoryname %]</option> [% ELSE %] <option value="[% categoryloo.categorycode %]" data-typename="[% typeloo.typename %]">[% categoryloo.categoryname %]</option> by <option value="[% categoryloo.categorycode %]" [% IF ( categoryloo.categorycodeselected ) %]selected="selected"[% END %] data-typename="[% typeloo.typename %]" title="blablabla">[% categoryloo.categoryname %]</option> in english, no problems. then ./translate update fr-FR translate blablabla into bliblibli then ./translate install fr-FR switching to english, I get a very nasty: Template process failed: file error - parse error - /home/paul/koha.dev/koha-community/koha-tmpl/intranet-tmpl/prog/fr-FR/modules/members/memberentrygen.tt line 520: unexpected token (%) [% "[%">[% categoryloo.categoryname %] at /home/paul/koha.dev/koha-community/C4/Templates.pm line 127. at /home/paul/koha.dev/koha-community/members/memberentry.pl line 842 Checking the french templates, I see: [% FOREACH typeloo IN typeloop %] [% FOREACH categoryloo IN typeloo.categoryloop %] [% IF ( loop.first ) %] [% IF ( typeloo.typename_C ) %]<optgroup value="C" label="Enfant">[% END %] [% IF ( typeloo.typename_A ) %]<optgroup value="A" label="Adulte">[% END %] [% IF ( typeloo.typename_S ) %]<optgroup label="Staff" value="S">[% END %] [% IF ( typeloo.typename_I ) %]<optgroup value="I" label="Collectivité">[% END %] [% IF ( typeloo.typename_P ) %]<optgroup value="P" label="Professionnel">[% END %] [% IF ( typeloo.typename_X ) %]<optgroup value="X" label="Statistiques">[% END %] [% END %] <option data-typename="[% typeloo.typename %]" value="[% categoryloo.categorycode %]" categoryloo.categorycodeselected="categoryloo.categorycodeselected" %]="%]" if="IF" end="END" )=")" title="bliblibli" %]selected="selected" (="(" [%="[%">[% categoryloo.categoryname %]</option>
(In reply to comment #12) > switching to english, I get a very nasty: One must read "switching to french, I get a very nasty:...3 In english, everything is OK
I guess that proves that the problem still exists with TT :( So we shouldn't change the test and fix the templates.
Problem is in routine text_replace_tag (tmpl_process3.pl). I don't know how we can fix it :-/ I abandon this bug (for now...) and will submit a patch for Bug 8385 ;-(