Template Path Has Bug in TWiki 4.0

TWiki 4.0 made changes to the template search path which seem both unintuitive and not warranted by the discussions about requirements.

In particular, while in general the algorithm is to first find a template that has the current skin applied, when looking for a template name web.topic, it will first find the template that does not have the skin applied. That just seems plain wrong.

Note: This discussion started of with the request to make the search path configurable in TemplatePathIsCounterintuitive. Implementation of the enhancement request based on the documented algorithms revealed the discrepancy discussed here. Some discussion is refactored from that topic.

History of Template Search Algorithm in TWiki

In the following, please read

• script refers to the script name, e.g view, edit
• skin refers to a skin name, e.g dragon, pattern. All skins are checked at each stage, in the order they appear in the skin path.
• Note that when a template is taken from topic, the first character of the script and skin names are capitalized.
• web refers to the current web

Athens and Beijing

1. templates/web/script.skin.tmpl for each skin on the skin path
2. templates/script.skin.tmpl for each skin on the skin path
3. templates/web/script.tmpl
4. templates/script.tmpl

Cairo

1. templates/web/script.skin.tmpl
2. templates/script.skin.tmpl
3. templates/web/script.tmpl
4. templates/script.tmpl
5. Attempt to parse the template name into webName.topicName, and set web = webName and script = topicName
6. If the parse attempt was successful
   1. The TWiki topic web.script
   2. The TWiki topic %TWIKIWEB%.script
7. If the parse attempt was successful
   1. The TWiki topic web.skinSkinscriptTemplate
   2. The TWiki topic %TWIKIWEB%.skinSkinscriptTemplate
   3. The TWiki topic web.scriptTemplate
   4. The TWiki topic %TWIKIWEB%.script%

Dakar (TWiki 4.0)

1. If template name ends in .tmpl, templates/script (note that the template name may include skin information).
2. templates/web/script.skin.tmpl for each skin on the skin path
3. templates/script.skin.tmpl for each skin on the skin path
4. templates/web/script.tmpl
5. templates/script.tmpl
6. Attempt to parse the template name into webName.topicName
7. If the parse attempt was successful, do not capitalize the name of the script or web, and only for this step set web = webName and script = topicName
   1. The TWiki topic web.script
8. The TWiki topic web.skinSkinscriptTemplate for each skin on the skin path
9. The TWiki topic %TWIKIWEB%.skinSkinscriptTemplate for each skin on the skin path
10. The TWiki topic web.scriptTemplate
11. The TWiki topic %TWIKIWEB%.script%

Recommendation

1. If template name ends in .tmpl, templates/script (note that the template name may include skin information).
2. templates/web/script.skin.tmpl for each skin on the skin path
3. templates/script.skin.tmpl for each skin on the skin path
4. templates/web/script.tmpl
5. templates/script.tmpl
6. Attempt to parse the template name into webName.topicName, and set web = webName and script = topicName
7. The TWiki topic web.skinSkinscriptTemplate for each skin on the skin path
8. The TWiki topic web.scriptTemplate
9. The TWiki topic %TWIKIWEB%.skinSkinscriptTemplate for each skin on the skin path
10. The TWiki topic %TWIKIWEB%.scriptTemplate

Notes

• I am unsure whether in the design of the above algorithms the impact of that topic read uses TWiki::normalizeWebTopicName to allow the name to contain the web prefix was ever considered.
• The step marked is new but appears never used in TWiki today. My suggestion is to delete it.
• This algorithm is a change in that it never prefers an unskinned template over a skinned template when a skin is applied (except for the first step which is an easily visible exception, even if preserved).

-- Contributors: ThomasWeigert - 23 Nov 2006

Discussion

Allow "." in template names

In templates/search.pattern.tmpl, there is a line

%TMPL:INCLUDE{"view.pattern"}%

From the spec it is not clear that this should actually work. It would be just as good to have just view here, as the pattern extension would be found anyway due to the skin. The specification is unclear here, but it seems to me to be a mistake in the template, not the code.

This use of a strange include parameter is in

backlinks.pattern.tmpl
changes.pattern.tmpl
editform.pattern.tmpl
edittext.pattern.tmpl
moveattachment.pattern.tmpl
renamewebbase.pattern.tmpl
search.pattern.tmpl
searchbookview.pattern.tmpl
searchformat.pattern.tmpl
viewnoleftbar.pattern.tmpl
viewnotopbar.pattern.tmpl

This is also used in Nat skin.

Does anybody have any idea on what the spec really wants here? Reading the spec in the Templates.pm file does not lead me to believe this usage is valid. In certain cases, such as the search template, it does not appear necessary.

