Item5880: TablePlugin may delete query parameters after reordering some table

TWiki:Plugins.TablePlugin can sort tables by column values. It turns header cell into links with parameters containing info about desired sort order. It should preserve other existing parameters, and it does, since parameters names doesn't end in up and doesn't have numeric values. The reason is that TablePlugin, when generate new links, delete current sort parameters and insert new ones. It performs this task with a greed regex instead of using delete method (available at bot CGI and TWiki::Request).

Example:

Fill the form bellow and submit.

Sortcolumn
Foo
Cup
Bar

Now, all submitted values are displayed. Click on some header cell on the table bellow.

H1	H2	H3
abc	234	xyz
def	056	qwe
987	foo	bar

If you filled Cup with some numeric value (or something that starts with a number), them the first three fields are empty after sort. Bar value is preserved.

This problem is very unlikely to occur, but it's easy to fix.

-- TWiki:Main/GilmarSantosJr - 04 Aug 2008

I suggest the following fix:

=== lib/TWiki/Plugins/TablePlugin/Core.pm
==================================================================
--- lib/TWiki/Plugins/TablePlugin/Core.pm       (revision 15801)
+++ lib/TWiki/Plugins/TablePlugin/Core.pm       (local)
@@ -1687,13 +1687,21 @@
         my $cgi = TWiki::Func::getCgiQuery();
         return unless $cgi;
 
-        # Extract and attach existing parameters
-        my $plist = $cgi->query_string();
-        $plist =~ s/\;/\&/go;
-        $plist =~ s/\&?sortcol.*up=[0-9]+\&?//go;
-        $plist .= '&' if $plist;
-        $url = $cgi->url . $cgi->path_info() . '?' . $plist;
-        $url =~ s/\&/\&/go;
+        # Copy existing values
+        my (@origSort, @origTable, @origUp);
+        @origSort  = $cgi->param('sortcol');
+        @origTable = $cgi->param('table');
+        @origUp    = $cgi->param('up');
+        $cgi->delete('sortcol', 'table', 'up');
+        $url = $cgi->url(-full => 1, -path => 1) . '?';
+        my $queryString = $cgi->query_string();
+        $url .= $queryString . ';' if $queryString;
+
+        # Restore parameters, so we don't interfere on the remaining execution
+        $cgi->param( -name => 'sortcol', -value => \@origSort );
+        $cgi->param( -name => 'table',   -value => \@origTable );
+        $cgi->param( -name => 'up',      -value => \@origUp );
+
         $sortColFromUrl =
           $cgi->param('sortcol');    # zero based: 0 is first column
         $requestedTable = $cgi->param('table');

Waiting for feedback from someone else to commit.

-- TWiki:Main.GilmarSantosJr - 04 Aug 2008

Will these proposed changes work on 4.2 and 4.1 of TWiki?

Marked urgent. Added (trunk) to title.

-- TWiki:Main.KennethLavrsen - 04 Aug 2008

The delete method is available on TWiki::Request with the same semantics of CGI::delete, so it should work. I'll test this change with TWikiRelease04x02 and report results.

-- TWiki:Main.GilmarSantosJr - 04 Aug 2008

I've just tested and the problem is present in TWikiRelease04x02. The fix applies as well. So, should I merge to TWikiRelease04x02 branch? (removed "trunk" from title, since the problem also existis in 4.2)

-- TWiki:Main.GilmarSantosJr - 05 Aug 2008

Yes please merge fixes also to TWikiRelease04x02 branch so we can have them in 4.2.2.

-- TWiki:Main.KennethLavrsen - 05 Aug 2008

Done

-- TWiki:Main.GilmarSantosJr - 05 Aug 2008

Revision 17326 breaks repeated sorting of columns because url parameters are copied. The url gets multiple instances of up=0. Reopening.

-- TWiki:Main.ArthurClemens - 30 Sep 2008

That revision indirectly caused the problem. The root was at TWiki::Response::delete, that didn't work if parameter value was false, like this case. I fixed it at Item6035.

I also updated unit tests, so they can work with 4.2: TWikiFnTestCase makes the request object (either TWiki::Request or CGI) available at $this->{request}. Take a look at Item5911.

-- TWiki:Main.GilmarSantosJr - 01 Oct 2008

You mean that a new bug item is needed? Because the bug is still there. I will do it for you: Item6038.

-- TWiki:Main.ArthurClemens - 01 Oct 2008

Works now indeed.

-- TWiki:Main.ArthurClemens - 01 Oct 2008