Merge branch 'ready/comments'
[ikiwiki] / doc / bugs / template_creation_error.mdwn
1 Hi,
2 I am trying to build a template. The compilation of this template results in a weird exception. I have isolated the cause of the exception to the following point:
3
4 If i have this in the template code:
5
6 \[[!inline<br/>
7 pages="\<TMPL_VAR SEL_PAGES\>"<br/>
8 template=extract-entry<br/>
9 \]]<br/>
10
11 There is no problem at all. I can use the template with the desired result. But if I try to use this (just adding the "show" parameter):
12
13 \[[!inline <br/>
14 pages="\<TMPL_VAR SEL_PAGES>"<br/>
15 template=extract-entry<br/>
16 show=\<TMPL_VAR CNTPG><br/>
17 \]]<br/>
18
19 I get this exception on the Git bash console:
20
21 <pre>
22 $ git push
23 Counting objects: 7, done.
24 Delta compression using up to 8 threads.
25 Compressing objects: 100% (4/4), done.
26 Writing objects: 100% (4/4), 410 bytes, done.
27 Total 4 (delta 3), reused 0 (delta 0)
28 remote: From /home/b-odelama-com/source
29 remote:    eb1421e..5e1bac5  master     -> origin/master
30 remote: Argument "\x{3c}\x{54}..." isn't numeric in numeric lt (<) at /usr/share/perl5/IkiWiki/Plugin/inline.pm line 231.
31 remote: Argument "\x{3c}\x{54}..." isn't numeric in numeric lt (<) at /usr/share/perl5/IkiWiki/Plugin/inline.pm line 231.
32 To ssh://b-odelama-com@odelama-com.branchable.com/
33    eb1421e..5e1bac5  master -> master
34 </pre>
35
36 Please, let me know what to do to avoid this kind of error.
37
38 > When you add a template page `templates/foo.mdwn` for use
39 > the [[ikiwiki/directive/template]] directive, two things happen:
40 >
41 > 1. `\[[!template id=foo ...]]` becomes available;
42 > 2. a wiki page `templates/foo` is built, resulting in a HTML file,
43 >    typically `templates/foo/index.html`
44 >
45 > The warnings you're seeing are the second of these: when ikiwiki
46 > tries to process `templates/foo.mdwn` as an ordinary page, without
47 > interpreting the `<TMPL_VAR>` directives, `inline` receives invalid
48 > input.
49 >
50 > This is a bit of a design flaw in [[plugins/template]] and
51 > [[plugins/edittemplate]], I think - ideally it would be possible to
52 > avoid parts of the page being interpreted when the page is being
53 > rendered normally rather than being used as a template.
54 >
55 > There *is* a trick to avoid parts of the page being interpreted when
56 > the page is being used as a template, while having them appear
57 > when it's rendered as a page:
58 >
59 >     <TMPL_IF FALSE>
60 >     <!-- This part only appears when being used as a page.
61 >          It assumes that you never set FALSE to a true value :-) -->
62 >     \[[!meta robots="noindex,nofollow"]]
63 >     This template is used to describe a thing. Parameters:
64 >     * name: the name of the thing
65 >     * size: the size of the thing
66 >     </TMPL_IF>
67 >
68 >     The thing is called <TMPL_VAR name> and its size is <TMPL_VAR size>
69 >
70 > I suppose you could maybe extend that to something like this:
71 >
72 >     <TMPL_IF FALSE>
73 >     <!-- This part only appears when being used as a page.
74 >          It assumes that you never set FALSE to a true value :-) -->
75 >     \[[!meta robots="noindex,nofollow"]]
76 >     This template is used to describe a thing. Parameters:
77 >     * name: the name of the thing
78 >     * size: the size of the thing
79 >     </TMPL_IF>
80 >
81 >     <TMPL_IF FALSE>
82 >     \[[!if test="included() and !included()" then="""
83 >     </TMPL_IF>
84 >     <!-- This part only appears when being used as a template. It also
85 >          assumes that you never set FALSE to a true value, and it
86 >          relies on the [[ikiwiki/pagespec]] "included() and !included()"
87 >          never being true. -->
88 >     The thing is called <TMPL_VAR name> and its size is <TMPL_VAR size>
89 >     <TMPL_IF FALSE>
90 >     """]]
91 >     </TMPL_IF>
92 >
93 > but that's far harder than it ought to be!
94 >
95 > Perhaps the right solution would be to change how the template plugin
96 > works, so that templates are expected to contain a new `definetemplate`
97 > directive:
98 >
99 >     This template is used to describe a thing. Parameters:
100 >     * name: the name of the thing
101 >     * size: the size of the thing
102 >     
103 >     \[[!definetemplate """
104 >     The thing is called <TMPL_VAR name> and its size is <TMPL_VAR size>
105 >     """]]
106 >
107 > with templates not containing a `\[[!definetemplate]]` being treated
108 > as if the whole text of the page was copied into a `\[[!definetemplate]]`,
109 > for backwards compatibility?
110 >
111 > --[[smcv]]
112
113 >> OK, here is a branch implementing what I said. It adds the `definetemplate`
114 >> directive to [[plugins/goodstuff]] as its last commit.
115 >>
116 >> Templates with the current strange semantics will still work, until
117 >> IkiWiki breaks compatibility.
118 >>
119 >> Possible controversies:
120 >>
121 >> * Should the `definetemplate` plugin be core, or in goodstuff, or neither?
122 >>
123 >> * Should \[[!definetemplate]] be allowed on any page (with the implementation
124 >>   of `template("foo")` looking for a `definetemplate` in `templates/foo`,
125 >>   then a `definetemplate` in `foo`, then fall back to the current logic)?
126 >>   If not, should \[[!definetemplate]] raise an error when used on a page not
127 >>   in `templates/`, since it will have no practical effect there?
128 >>
129 >> * Is it OK to rely on `definetemplate` being enabled in the basewiki's
130 >>   templates?
131 >>
132 >> * Should the "use definetemplate" wording in the documentation of
133 >>   template and edittemplate be stronger? Should those plugins automatically
134 >>   load definetemplate?
135 >>
136 >> --[[smcv]]
137
138 >>> this looks like a good idea to me.
139 >>>
140 >>> * i'd put it in core, and add a transition for the time compatibility gets
141 >>>   broken, provided the transitioning system will be used in that. templates
142 >>>   can't be expected to just work as markdown+ikiwiki too.
143 >>>
144 >>>   (it being in core would also solve my qualms about `section => "web"` /
145 >>>   `\[[!tag type/web]]`).
146 >>>
147 >>> * if definetemplate gets deemed core, no "use definetemplate!" notes on the
148 >>>   template/edittemplate pages will be required any more.
149 >>>
150 >>> * first i was sceptical of the approach of re-running scan to make sure the
151 >>>   `my %templates` is filled, but it is indeed a practical solution.
152 >>>
153 >>> * the name "`definetemplate`" gives me the first impression that something
154 >>>   is assigned (as in `#define`), but actually it highlights a region in the
155 >>>   file. wouldn't "`templatebody`" be a better description of the meaning of
156 >>>   the directive?
157 >>>
158 >>> --[[chrysn]]
159
160 >>>> Thanks for your feedback!
161 >>>> Looking at its description on this wiki, I agree that `type/web` doesn't
162 >>>> fit, and core does seem better. I like your `templatebody` suggestion,
163 >>>> too, particularly if templates remain restricted to `/templates`.
164 >>>> I'll try to come up with better wording for the documentation to say
165 >>>> "use `templatebody`, like this", with a note about backwards
166 >>>> compatibility later.
167 >>>>
168 >>>> Rationale for `my %templates`: yes it does seem a bit odd, but
169 >>>> if I used `$pagestate{$tpage}{template}` instead of a `my` variable,
170 >>>> I'd sometimes _still_ have to force a `scan`, because
171 >>>> [[plugins/template]] has to expand the template at scan time so that
172 >>>> it can contain links etc. - so I have to make sure that if the
173 >>>> template has changed, it has already been scanned (scanning happens
174 >>>> in random order, so that can't be guaranteed). This means there's
175 >>>> no benefit in reading it back from the index, so it might as well
176 >>>> just be in-memory.
177 >>>>
178 >>>> I suppose an alternative way to do it would be to remember what was
179 >>>> passed to `needsbuild`, and only force a `scan` for templates that
180 >>>> were in that list - which potentially reduces CPU time and I/O a
181 >>>> little, in exchange for a bigger index. I could do that if Joey
182 >>>> wants me to, but I think the current approach is simpler,
183 >>>> so I'll stick with the current approach if it isn't vetoed.
184 >>>> --[[smcv]]
185
186 >>>>> @name: even outside `/templates`, `\[[!templatebody]]` would be
187 >>>>> interpreted as "when this page is used as a template, this is what its
188 >>>>> contents should be", and be suitable.
189 >>>>>
190 >>>>> @`%templates`: my surprise wasn't to it not being in `%pagestate`, but
191 >>>>> rather that the `scan` function was used for it at all, rather than plain
192 >>>>> directive parsing that ignores everything else -- but i agree that it's
193 >>>>> the right thing to do in this situation.
194 >>>>>
195 >>>>> --[[chrysn]]
196
197 ----
198
199 [[!template id=gitbranch author="[[smcv]]" branch=smcv/ready/templatebody
200   browse=http://git.pseudorandom.co.uk/smcv/ikiwiki.git/shortlog/refs/heads/ready/templatebody]]
201 [[!tag patch users/smcv/ready]]
202 Branch and directive renamed to `ready/templatebody` as chrysn suggested.
203 It's on-by-default now (or will be if that branch is merged).
204 Joey, any chance you could review this?
205
206 There is one known buglet: `template_syntax.t` asserts that the entire
207 file is a valid HTML::Template, whereas it would ideally be doing the
208 same logic as IkiWiki itself. I don't think that's serious. --[[smcv]]
209
210 > Looking over this, I notice it adds a hash containing all scanned
211 > files. This seems to me to be potentially a scalability problem on
212 > rebuild of a site with many pages. Ikiwiki already keeps a lot
213 > of info in memory, and this adds to it, for what is a fairly
214 > minor reason. It seems to me there should be a way to avoid this. --[[Joey]] 
215
216 >> Maybe. Are plugins expected to cope with scanning the same
217 >> page more than once? If so, it's just a tradeoff between
218 >> "spend more time scanning the template repeatedly" and
219 >> "spend more memory on avoiding it", and it would be OK to
220 >> omit that, or reduce it to a set of scanned *templates*
221 >> (in practice that would mean scanning each template twice
222 >> in a rebuild). --s
223 >>> [Commit f7303db5](http://source.ikiwiki.branchable.com/?p=source.git;a=commitdiff;h=f7303db5)
224 >>> suggests that scanning the same page more than once is problematic,
225 >>> so that solution is probably not going to work.
226 >>>
227 >>> The best idea I've come up with so far is to track whether
228 >>> we're in the scan or render phase. If we're in the scan
229 >>> phase, I think we do need to keep track of which pages
230 >>> we've scanned, so we don't do them again? (Or perhaps that's
231 >>> unnecessary - commit f7303db5 removed a scan call that's in
232 >>> the render phase.) If we're in the render phase, we can assume
233 >>> that all changed pages have been scanned already, so we can
234 >>> drop the contents of `%scanned` and rely on a single boolean
235 >>> flag instead.
236 >>>
237 >>> `%scanned` is likely to be no larger than `%rendered`, which
238 >>> we already track, and whose useful lifetime does not overlap
239 >>> with `%scanned` now. I was tempted to merge them both and call
240 >>> the result `%done_in_this_phase`, but that would lead to really
241 >>> confusing situations if a bug led to `render` being called sooner
242 >>> than it ought to be.
243 >>>
244 >>> My ulterior motive here is that I would like to formalize
245 >>> the existence of different phases of wiki processing - at the
246 >>> moment there are at least two phases, namely "it's too soon to
247 >>> match pagespecs reliably" and "everything has been scanned,
248 >>> you may use pagespecs now", but those phases don't have names,
249 >>> so [[plugins/write]] doesn't describe them.
250 >>>
251 >>> I'm also considering adding warnings
252 >>> if people try to match a pagespec before scanning has finished,
253 >>> which can't possibly guarantee the right result, as discussed in
254 >>> [[conditional_preprocess_during_scan]]. My `wip-too-soon` branch
255 >>> is a start towards that; the docwiki builds successfully, but
256 >>> the tests that use IkiWiki internals also need updating to
257 >>> set `$phase = PHASE_RENDER` before they start preprocessing. --s
258
259 >>>> reviewing those modifications, i think this is a good way to go. along
260 >>>> with warning about pagespecs evaluated in scan phase, i think it should be
261 >>>> an error to invoke scan in the render phase; that would mean that
262 >>>> `readtemplate` needs to check whether it's invoked as a scan or not to
263 >>>> decide whether to scan the template page, but would be generally more
264 >>>> robust for future plugin writing.
265 >>>>
266 >>>> **addendum**: if the new phase state is used to create warnings/errors
267 >>>> about improper ikiwiki api use of plugins (which is something i'd
268 >>>> advocate), that should likewise warn if `add_link` actually adds a link in
269 >>>> the render phase.  such a warning would have helped spotting the
270 >>>> link-related [[template evaluation oddities]] earlier. --[[chrysn]]