There are no unit test cases that would have caught that.

-- ThomasWeigert - 18 Nov 2006

The way this is used in the current templates is that a skin, say, natedit, may call a template view.nat.tmpl, rather than getting view.tmpl, if view.natedit.tmpl is not present.

Note that this trick only works for templates ending in *.tmpl, as templates in user topics are examined for a web prefix and thus the dot notation will not be preserved. Consequentially, there will be no way of overriding such template in the user topics.

-- ThomasWeigert - 18 Nov 2006

the current template calling mechanism does provide a way to explicitly call a template no matter if it were found on the search path or not. That is: %TMPL:INCLUDE{"view.pattern"}% may refer to the view template in the upper part of the search path or not. So this include is not a SUPER call like in OO-languages. It is a hardcoded include of one specific template and just by accident it is the one wanted and needed.

I just wanted to mention that we need such a construction to do any skin customization at all.

-- MichaelDaum

Yes, I understand what you are using this for, but this usage only worked "by accident" and is not warranted by the spec. The documented way of specifically calling a template is via %TMPL:INCLUDE{"view.pattern.tmpl"}%, that is, by having the ".tmpl" explicitly in the name. Is that the usage you had in mind? Note the difference: Using %TMPL:INCLUDE{"view.pattern"}% worked "by accident", while %TMPL:INCLUDE{"view.pattern.tmpl"}% would guarantee which template you get.

Which of these constructions did you think we need?

-- ThomasWeigert

Oh, right. If %TMPL:INCLUDE{view.nat.tmpl}% will do the same as w/o the tmpl ending then I can easily fix that in NatSkin. What ever makes things more spec-t-able...

-- MichaelDaum

My concern about the %TMPL:INCLUDE{"view.pattern"}% is that it does not work for topic templates, so it cannot be overridden. But I guess neither can %TMPL:INCLUDE{"view.pattern.tmpl"}%. So there goes that argument.

Note that there is one difference:

• %TMPL:INCLUDE{"view.pattern.tmpl"}% is immediately selected without any

further search

• %TMPL:INCLUDE{"view.pattern"}% is subject to the search and there might

be some interactions. E.g., if "view.pattern.tmpl" is not found, but "View" is a web and there is a topic "View.PatternTemplate" the latter would be chosen, but not in the case (i).

"Web.Topic" overrides other paths

The recent changes in TWiki::Templates::_readTemplateFile() broke all of my TWikiApplications that make use of VIEW_TEMPLATE and EDIT_TEMPLATE where the value is a web.topic. This is because $web.$name isn't part of the template search path anymore. The current implementation only prunes the search if the template name is ending with .tmpl. Along the same lines a $web.$topic template name should not need any search at all.

Reopening Bugs:Item2907

-- MichaelDaum - 22 Nov 2006

Michael, when you are using, say,

% META:PREFERENCE{name="VIEW_TEMPLATE" title="VIEW_TEMPLATE" type="Set" value="Sandbox.Test"}%

are you using the topic Sandbox.TestTemplate or Sandbox.Test.

I believe the "spec" is that you must use the former, which works just fine. I guess one can interpret "parse $name into a web name (default to $web) and a topic name and looks for this topic" as either meaning:

• look literally for $web.$name, or
• in the following search, use $web and $name rather than what was given originally.

I interpreted this to mean the latter, but I guess you are interpreting this to mean the latter?

I had considered this to be a bug in the previous code...

-- ThomasWeigert - 22 Nov 2006

If I use (a)

* Set VIEW_TEMPLATE = Sandbox.TestTemplate

or (b)

* Set VIEW_TEMPLATE = Sandbox.Test

then I mean it so as in both cases the template name is explicit.

Appending the Template string to the end is just there for convenience and serves no real purpose. It is only added to those variations that are not explicit. Previously, there have been two ways to explicitly specify which template to use, either by using a filename ending with .tmpl or let it be a full web.topic name. I think it is quite important to bypass all this template search fanciness and just say which view-template to use no matter what the search path is.

-- MichaelDaum - 22 Nov 2006

Actually, there was much discussion about the syntax of the templates in topics when that was implemented, which debate about whether "Template" should be added to the name, etc. I believe in the end "Template" was added to ensure that these would be wiki names.

Anyway, I now implemented a direct lookup for $web.$topic to support Michael's use case, but am not sure whether this is right.

We seem to be defining the spec once again based on a previous rewrite by CC of a an earlier algorithm written by an unknown author.

My interpretation of the actual spec is as stated at the top of this topic, which would not support that feature, but that probably does not really matter.

