You are here: Foswiki>Tasks Web>Item13385 (05 Jul 2015, GeorgeClark)Edit Attach

Item13385: Form is corrupted by escaped column separator

pencil
Priority: Low
Current State: Closed
Released In: 2.0.0
Target Release: major
Applies To: Engine
Component: FoswikiTableParser
Branches: master
Reported By: GeorgeClark
Waiting For:
Last Change By: GeorgeClark
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):

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
 
Topic revision: r7 - 05 Jul 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