Priority: Urgent
Current State: Closed
Released In: 1.0.1
Target Release: patch
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
--
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