Anyway, this leaves the following questions, based on Michael's comments:

• if $web.$topic is interpreted as a direct lookup of $web.$topic without any further searches, if this topic is present, this would return null it (MD - 23 Nov 2006) , even if another match could be found at the search path later, similar to when we directly look up a topic ending in *.tmpl.
• does this get called in the middle of the search, or at the beginning, similar to *.tmpl to avoid any search?

In other words, say you are looking for sandbox.test with skin pattern, and the following are on the path:

• PatternSkinSandboxTestTemplate
• sandbox.test.pattern.tmpl
• sandbox.test.tmpl

but Sandbox.Test is not. Would you find these? Which? And if Sandbox.Test is present, which do you think should be found?

-- ThomasWeigert - 22 Nov 2006

Here is a test case I cooked up to verify the Michael Daum $web.$topic first interpretation. Given the documented algorithm above, it sure is counter-intuitive that Web.Test is found before Web.PatternSkinTestTemplate. Also, if it is allowed to drop the "Template" at the end of Web.Test, why is it not allowed to drop it from Web.PatternSkinTest?

As you can see, these are not issues due to this feature, but these are issues due to the strange implementation of the search path in TWiki 4.0.

sub test_WebDotTopicQuestions {
    my $this = shift;
    my $data;

     write_topic( 'Web', 'TestTemplate', 'the Web.TestTemplate template' );
    $data = $tmpls->readTemplate('web.test', 'skin', '' );
    $this->assert_str_equals('the Web.TestTemplate template', $data );

    write_topic( 'Web', 'SkinSkinTestTemplate', 'the Web.SkinSkinTestTemplate template' );
    $data = $tmpls->readTemplate('web.test', 'skin', '' );
    $this->assert_str_equals('the Web.SkinSkinTestTemplate template', $data );

    write_topic( 'Web', 'Test', 'the Web.Test template' );
    $data = $tmpls->readTemplate('web.test', 'pattern', '' );
    $this->assert_str_equals('the Web.Test template', $data );
}

Note that this was not possible in Cairo, see the specification quoted by Raffael further up in the document.

My diagnosis is that there is a bug in the TWiki 4.0 implementation which was discovered through reimplementation of this feature. Sadly it turns out that at least Michael has exploited this bug in his templates, albeit I have not seen an example.

Are we now forced to make this bug a part of the specification?

-- ThomasWeigert - 23 Nov 2006

About your test cases: there are test cases missing where Web.Test, Web.TestTemplate, Web.PatternTest, Web.PatternTestTemplate exist and either of them is called. Something like

    write_topic( 'Web', 'Test', 'the Web.Test template' );
    write_topic( 'Web', 'TestTemplate', 'the Web.TestTemplate template' );
    write_topic( 'Web', 'PatternSkinTest', 'the Web.PatternSkinTest template' );
    write_topic( 'Web', 'PatternSkinTestTemplate', 'the Web.PatternSkinTestTemplate template' );

    $data = $tmpls->readTemplate('Web.Test', 'Pattern', '' );
    $this->assert_str_equals('the Web.Test template', $data );
    # SMELL: or   $this->assert_str_equals('the Web.PatternSkinTest template', $data ); ???

    $data = $tmpls->readTemplate('Web.TestTemplate', 'Pattern', '' );
    $this->assert_str_equals('the Web.TestTemplate template', $data );
    # SMELL: or   $this->assert_str_equals('the Web.PatternSkinTestTemplate template', $data ); ???

    $data = $tmpls->readTemplate('Web.PatternSkinTest');
    $this->assert_str_equals('the Web.PatternSkinTest template', $data );

    $data = $tmpls->readTemplate('Web.PatternSkinTestTemplate');
    $this->assert_str_equals('the Web.PatternSkinTestTemplate template', $data );

-- MichaelDaum - 23 Nov 2006

I did not include those as they were already in the existing set of test cases.

I also agree with your "SMELL" comment. But that was not in TWiki 4.0 code. Personally, I think it is a bug.

Well, I also think that it is a bug to prefer Web.Test, but that is another sob story....

-- ThomasWeigert - 23 Nov 2006

We have a release blocking Bugs:Item3181 hanging on this one.

Is there a conclusion? Is there a bug and is there any code that needs to get checked in now to fix it? Or will we rather wait till after 4.1?

-- KennethLavrsen - 07 Dec 2006

I am fine with not documenting the strange behavior additions and leaving the code as is, but discussing this later. I anticipate that people will leverage the ability to modify the search path...

-- ThomasWeigert - 07 Dec 2006