Item9041: TWikiCompatibilityPlugin should fix links to system topics renamed in Foswiki

As someone pointed out on IRC, links to topics that were renamed as part of the Foswiki rebranding, such as ATasteOfTWiki, render as links to topics that do not exist. Additionally, if you click on one of those links, the browser opens the editor, and the editor shows the text of the equivalent page, in this case BeginnersStartHere.

However, links like %TWIKIWEB%.WelcomeGuest render as links to topics that exist. That is, it seems to work for topics that were not renamed as when Foswiki was started. Update: these are also broken on trunk.

Here are some examples (static rendering taken from foswiki.org and trunk.foswiki.org at about 5am UTC on 20 May 2010):

TML	Renders as	Static copy of 1.0.x rendering	Static copy of trunk rendering
TWiki.WelcomeGuest	WelcomeGuest	WelcomeGuest	WelcomeGuest?
TWiki.ATasteOfTWiki	ATasteOfTWiki	ATasteOfTWiki?	ATasteOfTWiki?

It is possible to fix this problem by adding a renderWikiWordHandler to the TWikiCompatibilityPlugin. For example, here is the diff of the change I made on trunk:

===================================================================
--- TWikiCompatibilityPlugin.pm   (revision 7466)
+++ TWikiCompatibilityPlugin.pm   (working copy)
 -66,11 +66,30 @@
 sub earlyInitPlugin {
 
     my $session = $Foswiki::Plugins::SESSION;
+
+    _patchWebTopic($session->{webName}, $session->{topicName});
+
+    #Map TWIKIWEB to SYSTEMWEB and MAINWEB to USERSWEB
+    #TODO: should we test for existance and other things?
+    Foswiki::Func::setPreferencesValue( 'TWIKIWEB', 'TWiki' );
+
+    # Load TWiki::Func and TWiki::Plugins, for badly written plugins
+    # which rely on them being there without using them first
+    use TWiki;
+    use TWiki::Func;
+    use TWiki::Plugins;
+
+    return;
+}
+
+sub _patchWebTopic {
+    # my ($web, $topic) = @_;
+    # don't uncomment, use $_[0] etc
     if (
-        ( $session->{webName} eq 'TWiki' )
+        ( $_[0] eq 'TWiki' )
         && (
             !Foswiki::Func::topicExists(
-                $session->{webName}, $session->{topicName}
+                $_[0], $_[1]
             )
         )
       )
 -78,40 +97,28 @@
         my $TWikiWebTopicNameConversion =
           $Foswiki::cfg{Plugins}{TWikiCompatibilityPlugin}
           {TWikiWebTopicNameConversion};
-        $session->{webName} = $Foswiki::cfg{SystemWebName};
+        $_[0] = $Foswiki::cfg{SystemWebName};
         if (
-            defined( $TWikiWebTopicNameConversion->{ $session->{topicName} } ) )
+            defined( $TWikiWebTopicNameConversion->{ $_[1] } ) )
         {
-            $session->{topicName} =
-              $TWikiWebTopicNameConversion->{ $session->{topicName} };
+            $_[1] =
+              $TWikiWebTopicNameConversion->{ $_[1] };
 
-            #print STDERR "converted to $session->{topicName}";
+            #print STDERR "converted to $_[1]";
         }
     }
     my $MainWebTopicNameConversion =
       $Foswiki::cfg{Plugins}{TWikiCompatibilityPlugin}
       {MainWebTopicNameConversion};
-    if (   ( $session->{webName} eq 'Main' )
-        && ( defined( $MainWebTopicNameConversion->{ $session->{topicName} } ) )
+    if (   ( $_[0] eq 'Main' )
+        && ( defined( $MainWebTopicNameConversion->{ $_[1] } ) )
       )
     {
-        $session->{topicName} =
-          $MainWebTopicNameConversion->{ $session->{topicName} };
+        $_[1] =
+          $MainWebTopicNameConversion->{ $_[1] };
 
-        #print STDERR "converted to $session->{topicName}";
+        #print STDERR "converted to $_[1]";
     }
-
-    #Map TWIKIWEB to SYSTEMWEB and MAINWEB to USERSWEB
-    #TODO: should we test for existance and other things?
-    Foswiki::Func::setPreferencesValue( 'TWIKIWEB', 'TWiki' );
-
-    # Load TWiki::Func and TWiki::Plugins, for badly written plugins
-    # which rely on them being there without using them first
-    use TWiki;
-    use TWiki::Func;
-    use TWiki::Plugins;
-
-    return;
 }
 
 sub augmentedTemplatePath {
 -188,4 +195,29 @@
     return $pubUrl . $web . $file;
 }
 
