Item13385: Form is corrupted by escaped column separator

EditingShorthand documents tables as "Use %VBAR% or | to add | characters in tables." It does not mention using \| to escape a column.

The following form causes a silent failure in the logs, when it attempts to find a handler for type 40, because the columns have all been shifted left due to the active escape. Note however the table renders correctly, ignoring the escape.

Name	Type	Size	Values	Description	Attributes	Default
Something \	text	40

The cause is in Foswiki::Table::Parser, commenting out the protection of \| fixes the Form processing.

@@ -184,7 +184,7 @@ sub parse {
 
                 if ( length($line) ) {
 
-                    $line =~ s/\\\|/\007/g;    # protect \| from split
+                    #$line =~ s/\\\|/\007/g;    # protect \| from split
                          # Expand comments again after we split
                     my @cols =
                       map { _rewrite( $_, \@comments ) }

-- GeorgeClark - 26 Apr 2015

Foswiki 1.1.9 has the same issue, with a completely different table parser. So definitely low priority.

-- GeorgeClark - 26 Apr 2015

It doesn't mention \| because I'm not sure it is consistently supported.

Does

\

Work?

should have three columns.

Right, I disabled the escape in core and the TablesContrib. That change needs a proposal.

-- Main.CrawfordCurrie - 20 Jun 2015 - 07:59

This breaks the FormDef unit tests which expects the escaped separator.

Module Failure summary

FormDefTests has 2 unexpected results (of 10):

• F: FormDefTests::test_searchForOptions
• F: FormDefTests::test_searchForOptionsQuery

The test embeds a regex %SEARCH macro inside a table. Without the ability to escape the | or condition, I'm not sure just killing this "feature" is the right solution. It's been in trunk since 2011. Item11040 and distro:848b64bc59c5 I tried other ways of encoding the vbar to get it into the regex, but I've not come up with a solution.

Although, also this seems to have been slipped in as part of the MongoDB work.

Hm This escape is also documented in DataForms

Macros in the initial values of a form definition get expanded when the form definition is loaded.

• If you want to use a | character in the initial values field, you have to precede it with a backslash, thus: \|.
• ...

And a "git blame" traces that 1st bullet back to the initial import back in 2009. Although reading that literally, the only place the \| escape should be active is in a DataForm initial values field.

-- GeorgeClark - 22 Jun 2015

I believe that the root cause is how the Foswiki::Form goes about parsing a table. In normal rendering, the %SEARCH would be expanded before the table is parsed in render, so there is no escaping necessary. But Foswiki::Form parses the table before any macro expansion has taken place.

I have a fix. Expand the % variables in Foswiki::Form before parsing the table. It requires some test changes because of the earlier macro expansion, but I don't believe that they are of consequence.

(Deleted this fix. not correct) ` -- GeorgeClark - 22 Jun 2015

Foswiki::Form 1.1.x used it's own table parse code that escaped \| Foswiki::Form 1.2.x has been changed to use the core table parser which does NOT escape \|. So there are 3 issues here

1. Foswiki 1.2 Form parser must have the \| escape for the values field.
2. For this particular bug, escapes should not be active in any other column
3. Table parsing should never use \| as an escape.

And my proposed solution above is not good, because early expansion of macros within form definitions will have performance implications.

-- GeorgeClark - 22 Jun 2015

I have a fix that doesn't change rendering order, and limits \| pipe escapes to just Forms.

diff --git a/core/lib/Foswiki/Tables/Parser.pm b/core/lib/Foswiki/Tables/Parser.pm
index d213369..2105cb7 100644
--- a/core/lib/Foswiki/Tables/Parser.pm
+++ b/core/lib/Foswiki/Tables/Parser.pm
@@ -100,6 +100,8 @@ a table (i.e. not verbatim or literal lines).
 sub parse {
     my ( $text, $dispatch ) = @_;
 
+    my $procform = ( (caller)[0] eq 'Foswiki::Form' );
+
     my $in_table = 0;
     my %scope = ( verbatim => 0, literal => 0, include => 0 );
     my $openRow;
@@ -186,29 +188,44 @@ sub parse {
 
                 if ( length($line) ) {
 
-                    # See Item13385 for why this is commented out
-                    #$line =~ s/\\\|/\007/g;    # protect \| from split
-
                     # Expand comments again after we split
                     my @cols =
                       map { _rewrite( $_, \@comments ) }
-                      map { s/\007/|/g; $_ }
                       split( /\|/, $line, -1 );
 
                     # Note use of LIMIT=-1 on the split so we don't lose
                     # empty columns
 
-                    foreach my $col (@cols) {
-                        my ( $prec, $text, $postc, $ish ) = split_cell($col);
-                        if ($ish) {
-                            print STDERR "TH '$prec', '$text', '$postc'\n"
+                    my $rowlen = scalar @cols;
+                    for ( my $i = 0 ; $i < $rowlen ; $i++ ) {
+                        if (   $procform
+                            && $i == 3
+                            && ( substr( $cols[$i], -1 ) eq '\\' )
+                            && $i < $rowlen )
+                        {
+# Form definitions allow use of \| escapes in the initial values colunn - column 4
+# So this code removes the "splits" from within the initial values
+# But only when processing a form.  See Item13385
+                            print STDERR "Merging Form values column.\n"
                               if TRACE;
-                            &$dispatch( 'th', $prec, $text, $postc );
+                            chop $cols[$i];
+                            $cols[$i] .= '|' . splice( @cols, $i + 1, 1 );
+                            $rowlen--;
+                            redo;
                         }
                         else {
-                            print STDERR "TD '$prec', '$text', '$postc'\n"
-                              if TRACE;
-                            &$dispatch( 'td', $prec, $text, $postc );
+                            my ( $prec, $text, $postc, $ish ) =
+                              split_cell( $cols[$i] );
+                            if ($ish) {
+                                print STDERR "TH '$prec', '$text', '$postc'\n"
+                                  if TRACE;
+                                &$dispatch( 'th', $prec, $text, $postc );
+                            }
+                            else {
+                                print STDERR "TD '$prec', '$text', '$postc'\n"
+                                  if TRACE;
+                                &$dispatch( 'td', $prec, $text, $postc );
+                            }
                         }
                     }
                 }

-- GeorgeClark - 22 Jun 2015

Crawford. Fix checked in. It passed the unit tests, please set this to Waiting for Release, if the fix looks okay. Thanks.

-- GeorgeClark - 22 Jun 2015

Commenting is disabled while not logged in