X-Git-Url: http://git.vanrenterghem.biz/git.ikiwiki.info.git/blobdiff_plain/d72e5fd58c121582b2a2a1feca39596c29a1feb0..ebc5016cbbb344e1d84807b749e250bf4fafcbcb:/doc/bugs/template_creation_error.mdwn?ds=sidebyside diff --git a/doc/bugs/template_creation_error.mdwn b/doc/bugs/template_creation_error.mdwn index 79dccc136..9d6915b09 100644 --- a/doc/bugs/template_creation_error.mdwn +++ b/doc/bugs/template_creation_error.mdwn @@ -109,3 +109,178 @@ Please, let me know what to do to avoid this kind of error. > for backwards compatibility? > > --[[smcv]] + +>> OK, here is a branch implementing what I said. It adds the `definetemplate` +>> directive to [[plugins/goodstuff]] as its last commit. +>> +>> Templates with the current strange semantics will still work, until +>> IkiWiki breaks compatibility. +>> +>> Possible controversies: +>> +>> * Should the `definetemplate` plugin be core, or in goodstuff, or neither? +>> +>> * Should \[[!definetemplate]] be allowed on any page (with the implementation +>> of `template("foo")` looking for a `definetemplate` in `templates/foo`, +>> then a `definetemplate` in `foo`, then fall back to the current logic)? +>> If not, should \[[!definetemplate]] raise an error when used on a page not +>> in `templates/`, since it will have no practical effect there? +>> +>> * Is it OK to rely on `definetemplate` being enabled in the basewiki's +>> templates? +>> +>> * Should the "use definetemplate" wording in the documentation of +>> template and edittemplate be stronger? Should those plugins automatically +>> load definetemplate? +>> +>> --[[smcv]] + +>>> this looks like a good idea to me. +>>> +>>> * i'd put it in core, and add a transition for the time compatibility gets +>>> broken, provided the transitioning system will be used in that. templates +>>> can't be expected to just work as markdown+ikiwiki too. +>>> +>>> (it being in core would also solve my qualms about `section => "web"` / +>>> `\[[!tag type/web]]`). +>>> +>>> * if definetemplate gets deemed core, no "use definetemplate!" notes on the +>>> template/edittemplate pages will be required any more. +>>> +>>> * first i was sceptical of the approach of re-running scan to make sure the +>>> `my %templates` is filled, but it is indeed a practical solution. +>>> +>>> * the name "`definetemplate`" gives me the first impression that something +>>> is assigned (as in `#define`), but actually it highlights a region in the +>>> file. wouldn't "`templatebody`" be a better description of the meaning of +>>> the directive? +>>> +>>> --[[chrysn]] + +>>>> Thanks for your feedback! +>>>> Looking at its description on this wiki, I agree that `type/web` doesn't +>>>> fit, and core does seem better. I like your `templatebody` suggestion, +>>>> too, particularly if templates remain restricted to `/templates`. +>>>> I'll try to come up with better wording for the documentation to say +>>>> "use `templatebody`, like this", with a note about backwards +>>>> compatibility later. +>>>> +>>>> Rationale for `my %templates`: yes it does seem a bit odd, but +>>>> if I used `$pagestate{$tpage}{template}` instead of a `my` variable, +>>>> I'd sometimes _still_ have to force a `scan`, because +>>>> [[plugins/template]] has to expand the template at scan time so that +>>>> it can contain links etc. - so I have to make sure that if the +>>>> template has changed, it has already been scanned (scanning happens +>>>> in random order, so that can't be guaranteed). This means there's +>>>> no benefit in reading it back from the index, so it might as well +>>>> just be in-memory. +>>>> +>>>> I suppose an alternative way to do it would be to remember what was +>>>> passed to `needsbuild`, and only force a `scan` for templates that +>>>> were in that list - which potentially reduces CPU time and I/O a +>>>> little, in exchange for a bigger index. I could do that if Joey +>>>> wants me to, but I think the current approach is simpler, +>>>> so I'll stick with the current approach if it isn't vetoed. +>>>> --[[smcv]] + +>>>>> @name: even outside `/templates`, `\[[!templatebody]]` would be +>>>>> interpreted as "when this page is used as a template, this is what its +>>>>> contents should be", and be suitable. +>>>>> +>>>>> @`%templates`: my surprise wasn't to it not being in `%pagestate`, but +>>>>> rather that the `scan` function was used for it at all, rather than plain +>>>>> directive parsing that ignores everything else -- but i agree that it's +>>>>> the right thing to do in this situation. +>>>>> +>>>>> --[[chrysn]] + +---- + +[[!template id=gitbranch author="[[smcv]]" branch=smcv/ready/templatebody + browse=http://git.pseudorandom.co.uk/smcv/ikiwiki.git/shortlog/refs/heads/ready/templatebody]] +[[!tag patch users/smcv/ready]] +Branch and directive renamed to `ready/templatebody` as chrysn suggested. +It's on-by-default now (or will be if that branch is merged). +Joey, any chance you could review this? + +There is one known buglet: `template_syntax.t` asserts that the entire +file is a valid HTML::Template, whereas it would ideally be doing the +same logic as IkiWiki itself. I don't think that's serious. --[[smcv]] + +> Looking over this, I notice it adds a hash containing all scanned +> files. This seems to me to be potentially a scalability problem on +> rebuild of a site with many pages. Ikiwiki already keeps a lot +> of info in memory, and this adds to it, for what is a fairly +> minor reason. It seems to me there should be a way to avoid this. --[[Joey]] + +>> Maybe. Are plugins expected to cope with scanning the same +>> page more than once? If so, it's just a tradeoff between +>> "spend more time scanning the template repeatedly" and +>> "spend more memory on avoiding it", and it would be OK to +>> omit that, or reduce it to a set of scanned *templates* +>> (in practice that would mean scanning each template twice +>> in a rebuild). --s +>>> [Commit f7303db5](http://source.ikiwiki.branchable.com/?p=source.git;a=commitdiff;h=f7303db5) +>>> suggests that scanning the same page more than once is problematic, +>>> so that solution is probably not going to work. +>>> +>>> The best idea I've come up with so far is to track whether +>>> we're in the scan or render phase. If we're in the scan +>>> phase, I think we do need to keep track of which pages +>>> we've scanned, so we don't do them again? (Or perhaps that's +>>> unnecessary - commit f7303db5 removed a scan call that's in +>>> the render phase.) If we're in the render phase, we can assume +>>> that all changed pages have been scanned already, so we can +>>> drop the contents of `%scanned` and rely on a single boolean +>>> flag instead. +>>> +>>>> This is not actually good enough for the templatebody +>>>> directive, which does in fact need to scan certain pages +>>>> during the render phase, namely when a page that is rendered +>>>> due to dependencies uses a template that no other page being +>>>> rendered in this pass was using. I've reverted this optimization, +>>>> to fix [[wrong rendering of templatebody]], and applied a more +>>>> limited version which only optimizes rebuilds (the worst case +>>>> of this memory consumption). --[[smcv]] +>>> +>>> `%scanned` is likely to be no larger than `%rendered`, which +>>> we already track, and whose useful lifetime does not overlap +>>> with `%scanned` now. I was tempted to merge them both and call +>>> the result `%done_in_this_phase`, but that would lead to really +>>> confusing situations if a bug led to `render` being called sooner +>>> than it ought to be. +>>> +>>> My ulterior motive here is that I would like to formalize +>>> the existence of different phases of wiki processing - at the +>>> moment there are at least two phases, namely "it's too soon to +>>> match pagespecs reliably" and "everything has been scanned, +>>> you may use pagespecs now", but those phases don't have names, +>>> so [[plugins/write]] doesn't describe them. +>>> +>>> I'm also considering adding warnings +>>> if people try to match a pagespec before scanning has finished, +>>> which can't possibly guarantee the right result, as discussed in +>>> [[conditional_preprocess_during_scan]]. My `wip-too-soon` branch +>>> is a start towards that; the docwiki builds successfully, but +>>> the tests that use IkiWiki internals also need updating to +>>> set `$phase = PHASE_RENDER` before they start preprocessing. --s + +>>>> reviewing those modifications, i think this is a good way to go. along +>>>> with warning about pagespecs evaluated in scan phase, i think it should be +>>>> an error to invoke scan in the render phase; that would mean that +>>>> `readtemplate` needs to check whether it's invoked as a scan or not to +>>>> decide whether to scan the template page, but would be generally more +>>>> robust for future plugin writing. +>>>> +>>>>> At the moment templatebody really does need to re-scan templates in +>>>>> the render phase, unfortunately. Not scanning in the render phase +>>>>> seems to be precisely how [[wrong rendering of templatebody]] +>>>>> happened. --s +>>>> +>>>> **addendum**: if the new phase state is used to create warnings/errors +>>>> about improper ikiwiki api use of plugins (which is something i'd +>>>> advocate), that should likewise warn if `add_link` actually adds a link in +>>>> the render phase. such a warning would have helped spotting the +>>>> link-related [[template evaluation oddities]] earlier. --[[chrysn]] + +>>>>> [[Merged|done]] --[[smcv]]