You are here: Foswiki>Tasks Web>Item13619 (30 Aug 2015, GeorgeClark)Edit Attach

Item13619: RevCommentPlugin not compatible with Foswik 2.0

pencil
Priority: Normal
Current State: Closed
Released In: n/a
Target Release: n/a
Applies To: Extension
Component: NatEditPlugin, RevCommentPlugin
Branches: master
Reported By: KennethLavrsen
Waiting For:
Last Change By: GeorgeClark
PatternSkin is made to work well with RevCommentPlugin

When you have NatEditPlugin shipped with Foswiki by default it should also work with it (support somewhere the feature to add the one line comment).

RevCommentPlugin is made to work with HistoryPlugin and CompareRevisionsAddOn. These 3 plugins were made to work together. It is a bit sad if the RevCommentPlugin no longer works without disabling NatEditPlugin

-- KennethLavrsen - 13 Aug 2015

I fully agree with Kenneth. NatEditPlugin should play together with RevCommentPlugin nicely.

-- FranzJosefGigler - 13 Aug 2015

Someone said that someone here has deprecated RevCommentPlugin.

Well. You cannot deprecate people CONTENT. We have used this plugin for 10 years and topics are full of these comments. You cannot just throw the information people have keyed in. This is an essential plugin. I have many comments in quality management systems, requirements apps where people add comments like "This is the agreed baseline" or "This is the official version".

-- KennethLavrsen - 13 Aug 2015

I have seen Item3982 and I am not happy.

-- KennethLavrsen - 13 Aug 2015

Item3982 is specific for NatSkin, not NatEditPlugin. NatEditPlugin is now a default extension, and it really should continue to work with other PatternSkin compatible extensions. I have not used RevCommentPlugin, but there is a suggested fix in Item3982. If that works, we could add a skin template into RevCommentPlugin to apply the fix?

-- GeorgeClark - 14 Aug 2015

The suggested template fix works, but there are more issues. It appears that META parsing is much stricter in Foswiki 2.0. The registerMeta call registers:
Foswiki::Meta::registerMETA( 'REVCOMMENT',
            allow => [qw(comment t minor rev ncomments)] );
However the actual comment meta is added:

Meta ignores the data because of the _1 suffix used on comment, minor, rev and t. It appears that this is an API change to the META module, or maybe RevCommentPlugin was depending on a bug in the older META implementation that was "tightened up" in the new META serializer implementation. Possible fixes?
  • Change the meta serializer to work like 1.1 - might negatively impact mapping of meta data into database structures
  • Change the RevCommentPlugin to use the multiple= feature - supports multiple instances of a single META type, instead of indexing the variable names. This would need a migration tool
  • change RevCommentPlugin to eliminate the allow=[] definition from the registerMeta call. The serializer does no enforcing, and the meta works.

-- GeorgeClark - 14 Aug 2015

I've tested option 3, with addition of a revComment skin, and it seems to resolve the issue. These are probably the least disruption and isolates the changes into RevCommentPlugin, which is preferable to making core changes.

-- GeorgeClark - 14 Aug 2015 Patch:
diff --git a/lib/Foswiki/Plugins/RevCommentPlugin.pm b/lib/Foswiki/Plugins/RevCommentPlugin.pm
index 6ccc86c..9534d38 100755
--- a/lib/Foswiki/Plugins/RevCommentPlugin.pm
+++ b/lib/Foswiki/Plugins/RevCommentPlugin.pm
@@ -73,8 +73,7 @@ sub initPlugin {
 
     # Need to register meta, Item11249
     if ( defined &Foswiki::Meta::registerMETA ) {
-        Foswiki::Meta::registerMETA( 'REVCOMMENT',
-            allow => [qw(comment t minor rev ncomments)] );
+        Foswiki::Meta::registerMETA( 'REVCOMMENT');
     }
 
     # Plugin correctly initialized
And a usable template: Enable with *Set SKIN = revcomment,natedit,pattern
%TMPL:INCLUDE{"edittoolbar"}%
%{ ################################################################################ }%
%TMPL:DEF{"bottomtopicactions"}%<div class="natEditBottomBar">
%IF{ "context RevCommentPluginEnabled" then="%TMPL:P{"revcomment"}%"}%
<ul>%TMPL:P{"saveorrestorebutton"}%%TMPL:P{"checkpointbutton"}%%TMPL:P{"previewbutton"}%%TMPL:P{"formbutton"}%%TMPL:P{"changeform"}%%TMPL:P{"cancelbutton"}%%TMPL:P{"forcenewrevision"}%</ul>
%CLEAR%
</div>
%TMPL:END%
%TMPL:DEF{"revcomment"}%
<div class='revComment'>
<label for='comment'>Summary of changes</label>
<input class='foswikiInputField' type='text' style='width:50%' name='comment' id='comment' />
%POPUPWINDOW{"%SYSTEMWEB%.RevComment" label="%MAKETEXT{"help"}%"}%</div>%TMPL:END%

Changed the task to reflect that the bug is in RevCommentPlugin - in that it's not compatible with changes in Foswiki 2.0. We really should not be polluting core / default templates with RevComment code anyway. The proper way to add this type of interaction is by using template or skin overrides isolated to the RevCommentPlugin.

-- GeorgeClark - 14 Aug 2015

Kenneth, I've checked in a fix to RevCommentPlugin which appears to work on an unmodified Foswiki 2.0. Add revcomment to the skin path to test
  • Set #SKIN = revcomment,natedit,pattern

Once you've reviewed, I'll check in some core fixes to remove the unneeded revcomment implementation.

-- GeorgeClark - 14 Aug 2015

I have checked the feature and it works with the skin setting.

I also tried with just SKIN = pattern. Then the summary field is visible but the plugin ignores the submitted value. It is never stored. Also the help link does now work with pure pattern skin. The plugin should still work in a 1.1.9 in case people upgrade the plugin. Any reason why the help topic name changes or the field name changes?

-- KennethLavrsen - 21 Aug 2015

The CommentPlugin uses the comment parameter. The result is that when a comment is posted, it also ends up in the RevComment field. Issue reported on 1.1.x in Item11068, so this was a long standing conflict. I didn't see any way to fix that without changing the form field name, and that will break compatibility with the 1.1.9 pattern skin.

And update... the template overrides are not backwards compatible with 1.1.x. It looks like it will need a 1.1.x specific skin override as well.

-- GeorgeClark - 21 Aug 2015
 
Topic revision: r18 - 30 Aug 2015, GeorgeClark
The copyright of the content on this website is held by the contributing authors, except where stated elsewhere. See Copyright Statement. Creative Commons License    Legal Imprint    Privacy Policy