]> sipb.mit.edu Git - ikiwiki.git/blob - doc/plugins/po/discussion.mdwn
Merge branch 'filter-full'
[ikiwiki.git] / doc / plugins / po / discussion.mdwn
1 [[!toc ]]
2
3 ----
4
5 # Security review
6
7 ## Probable holes
8
9 _(The list of things to fix.)_
10
11 ### po4a-gettextize
12
13 * po4a CVS 2009-01-16
14 * Perl 5.10.0
15
16 `po4a-gettextize` uses more or less the same po4a features as our
17 `refreshpot` function.
18
19 Without specifying an input charset, zzuf'ed `po4a-gettextize` quickly
20 errors out, complaining it was not able to detect the input charset;
21 it leaves no incomplete file on disk. I therefore had to pretend the
22 input was in UTF-8, as does the po plugin.
23
24         zzuf -c -s 13 -r 0.1 \
25             po4a-gettextize -f text -o markdown -M utf-8 -L utf-8 \
26              -m GPL-3 -p GPL-3.pot
27
28 Crashes with:
29
30         Malformed UTF-8 character (UTF-16 surrogate 0xdfa4) in substitution
31         iterator at /usr/share/perl5/Locale/Po4a/Po.pm line 1449.
32         Malformed UTF-8 character (fatal) at /usr/share/perl5/Locale/Po4a/Po.pm
33         line 1449.
34
35 An incomplete pot file is left on disk. Unfortunately Po.pm tells us
36 nothing about the place where the crash happens.
37
38 > It's fairly standard perl behavior when fed malformed utf-8. As long
39 > as it doesn't crash ikiwiki, it's probably acceptable. Ikiwiki can
40 > do some similar things itself when fed malformed utf-8 (doesn't
41 > crash tho) --[[Joey]]
42
43 ----
44
45 ## Potential gotchas
46
47 _(Things not to do.)_
48
49
50 ### Blindly activating more po4a format modules
51
52 The format modules we want to use have to be checked, as not all are
53 safe (e.g. the LaTeX module's behaviour is changed by commands
54 included in the content); they may use regexps generated from
55 the content.
56
57 ----
58
59 ## Hopefully non-holes
60
61 _(AKA, the assumptions that will be the root of most security holes...)_
62
63 ### PO file features
64
65 No [documented](http://www.gnu.org/software/gettext/manual/gettext.html#PO-Files)
66 directive that can be put in po files is supposed to cause mischief
67 (ie, include other files, run commands, crash gettext, whatever).
68
69 ### gettext
70
71 #### Security history
72
73 The only past security issue I could find in GNU gettext is
74 [CVE-2004-0966](http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2004-0966),
75 *i.e.* [Debian bug #278283](http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=278283):
76 the autopoint and gettextize scripts in the GNU gettext package (1.14
77 and later versions) may allow local users to overwrite files via
78 a symlink attack on temporary files.
79
80 This plugin would not have allowed to exploit this bug, as it does not
81 use, either directly or indirectly, the faulty scripts.
82
83 Note: the lack of found security issues can either indicate that there
84 are none, or reveal that no-one ever bothered to find or publish them.
85
86 #### msgmerge
87
88 `refreshpofiles()` runs this external program.
89
90 * I was not able to crash it with `zzuf`.
91 * I could not find any past security hole.
92
93 #### msgfmt
94
95 `isvalidpo()` runs this external program.
96
97 * I was not able to make it behave badly using zzuf: it exits cleanly
98   when too many errors are detected.
99 * I could not find any past security hole.
100
101 ### po4a
102
103 #### Security history
104
105 The only past security issue I could find in po4a is
106 [CVE-2007-4462](http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2007-4462):
107 `lib/Locale/Po4a/Po.pm` in po4a before 0.32 allowed local users to
108 overwrite arbitrary files via a symlink attack on the
109 gettextization.failed.po temporary file.
110
111 This plugin would not have allowed to exploit this bug, as it does not
112 use, either directly or indirectly, the faulty `gettextize` function.
113
114 Note: the lack of found security issues can either indicate that there
115 are none, or reveal that no-one ever bothered to find or publish them.
116
117 #### General feeling
118
119 Are there any security issues on running po4a on untrusted content?
120
121 To say the least, this issue is not well covered, at least publicly:
122
123 * the documentation does not talk about it;
124 * grep'ing the source code for `security` or `trust` gives no answer.
125
126 On the other hand, a po4a developer answered my questions in
127 a convincing manner, stating that processing untrusted content was not
128 an initial goal, and analysing in detail the possible issues.
129 The following analysis was done with his help.
130
131 #### Details
132
133 * the core (`Po.pm`, `Transtractor.pm`) should be safe
134 * po4a source code was fully checked for other potential symlink
135   attacks, after discovery of one such issue
136 * the only external program run by the core is `diff`, in `Po.pm` (in
137   parts of its code we don't use)
138 * `Locale::gettext` is only used to display translated error messages
139 * Nicolas François "hopes" `DynaLoader` is safe, and has "no reason to
140   think that `Encode` is not safe"
141 * Nicolas François has "no reason to think that `Encode::Guess` is not
142   safe". The po plugin nevertheless avoids using it by defining the
143   input charset (`file_in_charset`) before asking `TransTractor` to
144   read any file. NB: this hack depends on po4a internals.
145
146 ##### Locale::Po4a::Text
147
148 * does not run any external program
149 * only `do_paragraph()` builds regexp's that expand untrusted
150   variables; according to [[Joey]], this is "Freaky code, but seems ok
151   due to use of `quotementa`".
152
153 ##### Locale::Po4a::Xhtml
154
155 * does not run any external program
156 * does not build regexp's from untrusted variables
157
158 => Seems safe as far as the `includessi` option is disabled; the po
159 plugin explicitly disables it.
160
161 Relies on Locale::Po4a::Xml` to do most of the work.
162
163 ##### Locale::Po4a::Xml
164
165 * does not run any external program
166 * the `includeexternal` option makes it able to read external files;
167   the po plugin explicitly disables it
168 * untrusted variables are escaped when used to build regexp's
169
170 ##### Text::WrapI18N
171
172 `Text::WrapI18N` can cause DoS
173 ([Debian bug #470250](http://bugs.debian.org/470250)).
174 It is optional, and we do not need the features it provides.
175
176 If a recent enough po4a (>=0.35) is installed, this module's use is
177 fully disabled. Else, the wiki administrator is warned about this
178 at runtime.
179
180 ##### Term::ReadKey
181
182 `Term::ReadKey` is not a hard dependency in our case, *i.e.* po4a
183 works nicely without it. But the po4a Debian package recommends
184 `libterm-readkey-perl`, so it will probably be installed on most
185 systems using the po plugin.
186
187 `Term::ReadKey` has too far reaching implications for us to
188 be able to guarantee anything wrt. security.
189
190 If a recent enough po4a (>=2009-01-15 CVS, which will probably be
191 released as 0.35) is installed, this module's use is fully disabled.
192
193 ##### Fuzzing input
194
195 ###### po4a-translate
196
197 * po4a CVS 2009-01-16
198 * Perl 5.10.0
199
200 `po4a-translate` uses more or less the same po4a features as our
201 `filter` function.
202
203 Without specifying an input charset, same behaviour as
204 `po4a-gettextize`, so let's specify UTF-8 as input charset as of now.
205
206 `LICENSES` is a 21M file containing 100 concatenated copies of all the
207 files in `/usr/share/common-licenses/`; I had no existing PO file or
208 translated versions at hand, which renders these tests
209 quite incomplete.
210
211         zzuf -cv -s 0:10 -r 0.001:0.3 \
212           po4a-translate -d -f text -o markdown -M utf-8 -L utf-8 \
213             -k 0 -m LICENSES -p LICENSES.fr.po -l test.fr
214
215 ... seems to lose the fight, at the `readpo(LICENSES.fr.po)` step,
216 against some kind of infinite loop, deadlock, or any similar beast.
217
218 The root of this bug lies in `Text::WrapI18N`, see the corresponding
219 section.
220
221
222 ----
223
224 ## Fixed holes
225
226
227 ----
228
229 # original contrib/po page, with old commentary
230
231 I've been working on a plugin called "po", that adds support for multi-lingual wikis,
232 translated with gettext, using [po4a](http://po4a.alioth.debian.org/).
233
234 More information:
235
236 * It can be found in my "po" branch:
237   `git clone git://gaffer.ptitcanardnoir.org/ikiwiki.git`
238 * It is self-contained, *i.e.* it does not modify ikiwiki core at all.
239 * It is documented (including TODO and plans for next work steps) in
240   `doc/plugins/po.mdwn`, which can be found in the same branch.
241 * No public demo site is available so far, I'm working on this.
242
243 My plan is to get this plugin clean enough to be included in ikiwiki.
244
245 The current version is a proof-of-concept, mature enough for me to dare submitting it here,
246 but I'm prepared to hear various helpful remarks, and to rewrite parts of it as needed.
247
248 Any thoughts on this?
249
250 > Well, I think it's pretty stunning what you've done here. Seems very
251 > complete and well thought out. I have not read the code in great detail
252 > yet.
253
254 > Just using po files is an approach I've never seen tried with a wiki. I
255 > suspect it will work better for some wikis than others. For wikis that
256 > just want translations that match the master language as closely as
257 > possible and don't wander off and diverge, it seems perfect. (But what happens
258 > if someone edits the Discussion page of a translated page?)
259
260 > Please keep me posted, when you get closer to having all issues solved
261 > and ready for merging I can do a review and hopefully help with the
262 > security items you listed. --[[Joey]]
263
264 >> Thanks a lot for your quick review, it's reassuring to hear such nice words
265 >> from you. I did not want to design and write a full translation system, when
266 >> tools such as gettext/po4a already have all the needed functionality, for cases
267 >> where the master/slave languages paradigm fits.
268 >> Integrating these tools into ikiwiki plugin system was a pleasure.
269 >>
270 >> I'll tell you when I'm ready for merging, but in the meantime,
271 >> I'd like you to review the changes I did to the core (3 added hooks).
272 >> Can you please do this? If not, I'll go on and hope I'm not going to far in
273 >> the wrong direction.
274 >>
275 >>> Sure.. I'm not completly happy with any of the hooks since they're very
276 >>> special purpose, and also since `run_hooks` is not the best interface
277 >>> for a hook that modifies a variable, where only the last hook run will
278 >>> actually do anything. It might be better to just wrap
279 >>> `targetpage`, `bestlink`, and `beautify_urlpath`. But, I noticed
280 >>> the other day that such wrappers around exported functions are only visible by
281 >>> plugins loaded after the plugin that defines them.
282 >>> 
283 >>> Update: Take a look at the new "Function overriding" section of
284 >>> [[plugins/write]]. I think you can just inject wrappers about a few ikiwiki
285 >>> functions, rather than adding hooks. The `inject` function is pretty
286 >>> insane^Wlow level, but seems to work great. --[[Joey]]
287 >>>
288 >>>> Thanks a lot, it seems to be a nice interface for what I was trying to achieve.
289 >>>> I may be forced to wait two long weeks before I have a chance to confirm
290 >>>> this. Stay tuned. --[[intrigeri]]
291 >>>>
292 >>>>> I've updated the plugin to use `inject`. It is now fully self-contained,
293 >>>>> and does not modify the core anymore. --[[intrigeri]]
294 >>
295 >> The Discussion pages issue is something I am not sure about yet. But I will
296 >> probably decide that "slave" pages, being only translations, don't deserve
297 >> a discussion page: the discussion should happen in the language in which the
298 >> pages are written for real, which is the "master" one. --[[intrigeri]]
299 >> 
300 >> I think that's a good decision, you don't want to translate discussion,
301 >> and if the discussion page turns out multilingual, well, se la vi. ;-)
302 >> 
303 >> Relatedly, what happens if a translated page has a broken link, and you
304 >> click on it to edit it? Seems you'd first have to create a master page
305 >> and could only then translate it, right? I wonder if this will be clear
306 >> though to the user.
307 >>
308 >>> Right: a broken link points to the URL that allows to create
309 >>> a page that can either be a new master page or a non-translatable
310 >>> page, depending on `po_translatable_pages` value. The best
311 >>> solution I can thing of is to use [[plugins/edittemplate]] to
312 >>> insert something like "Warning: this is a master page, that must
313 >>> be written in $MASTER_LANGUAGE" into newly created master pages,
314 >>> and maybe another warning message on newly created
315 >>> non-translatable pages. It seems quite doable to me, but in order
316 >>> to avoid breaking existing functionality, it implies to hack a bit
317 >>> [[plugins/edittemplate]] so that multiple templates can be
318 >>> inserted at page creation time. [[--intrigeri]]
319 >>>
320 >>>> I implemented such a warning using the formbuilder_setup hook.
321 >>>> --[[intrigeri]]
322 >>
323 >> And also, is there any way to start a translation of a page into a new
324 >> lanauge using the web interface?
325 >>
326 >>> When a new language is added to `po_slave_languages`, a rebuild is
327 >>> triggered, and all missing PO files are created and checked into
328 >>> VCS. An unpriviledged wiki user can not add a new language to
329 >>> `po_slave_languages`, though. One could think of adding the needed
330 >>> interface to translate a page into a yet-unsupported slave
331 >>> language, and this would automagically add this new language to
332 >>> `po_slave_languages`. It would probably be useful in some
333 >>> usecases, but I'm not comfortable with letting unpriviledged wiki
334 >>> users change the wiki configuration as a side effect of their
335 >>> actions; if this were to be implemented, special care would be
336 >>> needed. [[--intrigeri]]
337 >>>
338 >>>> Actually I meant into any of the currently supported languages.
339 >>>> I guess that if the template modification is made, it will list those
340 >>>> languages on the page, and if a translation to a language is missing,
341 >>>> the link will allow creating it?
342 >>>>
343 >>>>> Any translation page always exist for every supported slave
344 >>>>> language, even if no string at all have been translated yet.
345 >>>>> This implies the po plugin is especially friendly to people who
346 >>>>> prefer reading in their native language if available, but don't
347 >>>>> mind reading in English else.
348 >>>>>
349 >>>>> While I'm at it, there is a remaining issue that needs to be
350 >>>>> sorted out: how painful it could be for non-English speakers
351 >>>>> (assuming the master language is English) to be perfectly able
352 >>>>> to navigate between translation pages supposed to be written in
353 >>>>> their own language, when their translation level is most
354 >>>>> often low.
355 >>>>>
356 >>>>> (It is currently easy to display this status on the translation
357 >>>>> page itself, but then it's too late, and how frustrating to load
358 >>>>> a page just to realize it's actually not translated enough for
359 >>>>> you. The "other languages" loop also allows displaying this
360 >>>>> information, but it is generally not the primary
361 >>>>> navigation tool.)
362 >>>>>
363 >>>>> IMHO, this is actually a social problem (i.e. it's no use adding
364 >>>>> a language to the supported slave ones if you don't have the
365 >>>>> manpower to actually do the translations), that can't be fully
366 >>>>> solved by technical solutions, but I can think of some hacks
367 >>>>> that would limit the negative impact: a given translation's
368 >>>>> status (currently = percent translated) could be displayed next
369 >>>>> to the link that leads to it; a color code could as well be used
370 >>>>> ("just" a matter of adding a CSS id or class to the links,
371 >>>>> depending on this variable). As there is already work to be done
372 >>>>> to have the links text generation more customizable through
373 >>>>> plugins, I could do both at the same time if we consider this
374 >>>>> matter to be important enough. --[[intrigeri]]
375 >>>>>
376 >>>>>> The translation status in links is now implemented in my
377 >>>>>> `po`branch. It requires my `meta` branch changes to
378 >>>>>> work, though. I consider the latter to be mature enough to
379 >>>>>> be merged. --[[intrigeri]]
380
381 >> FWIW, I'm tracking your po branch in ikiwiki master git in the po
382 >> branch. One thing I'd like to try in there is setting up a translated
383 >> basewiki, which seems like it should be pretty easy to do, and would be
384 >> a great demo! --[[Joey]]
385 >>
386 >>> I have a complete translation of basewiki into danish, available merged into
387 >>> ikiwiki at git://source.jones.dk/ikiwiki-upstream (branch underlay-da), and am working with
388 >>> others on preparing one in german.  For a complete translated user
389 >>> experience, however, you will also need templates translated (there are a few
390 >>> translatable strings there too).  My most recent po4a Markdown improvements
391 >>> adopted upstream but not yet in Debian (see
392 >>> [bug#530574](http://bugs.debian.org/530574)) correctly handles multiple
393 >>> files in a single PO which might be relevant for template translation handling.
394 >>> --[[JonasSmedegaard]]
395 >>
396 >>> I've merged your changes into my own branch, and made great
397 >>> progress on the various todo items. Please note my repository
398 >>> location has changed a few days ago, my user page was updated
399 >>> accordingly, but I forgot to update this page at the same time.
400 >>> Hoping it's not too complicated to relocated an existing remote...
401 >>> (never done that, I'm a Git beginner as well as a Perl
402 >>> newbie) --[[intrigeri]]
403 >>>>
404 >>>> Just a matter of editing .git/config, thanks for the heads up.
405 >>>>>
406 >>>>> Joey, please have a look at my branch, your help would be really
407 >>>>> welcome for the security research, as I'm almost done with what
408 >>>>> I am able to do myself in this area. --[[intrigeri]]
409 >>>>>>
410 >>>>>> I came up with a patch for the WrapI18N issue --[[Joey]]
411
412 I've set this plugin development aside for a while. I will be back and
413 finish it at some point in the first quarter of 2009. --[[intrigeri]]
414
415 > Abstract: Joey, please have a look at my po and meta branches.
416
417 > Detailed progress report:
418
419 > * it seems the po branch in your repository has not been tracking my
420 >   own po branch for two months. any config issue?
421 > * all the plugin's todo items have been completed, robustness tests
422 >   done
423 > * I've finished the detailed security audit, and the fix for po4a
424 >   bugs has entered upstream CVS last week
425 > * I've merged your new `checkcontent` hook with the `cansave` hook
426 >   I previously introduced in my own branch; blogspam plugin updated
427 >   accordingly
428 > * the rename hook changes we discussed elsewhere are also part of my
429 >   branch
430 > * I've introduced two new hooks (`canremove` and `canrename`), not
431 >   a big deal; IMHO, they extend quite logically the plugin interface
432 > * as highlighted on [[bugs/pagetitle_function_does_not_respect_meta_titles]],
433 >   my `meta` branch contains a new feature that is really useful in a
434 >   translatable wiki
435
436 > As a conclusion, I'm feeling that my branches are ready to be
437 > merged; only thing missing, I guess, are a bit of discussion and
438 > subsequent adjustments.
439
440 > --[[intrigeri]]
441
442 > I've looked it over and updated my branch with some (untested)
443 > changes.
444
445 >> I've merged your changes into my branch. Only one was buggy.
446
447 > Sorry, I'd forgotten about your cansave hook.. sorry for the duplicate
448 > work there.
449
450 > Reviewing the changes, mostly outside of `po.pm`, I have
451 > the following issues.
452 >  
453 > * renamepage to renamelink change would break the ikiwiki
454 >   3.x API, which I've promised not to do, so needs to be avoided
455 >   somehow. (Sorry, I guess I dropped the ball on not getting this
456 >   API change in before cutting 3.0..)
457 >> 
458 >> Fixed, see [[todo/need_global_renamepage_hook]].
459 >>
460 > * I don't understand the parentlinks code change and need to figure it
461 >   out. Can you explain what is going on there?
462 >> 
463 >> I'm calling `bestlink` there so that po's injected `bestlink` is
464 >> run. This way, the parent links of a page link to the parent page
465 >> version in the proper language, depending on the
466 >> `po_link_to=current` and `po_link_to=negotiated` settings.
467 >> Moreover, when using my meta branch enhancements plus meta title to
468 >> make pages titles translatable, this small patch is needed to get
469 >> the translated titles into parentlinks.
470 >> 
471 > * canrename's mix of positional and named parameters is way too
472 >   ugly to get into an ikiwiki API. Use named parameters
473 >   entirely. Also probably should just use named parameters
474 >   for canremove.
475 > * `skeleton.pm.example`'s canrename needs fixing to use either
476 >   the current or my suggested parameters.
477 >> 
478 >> Done.
479 >> 
480 > * I don't like the exporting of `%backlinks` and `$backlinks_calculated`
481 >   (the latter is exported but not used).
482 >> 
483 >> The commit message for 85f865b5d98e0122934d11e3f3eb6703e4f4c620
484 >> contains the rationale for this change. I guess I don't understand
485 >> the subtleties of `our` use, and perldoc does not help me a lot.
486 >> IIRC, I actually did not use `our` to "export" these variables, but
487 >> rather to have them shared between `Render.pm` uses.
488 >>
489 >>> My wording was unclear, I meant exposing. --[[Joey]]
490 >>>  
491 >>>> I guess I still don't know Perl's `our` enough to understand clearly.
492 >>>> No matter whether these variables are declared with `my` or `our`,
493 >>>> any plugin can `use IkiWiki::Render` and then access
494 >>>> `$IkiWiki::backlinks`, as already does e.g. the pagestat plugin.
495 >>>> So I guess your problem is not with letting plugins use these
496 >>>> variables, but with them being visible for every piece of
497 >>>> (possibly external) code called from `Render.pm`. Am I right?
498 >>>> If I understand clearly, using a brace block to lexically enclose
499 >>>> these two `our` declarations, alongside with the `calculate_backlinks`
500 >>>> and `backlinks` subs definitions, would be a proper solution, wouldn't
501 >>>> it? --[[intrigeri]]
502 >>>>
503 >>>>> No, %backlinks and the backlinks() function are not the same thing.
504 >>>>> The variable is lexically scoped; only accessible from inside
505 >>>>> `Render.pm` --[[Joey]] 
506 >>>> 
507 > * What is this `IkiWiki::nicepagetitle` and why are you
508 >   injecting it into that namespace when only your module uses it?
509 >   Actually, I can't even find a caller of it in your module.
510 >> 
511 >> I guess you should have a look to my `meta` branch and to
512 >> [[bugs/pagetitle_function_does_not_respect_meta_titles]] in order
513 >> to understand this :)
514 >>
515 >>> It would probably be good if I could merge this branch without 
516 >>> having to worry about also immediatly merging that one. --[[Joey]] 
517 >>> 
518 >>>> I removed all dependencies on my `meta` branch from the `po` one.
519 >>>> This implied removing the `po_translation_status_in_links` and
520 >>>> `po_strictly_refresh_backlinks` features, and every link text is now
521 >>>> displayed in the master language. I believe the removed features really
522 >>>> enhance user experience of a translatable wiki, that's why I was
523 >>>> initially supposing the `meta` branch would be merged first.
524 >>>> IMHO, we'll need to come back to this quite soon after `po` is merged.
525 >>>> --[[intrigeri]]
526 >>>>
527 >>>> Maybe you should keep those features in a meta-po branch?
528 >>>> I did a cursory review of your meta last night, have some issues with it, 
529 >>>> but this page isn't the place for a detailed review. --[[Joey]] 
530 >>>>
531 >>>>> Done. --[[intrigeri]]
532 >>> 
533 > * I'm very fearful of the `add_depends` in `indexhtml`. 
534 >   Does this make every page depend on every page that links
535 >   to it? Won't this absurdly bloat the dependency pagespecs
536 >   and slow everything down? And since nicepagetitle is given
537 >   as the reason for doing it, and nicepagetitle isn't used,
538 >   why do it?
539 >> 
540 >> As explained in the 85f865b5d98e0122934d11e3f3eb6703e4f4c620 log:
541 >> this feature hits performance a bit. Its cost was quite small in my
542 >> real-world use-cases (a few percents bigger refresh time), but
543 >> could be bigger in worst cases. When using the po plugin with my
544 >> meta branch changes (i.e. the `nicepagetitle` thing), and having
545 >> enabled the option to display translation status in links, this
546 >> maintains the translation status up-to-date in backlinks. Same when
547 >> using meta title to make the pages titles translatable. It does
548 >> help having a nice and consistent translated wiki, but as it can
549 >> also involve problems, I just turned it into an option.
550 >> 
551 >>> This has been completely removed for now due to the removal of
552 >>> the dependency on my `meta` branch. --[[intrigeri]]
553 >> 
554 > * The po4a Suggests should be versioned to the first version
555 >   that can be used safely, and that version documented in 
556 >   `plugins/po.mdwn`.
557 >>
558 >> Done.
559 >> 
560 >> --[[intrigeri]]
561
562 > --[[Joey]] 
563
564 I reverted the `%backlinks` and `$backlinks_calculated` exposing.
565 The issue they were solving probably will arise again when I'll work
566 on my meta branch again (i.e. when the simplified po one is merged),
567 but the po thing is supposed to work without these ugly `our`.
568 Seems like it was the last unaddressed item from Joey's review, so I'm
569 daring a timid "please pull"... or rather, please review again :)
570 --[[intrigeri]]
571
572 > Ok, I've reviewed and merged into my own po branch. It's looking very
573 > mergeable.
574
575 > * Is it worth trying to fix compatability with `indexpages`?
576 >> 
577 >> Supporting `usedirs` being enabled or disabled was already quite
578 >> hard IIRC, so supporting all four combinations of `usedirs` and
579 >> `indexpages` settings will probably be painful. I propose we forget
580 >> about it until someone reports he/she badly needs it, and then
581 >> we'll see what can be done.
582 >> 
583 > * Would it make sense to go ahead and modify `page.tmpl` to use
584 >   OTHERLANGUAGES and PERCENTTRANSLATED, instead of documenting how to modify it?
585 >> 
586 >> Done in my branch.
587 >> 
588 > * Would it be better to disable po support for pages that use unsupported
589 >   or poorly-supported markup languages?
590
591 >> I prefer keeping it enabled, as:
592 >> 
593 >> * most wiki markups "almost work"
594 >> * when someone needs one of these to be fully supported, it's not
595 >>   that hard to add dedicated support for it to po4a; if it were
596 >>   disabled, I fear the ones who could do this would maybe think
597 >>   it's blandly impossible and give up.
598 >> 
599  
600 > * What's the reasoning behind checking that the link plugin
601 >   is enabled? AFAICS, the same code in the scan hook should
602 >   also work when other link plugins like camelcase are used.
603 >> 
604 >> That's right, fixed.
605 >> 
606 > * In `pagetemplate` there is a comment that claims the code
607 >   relies on `genpage`, but I don't see how it does; it seems
608 >   to always add a discussion link?
609 >> 
610 >> It relies on IkiWiki::Render's `genpage` as this function sets the
611 >> `discussionlink` template param iff it considers a discussion link
612 >> should appear on the current page. That's why I'm testing
613 >> `$template->param('discussionlink')`.
614 >> 
615 >>> Maybe I was really wondering why it says it could lead to a broken
616 >>> link if the cgiurl is disabled. I think I see why now: Discussionlink
617 >>> will be set to a link to an existing disucssion page, even if cgi is
618 >>> disabled -- but there's no guarantee of a translated discussion page
619 >>> existing in that case. *However*, htmllink actually checks
620 >>> for this case, and will avoid generating a broken link so AFAICS, the
621 >>> comment is actually innacurate.. what will really happen in this case
622 >>> is discussionlink will be set to a non-link translation of
623 >>> "discussion". Also, I consider `$config{cgi}` and `%links` (etc)
624 >>> documented parts of the plugin interface, which won't change; po could
625 >>> rely on them to avoid this minor problem. --[[Joey]] 
626 >>>> 
627 >>>> Done in my branch. --[[intrigeri]]
628 >>>> 
629 >
630 > * Is there any real reason not to allow removing a translation?
631 >   I'm imagining a spammy translation, which an admin might not
632 >   be able to fix, but could remove.
633 >> 
634 >> On the other hand, allowing one to "remove" a translation would
635 >> probably lead to misunderstandings, as such a "removed" translation
636 >> page would appear back as soon as it is "removed" (with no strings
637 >> translated, though). I think an admin would be in a position to
638 >> delete the spammy `.po` file by hand using whatever VCS is in use.
639 >> Not that I'd really care, but I am slightly in favour of the way
640 >> it currently works.
641 >>
642 >>> That would definitly be confusing. It sounds to me like if we end up
643 >>> needing to allow web-based deletion of spammy translations, it will
644 >>> need improvements to the deletion UI to de-confuse that. It's fine to
645 >>> put that off until needed --[[Joey]] 
646 >> 
647 > * Re the meta title escaping issue worked around by `change`. 
648 >   I suppose this does not only affect meta, but other things
649 >   at scan time too. Also, handling it only on rebuild feels
650 >   suspicious -- a refresh could involve changes to multiple
651 >   pages and trigger the same problem, I think. Also, exposing
652 >   this rebuild to the user seems really ugly, not confidence inducing.
653 >   
654 >   So I wonder if there's a better way. Such as making po, at scan time,
655 >   re-run the scan hooks, passing them modified content (either converted
656 >   from po to mdwn or with the escaped stuff cheaply de-escaped). (Of
657 >   course the scan hook would need to avoid calling itself!)
658
659 >   (This doesn't need to block the merge, but I hope it can be addressed
660 >   eventually..)
661 >  
662 > --[[Joey]] 
663 >> 
664 >> I'll think about it soon.
665 >> 
666 >> --[[intrigeri]]
667 >>
668 >>> Did you get a chance to? --[[Joey]] 
669
670  * As discussed at [[todo/l10n]] the templates needs to be translatable too.  They
671    should be treated properly by po4a using the markdown option - at least with my
672    later patches in [bug#530574](http://bugs.debian.org/530574)) applied.
673
674  * It seems to me that the po plugin (and possibly other parts of ikiwiki) wrongly
675    uses gettext.  As I understand it, gettext (as used currently in ikiwiki) always
676    lookup a single language, That might make sense for a single-language site, but
677    multilingual sites should emit all strings targeted at the web output in each own
678    language.
679
680    So generally the system language (used for e.g. compile warnings) should be separated
681    from both master language and slave languages.
682
683    Preferrably the gettext subroutine could be extended to pass locale as optional
684    secondary parameter overriding the default locale (for messages like "N/A" as
685    percentage in po plugin).  Alternatively (with above mentioned template support)
686    all such strings could be externalized as templates that can then be localized.
687
688 # Robustness tests
689
690 ### Enabling/disabling the plugin
691
692 * enabling the plugin with `po_translatable_pages` set to blacklist: **OK**
693 * enabling the plugin with `po_translatable_pages` set to whitelist: **OK**
694 * enabling the plugin without `po_translatable_pages` set: **OK**
695 * disabling the plugin: **OK**
696
697 ### Changing the plugin config
698
699 * adding existing pages to `po_translatable_pages`: **OK**
700 * removing existing pages from `po_translatable_pages`: **OK**
701 * adding a language to `po_slave_languages`: **OK**
702 * removing a language from `po_slave_languages`: **OK**
703 * changing `po_master_language`: **OK**
704 * replacing `po_master_language` with a language previously part of
705   `po_slave_languages`: needs two rebuilds, but **OK** (this is quite
706   a perverse test actually)
707
708 ### Creating/deleting/renaming pages
709
710 All cases of master/slave page creation/deletion/rename, both via RCS
711 and via CGI, have been tested.
712
713 ### Misc
714
715 * general test with `usedirs` disabled: **OK**
716 * general test with `indexpages` enabled: **not OK**
717 * general test with `po_link_to=default` with `userdirs` enabled: **OK**
718 * general test with `po_link_to=default` with `userdirs` disabled: **OK**
719
720 Duplicate %links ?
721 ------------------
722
723 I notice code in the scan hook that seems to assume
724 that %links will accumulate duplicate links for a page.
725 That used to be so, but the bug was fixed. Does this mean
726 that po might be replacing the only link on a page, in error? 
727 --[[Joey]] 
728
729 > It would replace it. The only problematic case is when another
730 > plugin has its own reasons, in its `scan` hook, to add a page
731 > that is already there to `$links{$page}`. This other plugin's
732 > effect might then be changed by po's `scan` hook... which could
733 > be either good (better overall l10n) or bad (break the other
734 > plugin's goal). --[[intrigeri]]
735
736 >> Right.. well, the cases where links are added is very small.
737 >> Grepping for `add_link`, it's just done by link, camelcase, meta, and
738 >> tag. All of these are supposed to work just link regular links
739 >> so I'd think that is ok. We could probably remove the currently scary
740 >> comment about only wanting to change the first link. --[[Joey]] 
741
742 >>> Commit 3c2bffe21b91684 in my po branch does this. --[[intrigeri]]
743 >>>> Cherry-picked --[[Joey]]