The foswiki svn repository will become read-only on Friday 8/8. Developers should register for a http://github.com/ account for commit access to foswiki.

Item727: Comment plugin loses data

Priority: CurrentState: AppliesTo: Component: WaitingFor:
Urgent Closed Extension CommentPlugin  
I discovered that the comment plugin can lose posted comments.

Consider this case:
%COMMENT{type="above" cols="100" target="%INCLUDINGTOPIC%#LatestComment"}%
The specified anchor does not exist - in this case, it was corrupted by the WYSIWYG plugin. Comment.pm tries a s//, but doesn't notice that it fails.

In any case, an error setting up the page should not cause user data to be lost. I marked this report Urgent because it results in user data loss.

Here's a (somewhat ugly) patch (against TWiki, but foswiki code is identical):
--- lib/TWiki/Plugins/CommentPlugin/Comment.pm~ 2008-09-11 23:41:58.000000000 -0400
+++ lib/TWiki/Plugins/CommentPlugin/Comment.pm  2009-01-10 13:45:46.000000000 -0500
@@ -309,32 +309,35 @@
             $text .= "\n" unless $output =~ m/^\n/s;
             $text .= $output;
             $text .= "\n" unless $text =~ m/\n$/s;
         } else {
             if ( $location ) {
                 if ( $position eq 'BEFORE' ) {
-                    $text =~ s/(?<!location\=\")($location)/$output$1/m;
+                    $text .= $output unless( $text =~ s/(?<!location\=\")($location)/$output$1/m );
                 } else { # AFTER
-                    $text =~ s/(?<!location\=\")($location)/$1$output/m;
+                    $text .= $output unless( $text =~ s/(?<!location\=\")($location)/$1$output/m );
                 }
+               $text .= "\n" unless $text =~ m/\n$/s;
             } elsif ( $anchor ) {
                 # position relative to anchor
                 if ( $position eq 'BEFORE' ) {
-                    $text =~ s/^($anchor\s)/$output$1/m;
+                    $text .= $output unless( $text =~ s/^($anchor\s)/$output$1/m );
                 } else { # AFTER
-                    $text =~ s/^($anchor\s)/$1$output/m;
+                    $text .= $output unless( $text =~ s/^($anchor\s)/$1$output/m );
                 }
+               $text .= "\n" unless $text =~ m/\n$/s;
             } else {
                 # Position relative to index'th comment
                 my $idx = 0;
                 unless( $text =~ s((%COMMENT({.*?})?%.*\n))
                           (&_nth($1,\$idx,$position,$index,$output))eg ) {
                     # If there was a problem adding relative to the comment,
                     # add to the end of the topic
                     $text .= $output;
                 };
+               $text .= "\n" unless $text =~ m/\n$/s;
             }
         }
     }
     if (defined $remove) {
         # remove the index'th comment box

done - slightly differently - so that above and below work as though no anchor/location was specified - with unit test smile

-- SvenDowideit - 15 Jan 2009

Thanks. I followed the example of the "position after the i'th comment fails" code.

Perhaps these failures should create a warning (or debug) log entry? We're not doing what the user expected, and it could be quite frustrating for someone to debug.

-- TimotheLitt - 15 Jan 2009

need to fix the unit test

debug and error logs won't help users much (unless the admin notices in a timely way :/)

i guess this is another one of those things where we need a user error status bad - like broadcastmsg where IF parse failures, eval errors, superceeded syntax detection and the like would go .

hard :/ -- SvenDowideit - 16 Jan 2009

Agree it's a general problem. Maybe not too hard.

Here's a possible approach:
  • Reserve a topic in each web - say 'WebErrors'.
  • Errors are written to this topic in a standard format:
    • #ErrorNumber<seq>
    • ---++ date-timestamp: Web.Topic:Rev: Component: Error: 1-line-summary
    • any error detail lines (traceback, parameters, etc)
    • date-timestamp: Error-end
      • This is a delimiter so anything can be logged
    • Component is core, plugin name, etc; Error is severity
  • Standard error (possible warn, debug) routines that handle locking, sequence, format
    • Special cases:
      • always lock topic with error, then WebErrors to prevent deadlock
      • Errors on the WebErrors topic
      • Hard limit on topic size to prevent runaway errors/denial of service. Configurable - perhaps 10_000 lines to start?
    • Foswiki::userError("Summary", <<"Detail", {web=>, topic=>, rev=>, component=>}); hash params default to current topic, caller
  • Error routine inserts [[WebErrors#ErrorNumber<seq>][ %RED%Error in <Component>%ENDCOLOR% ]] in topic with problem
    • Just after macro that causes problem, or foot of page if we can't do any better or aren't sure about nested macro calls
    • Provides link to data while maximizing chance that page is still useful
    • Do not update topic if no CHANGE access for viewer, or same Component has an error marker on page at this point.
  • For bonus points, put a checkbox 'remove this error' on each heading under WebErrors & a 'submit' at bottom - makes it easy to remove when fixed
  • Cron script (like statistics) to prune 'old' errors
  • Note that an admin can subscribe to WebErrors with MailerContrib, or a maintainer/developer can put a SEARCH on his home page that looks for errors on a component or topic that he supports.
-- TimotheLitt - 16 Jan 2009

Correct me if I'm wrong, but this looks like it's closed. Timothe, great ideas on the error reporting, but shouldn't that be a feature request, rather than an addendum to a plugin bug?

-- CrawfordCurrie - 21 Jan 2009

Yes, I thought about that when I wrote it -- but it was incidental to this instance, so I replied where Sven's comment was made.

I'll open a separate feature request so it doesn't get lost.

-- TimotheLitt - 23 Jan 2009

ItemTemplate edit

Summary Comment plugin loses data
ReportedBy TimotheLitt
Codebase 1.0.0, 1.0.0 beta3, 1.0.0 beta2, 1.0.0 beta1, trunk
SVN Range Foswiki-1.0.0, Thu, 08 Jan 2009, build 1878
AppliesTo Extension
Component CommentPlugin
Priority Urgent
CurrentState Closed
WaitingFor
Checkins Foswikirev:1984 Foswikirev:1998
TargetRelease patch
ReleasedIn 1.0.1
Topic revision: r14 - 23 Feb 2009, KennethLavrsen
 
The copyright of the content on this website is held by the contributing authors, except where stated elsewhere. see CopyrightStatement. Creative Commons License