Control: tag -1 - moreinfo Hi Simon,
Simon McVittie: > I have referenced an alternative solution (mostly code deletion) on the > upstream bug. Please try: > https://git.pseudorandom.co.uk/smcv/ikiwiki.git/commitdiff/wip/po-filter-every-time > I would particularly appreciate review and testing from intrigeri, who wrote > the code that I'm deleting and hopefully has a better picture of why it was/is > necessary. (I've tried to reply on the upstream bug but the anonpush mechanism seems to be broken and the CGI rejects my edit, on the grounds that the blogspam plugin is unhappy with it — presumably because I'm using Tor. So here we go :) First, thanks a lot for diving into this topic. Joey's commit 192ce7a2 was prompted by [[bugs/po_vs_templates]] and back then, it fixed that bug (and allowed me to remove some very ugly workarounds: d877b964, d4136aea), so surely it was _somewhat_ relevant to `po` users back then. But indeed, I doubt it was relevant for `inline`, which is a different beast, in that it still calls `filter` itself on the inlined content. I fully agree that "output of a filter hook is never passed back through filter hooks" should be an invariant. I've just spent quite some time trying to understand what these "preprocessing loops" were, and I've failed. I suspect that's related to the code the "Avoid loops of preprocessed pages preprocessing" comment in `IkiWiki.pm` is about, but I can't find how it could be. So I'm afraid I won't be able to shed any light on this: ten years have passed and I'm sorry my commit message back then was suboptimal :/ I'm not keen on keeping a workaround (the `alreadyfiltered` mechanism) for an under-specified bug that surely existed 10 years ago but might have been fixed since then, especially when this workaround itself clearly causes a well understood bug which causes major trouble for what might be the main user of this plugin nowadays (the Tails website, for which the `po` plugin was developed in the first place). So in principle, I'm very much in favor of removing the buggy workaround as you did. I've tested your proposed branch locally on the Tails website (957 Markdown and HTML files, 1758 PO files), that uses nested `inline`, `pagetemplate`, `toggle`, `sidebar` and more. I could not spot any issue, be it after a full rebuild, or when triggering an incremental refresh after modifying some pages involved in the most intense usage we have of the `po` plugin combined with these other ones. I will now deploy a modified version of ikiwiki, that includes your patch, on our production website, which will give it exposure to more realistic usage. I'll report back in 7-10 days, which hopefully should leave sufficient time for getting the fix in Buster; but if this timeframe is not adequate for you, feel free to release and upload without waiting for further test results from me. > (I would also like to have more extensive test coverage for the po plugin in > general: I don't think any of the ikiwiki maintainers use it, so automated > tests are the only way we can make sure it hasn't regressed.) I share these feelings: even though I'm using this plugin a lot, I deeply regret having written it before I learnt much about software testing. I can't realistically promise I'll increase the test coverage in general, but in the future I'll try my best to at least add tests for the areas affected by changes I'll submit (there's one more branch in the pipeline), simply because I have happily unlearned how to write code without writing tests. Cheers, -- intrigeri