+=pod
+
+---++ renderWikiWordHandler($linkText, $hasExplicitLinkLabel, $web, $topic) -> $linkText
+   * =$linkText= - the text for the link i.e. for =[<nop>[Link][blah blah]]=
+     it's =blah blah=, for =BlahBlah= it's =BlahBlah=, and for [[Blah Blah]] it's =Blah Blah=.
+   * =$hasExplicitLinkLabel= - true if the link is of the form =[<nop>[Link][blah blah]]= (false if it's ==<nop>[Blah]] or =BlahBlah=)
+   * =$web=, =$topic= - specify the topic being rendered
+
+Called during rendering, this handler allows the plugin a chance to change
+the rendering of labels used for links.
+
+Return the new link text.
+
+*Since:* Foswiki::Plugins::VERSION 2.0
+
+=cut
+
+sub renderWikiWordHandler {
+    my( $linkText, $hasExplicitLinkLabel, $web, $topic ) = @_;
+    if ($web eq 'TWiki' or $web eq 'Main') {
+        _patchWebTopic($_[2], $_[3]);
+    }
+    return $linkText;
+}
+
 1;

This works on 1.0.x and on trunk.

I have attached a patched version that will work on 1.0.x. If you decide to use this file, then backup your existing file first - just in case .

• TWikiCompatibilityPlugin.pm: Modified foswiki/lib/Foswiki/Plugins/TWikiCompatibilityPlugin.pm for version 1.0.x

This has a slight performance impact. I measured the time to serve System.WebTopicList with ab, and the time increases by about 10ms on 600ms (using CGI). Is that a problem?

My concern:

• This might be abusing the renderWikiWordHandler interface in a way that limits future options for the render

-- MichaelTempest - 19 May 2010

huh? I guess this means that the hash mapping got broken somewhere along the lines? I thought this was one of the things that TWikiCompatibilityPlugin already did. (given that I wrote it :/ )

-- SvenDowideit - 19 May 2010

The hash mapping is fine. Links from outside the wiki are fine - the earlyInitPlugin handler fixes web and topic as needed. The problem is with links in topic content.

I have updated the description above to demonstrate the problem and show that trunk and 1.0.x now behave differently.

A question: TWiki.ATasteForTWiki should link to System.BeginnersStartHere, but should the link text change too? (The link text should obviously not change for something like [[TWiki.ATasteForTWiki][Some link text]].)

-- MichaelTempest - 20 May 2010

Changed to urgent.

-- MichaelTempest - 26 Jun 2010

I checked in the TWikiLinkTests which reproduce this problem. They are not enabled at present because they fail.

-- MichaelTempest - 27 Jun 2010

ah.

in 1.0, Foswiki::Store::_getHandler looked like:

# PRIVATE
# Get the handler for the current store implementation.
# $web, $topic and $attachment _must_ be untainted.
sub _getHandler {
    my ( $this, $web, $topic, $attachment ) = @_;

    my $handler = $this->{IMPL}->new( $this->{session}, $web, $topic, $attachment );

    my $map = $Foswiki::cfg{Plugins}{TWikiCompatibilityPlugin}{WebSearchPath};
    if ($Foswiki::cfg{Plugins}{TWikiCompatibilityPlugin}{Enabled}
        && defined ($topic)
        && defined ($map)
        && defined ($map->{$web})
        && !$handler->storedDataExists()
        ) {
        #try the other
        my $newhandler = $this->{IMPL}->new( $this->{session}, $map->{$web}, $topic, $attachment );
        if ($newhandler->storedDataExists()) {
            $handler = $newhandler;
        }
    }

    return $handler;
}

this method has ceased to exist.

Having thought about it overnight - yes, Michael, your patch to the plugin is the best way to do it. I originally wondered if we should keep the magic mapping in the core for non TCP reasons, but really, that's a new feature, much like the one that MichaelDaum mooted - and I'm that thrilled about this kind of magic confusing users.

please commit your change

-- SvenDowideit - 28 Jun 2010

Fixed in 1.1

-- MichaelTempest - 29 Jun 2010

Hmmm. The new tests fail in the nightly build, but they pass for me. Sven - I don't know why they are failing so I cannot fix it :/ - marking this for your attention

-- MichaelTempest - 01 Jul 2010

They were failing on my box too; untaint errors. Fixed.

-- CrawfordCurrie - 15 Jul 2010

Ok, fixed Item9062 (Rolled back distro:b5aabfb9fe6f) and extended the unit tests to test all compatibility patched topics. Michael, please close this if it works for the "nightly" build.

-- OlivierRaginel - 16 Jul 2010

I tried to figure it out, but I ran out of time. I have to give my attention to other things - so I am marking this as "confirmed" again. If it is still not resolved in a week or two, I may be able to look at it again.

-- MichaelTempest - 18 Jul 2010

Ok, my guess is that Sven's build machine had a modified LocalSite.cfg, re-defining the TCP hashes, therefore it was not renaming the ATasteOfTWiki into BeginnersStartHere, and the last test was failing.

As a good rule to unit testing is never to rely on the machine configuration, I changed the 3rd test (the one which tests named links) to loop also over the hashes, and now all tests pass. Therefore, I'm closing this bug.

-- OlivierRaginel - 22 Jul 2010

Thanks, Olivier

-- MichaelTempest - 23 Jul 2010

Commenting is disabled while not logged in