Item12888: added percent-encoding test to FormDefTests, and refactore…

…d duplicate code, removing call to inappropriate Foswiki::urlDecode that was breaking George's tests. The percent encoding is a bit of a mystery; when it was first coded, the subclasses of ListFieldDefinition all did an urlDecode on labels in parsed +values. This was undocumented, but presumably was intended to protect any characters that might interfere with the rendering of the form table, but were desireable in the label. Quite why URL encoding was chosen over the more obvious entity encoding is obscure, as is the reasoning behind applying the encoding to the label, but not the value. Only MichaelDaum knows. The percent-encoding remains undocumented, and therefore a potential gotcha for the unwary.
foswiki · May 22, 2015 · 28f3ce0 · 28f3ce0
1 parent 9613bf2
commit 28f3ce0
Show file tree

Hide file tree

Showing 8 changed files with 73 additions and 127 deletions.
diff --git a/JQueryPlugin/lib/Foswiki/Form/Rating.pm b/JQueryPlugin/lib/Foswiki/Form/Rating.pm
@@ -53,23 +53,28 @@ sub getOptions {
     return $options if $options;
     $options = $this->SUPER::getOptions();
 
-    if ( $this->{type} =~ m/\+values/ ) {
-        $this->{valueMap} = ();
-        $this->{_options} = ();
-        my $str;
-        foreach my $val (@$options) {
-            if ( $val =~ m/^(.*?[^\\])=(.*)$/ ) {
-                $str = $1;
-                $val = $2;
-                $str =~ s/\\=/=/g;
+    unless ( $this->{valueMap} ) {
+
+        # 1.2.0 does the value map for you in the superclass.
+        if ( $this->{type} =~ m/\+values/ ) {
+            $this->{valueMap} = ();
+            $this->{_options} = ();
+            my $str;
+            foreach my $val (@$options) {
+                if ( $val =~ m/^(.*?[^\\])=(.*)$/ ) {
+                    $str = $1;
+                    $val = $2;
+                    $str =~ s/\\=/=/g;
+                }
+                else {
+                    $str = $val;
+                }
+                $str =~ s/%([\da-f]{2})/chr(hex($1))/gei;
+                $this->{valueMap}{$val} = $str;
+                push @{ $this->{_options} }, $val;
             }
-            else {
-                $str = $val;
-            }
-            $this->{valueMap}{$val} = Foswiki::urlDecode($str);
-            push @{ $this->{_options} }, $val;
+            $options = $this->{_options};
         }
-        $options = $this->{_options};
     }
 
     return $options;

diff --git a/JSCalendarContrib/lib/Foswiki/Form/Date.pm b/JSCalendarContrib/lib/Foswiki/Form/Date.pm
@@ -53,9 +53,8 @@ sub renderForEdit {
         {
             onclick => "return showCalendar('id$this->{name}','$ifFormat')",
             src     => Foswiki::Func::getPubUrlPath(
-                web        => $Foswiki::cfg{SystemWebName},
-                topic      => 'JSCalendarContrib',
-                attachment => 'img.gif'
+                $Foswiki::cfg{SystemWebName}, 'JSCalendarContrib',
+                'img.gif'
             ),
             alt   => 'Calendar',
             class => 'foswikiButton foswikiEditFormCalendarButton'

diff --git a/UnitTestContrib/test/unit/FormDefTests.pm b/UnitTestContrib/test/unit/FormDefTests.pm
@@ -339,7 +339,7 @@ sub test_Item972_selectPlusValues {
       Foswiki::Func::readTopic( $this->{test_web}, 'TestForm' );
     $topicObject->text( <<'FORM');
 | *Name* | *Type*   | *Size* | *Value* | *Tooltip* | *Attributes* |
-| Select | select+values | 5 | , =0, One, Two=2, Three=III, Four | Various values |
+| Select | select+values | 5 | , =0, One, Two=2, Th%72ee=III, Four | Various values |
 FORM
     $topicObject->save();
     $topicObject->finish();

diff --git a/core/lib/Foswiki.pm b/core/lib/Foswiki.pm
@@ -2891,9 +2891,7 @@ sub entityEncode {
 ---++ StaticMethod entityDecode ( $encodedText ) -> $text
 
 Decodes all numeric entities (e.g. &amp;#123;). _Does not_ decode
-named entities such as &amp;amp; (use HTML::Entities for that, though
-you will have to convert to unicode when you call it, and back again
-afterwards)
+named entities such as &amp;amp; (use HTML::Entities for that)
 
 =cut
 

diff --git a/core/lib/Foswiki/Form/Checkbox.pm b/core/lib/Foswiki/Form/Checkbox.pm
@@ -33,38 +33,6 @@ sub finish {
     undef $this->{valueMap};
 }
 
-sub getOptions {
-    my $this = shift;
-
-    return $this->{_options} if $this->{_options};
-
-    my $vals = $this->SUPER::getOptions(@_);
-    if ( $this->isValueMapped() ) {
-
-        # create a values map
-
-        $this->{valueMap} = ();
-        $this->{_options} = ();
-        my $str;
-        foreach my $val (@$vals) {
-            if ( $val =~ m/^(.*?[^\\])=(.*)$/ ) {
-                $str = TAINT($1);
-                my $descr = $this->{_descriptions}{$val};
-                $val = $2;
-                $this->{_descriptions}{$val} = $descr;
-                $str =~ s/\\=/=/g;
-            }
-            else {
-                $str = $val;
-            }
-            $this->{valueMap}{$val} = Foswiki::urlDecode($str);
-            push @{ $this->{_options} }, $val;
-        }
-    }
-
-    return $vals;
-}
-
 =begin TML
 
 ---++ getDefaultValue() -> $value
@@ -82,8 +50,6 @@ sub getDefaultValue {
 # Checkbox store multiple values
 sub isMultiValued { return 1; }
 
-sub isValueMapped { return shift->{type} =~ m/\+values/; }
-
 sub getDisplayValue {
     my ( $this, $value ) = @_;
 

diff --git a/core/lib/Foswiki/Form/ListFieldDefinition.pm b/core/lib/Foswiki/Form/ListFieldDefinition.pm
@@ -42,6 +42,10 @@ sub finish {
     undef $this->{_descriptions};
 }
 
+sub isMultiValued { return ( shift->{type} =~ m/\+multi/ ); }
+
+sub isValueMapped { return ( shift->{type} =~ m/\+values/ ); }
+
 # PROTECTED - parse the {value} and extract a list of options.
 # Done lazily to avoid repeated topic reads.
 sub getOptions {
@@ -55,6 +59,7 @@ sub getOptions {
     my %descr = ();
 
     @vals = split( /,/, $this->{value} );
+
     if ( !scalar(@vals) ) {
         my $topic = $this->{definingTopic} || $this->{name};
         my $session = $this->{session};
@@ -96,9 +101,52 @@ sub getOptions {
     }
     @vals = map { $_ =~ s/^\s*(.*)\s*$/$1/; $_; } @vals;
 
-    $this->{_options}      = \@vals;
     $this->{_descriptions} = \%descr;
 
+    if ( $this->isValueMapped() ) {
+
+        # create a values map
+        $this->{valueMap} = ();
+        $this->{_options} = ();
+        my $str;
+        foreach my $val (@vals) {
+            if ( $val =~ m/^(.*[^\\])*=(.*)$/ ) {
+                $str = TAINT( $1 || '' );    # label
+                      # Copy the description to the real value
+                my $descr = $this->{_descriptions}{$val};
+                $val = $2;
+                $this->{_descriptions}{$val} = $descr;
+
+                # Unescape = - legacy! Entities should suffice
+                $str =~ s/\\=/=/g;
+            }
+            else {
+                # Label and value are the same
+                $str = $val;
+            }
+
+            # SMELL: when it was first coded, the subclasses of
+            # ListFieldDefinition all did an urlDecode on labels in
+            # parsed +values. This was undocumented, but presumably
+            # was intended to protect any characters that might
+            # interfere with the rendering of the form table,
+            # but were desireable in the label. Quite why
+            # URL encoding was chosen over the more obvious
+            # entity encoding is obscure, as is the reasoning behind
+            # applying the encoding to the label, but not the value.
+            # For compatibility we retain this decoding step here.
+            # It remains undocumented, and therefore a potential
+            # gotcha for the unwary.
+            $str =~ s/%([\da-f]{2})/chr(hex($1))/gei;
+
+            $this->{valueMap}{$val} = $str;
+            push @{ $this->{_options} }, $val;
+        }
+    }
+    else {
+        $this->{_options} = \@vals;
+    }
+
     return $this->{_options};
 }
 

diff --git a/core/lib/Foswiki/Form/Radio.pm b/core/lib/Foswiki/Form/Radio.pm
@@ -35,38 +35,6 @@ sub finish {
     undef $this->{valueMap};
 }
 
-sub getOptions {
-    my $this = shift;
-
-    return $this->{_options} if $this->{_options};
-    my $vals = $this->SUPER::getOptions(@_);
-
-    if ( $this->{type} =~ m/\+values/ ) {
-
-        # create a values map
-
-        $this->{valueMap} = ();
-        $this->{_options} = ();
-        my $str;
-        foreach my $val (@$vals) {
-            if ( $val =~ m/^(.*?[^\\])=(.*)$/ ) {
-                $str = TAINT($1);
-                my $descr = $this->{_descriptions}{$val};
-                $val = $2;
-                $this->{_descriptions}{$val} = $descr;
-                $str =~ s/\\=/=/g;
-            }
-            else {
-                $str = $val;
-            }
-            $this->{valueMap}{$val} = Foswiki::urlDecode($str);
-            push @{ $this->{_options} }, $val;
-        }
-    }
-
-    return $vals;
-}
-
 sub getDisplayValue {
     my ( $this, $value ) = @_;
 

diff --git a/core/lib/Foswiki/Form/Select.pm b/core/lib/Foswiki/Form/Select.pm
@@ -52,40 +52,6 @@ sub getDefaultValue {
     return ( exists( $this->{default} ) ? $this->{default} : '' );
 }
 
-sub getOptions {
-    my $this = shift;
-
-    my $vals = $this->{_options};
-    return $vals if $vals;
-
-    $vals = $this->SUPER::getOptions(@_);
-
-    if ( $this->isValueMapped() ) {
-
-        # create a values map
-
-        $this->{valueMap} = ();
-        $this->{_options} = ();
-        my $str;
-        foreach my $val (@$vals) {
-            if ( $val =~ m/^(.*[^\\])*=(.*)$/ ) {
-                $str = TAINT( $1 || '' );
-                my $descr = $this->{_descriptions}{$val};
-                $val = $2;
-                $this->{_descriptions}{$val} = $descr;
-                $str =~ s/\\=/=/g;
-            }
-            else {
-                $str = $val;
-            }
-            $this->{valueMap}{$val} = Foswiki::urlDecode($str);
-            push @{ $this->{_options} }, $val;
-        }
-    }
-
-    return $vals;
-}
-
 =begin TML
 
 ---++ ObjectMethod finish()
@@ -106,10 +72,6 @@ sub finish {
     return;
 }
 
-sub isMultiValued { return ( shift->{type} =~ m/\+multi/ ); }
-
-sub isValueMapped { return ( shift->{type} =~ m/\+values/ ); }
-
 sub getDisplayValue {
     my ( $this, $value ) = @_;