[drupal-devel] [bug] Tablesort and $_REQUEST array handling

moshe weitzman drupal-devel at drupal.org
Thu Jul 28 18:15:53 UTC 2005


Issue status update for 
http://drupal.org/node/5371
Post a follow up: 
http://drupal.org/project/comments/add/5371

 Project:      Drupal
 Version:      cvs
 Component:    base system
 Category:     bug reports
 Priority:     normal
 Assigned to:  mathias
 Reported by:  mathias
 Updated by:   moshe weitzman
 Status:       patch

thanks for updating this patch, matt. looks good to me. i did not test
it yet though.




moshe weitzman



Previous comments:
------------------------------------------------------------------------

Fri, 23 Jan 2004 16:07:37 +0000 : mathias

Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays.patch (652 bytes)

Tablesort tries its best to rebuild the URI from which it was called.
The problem is when  $_REQUEST holds arrays such as $edit. Tablesort
will process this as:


example.com?edit=Array


This can be problematic since many module conditions rely on whether or
not the $edit array is set.  My patch disallows tablesort to append
"array $_REQUEST variables" to it's new query string, since it serves
no purpose.




------------------------------------------------------------------------

Sat, 31 Jan 2004 11:00:20 +0000 : Dries

I'd like to hear Moshe's take on this as I'm not sure this is the proper
fix.  I'd think that the tablesort code knows exactly what fields it
should add to the URL.




------------------------------------------------------------------------

Sat, 31 Jan 2004 13:24:36 +0000 : moshe weitzman

I don't think this is the proper fix either. For example, 


- browse to the Drupal.org issue search page [1].
- press Search button
- On the results page, look at the links in the table header and the
pager. The query state gets passed along by tablesort. This patch would
break this behavior.


Changing Status to 'Active', since this patch shouldn't be applied
without modification.
[1] http://drupal.org/project/drupal/issues/search




------------------------------------------------------------------------

Sat, 31 Jan 2004 17:01:56 +0000 : mathias

I'm pretty sure the project system is hacking the request object just
like I am before sending it to tablesort:



<?php
if ($_SERVER['REQUEST_METHOD'] == 'POST' && is_array($_POST['edit'])) {
  foreach ($_POST['edit'] as $key => $value) {
    if (!empty($value) && in_array($key, $fields)) {
      $query->$key = $value;
      $_POST[$key] = is_array($value) ? implode(',', $value) :
$value;
    }
  }
  unset($_POST['edit'], $_POST['op']);
}
?>




Basically, we're both moving the edit array into separate name / value
pairs and then destroying it before handing it off to tablesort.  I
don't understand what the benefit is of passing an array into a GET
query string since the key/value pairs aren't retained. We could set an
option in tablesort to ask it to decompose the array into it's key/value
pairs for us and pass them back in the query string.  For example an
edit array such as:



<?php
$edit['a'] => 1
$edit['b'] => 2
would have a query string of:
&edit_a=1&edit_b=2
instead of the currently meaningless:
&edit=Array
?>






------------------------------------------------------------------------

Sat, 31 Jan 2004 17:12:57 +0000 : Goba

You mean:



<?php
$edit['a'] => 1
$edit['b'] => 2
would have a query string of:
&edit[a]=1&edit[b]=2
instead of the currently meaningless:
&edit=Array
?>




This reuses the wonderful array handling provided by PHP (as long as
nested arrays are handled properly).




------------------------------------------------------------------------

Sun, 01 Feb 2004 15:02:25 +0000 : Kjartan

Indeed project module hacks the request data. I don't really like this
as a general fix as it is a hack, but for now it is the best way. I
dont think the original patch will break this as it just ignors
arrays(). Project module loops through the array and converts them to
normal array string elements; $_POST['edit']['status'] = value becomes
$_POST['status'] = value, then I think I unset the edit array to clean
up the urls.




------------------------------------------------------------------------

Thu, 05 Feb 2004 13:42:16 +0000 : moshe weitzman

After feedback from Matias and Kjartan, I now consider this patch to be
desireable




------------------------------------------------------------------------

Thu, 12 Aug 2004 15:10:30 +0000 : killes at www.drop.org

Patch doesn't apply anymore.




------------------------------------------------------------------------

Fri, 08 Oct 2004 04:14:30 +0000 : mathias

Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_0.patch (2.55 KB)

This patch resolves the hacks project module and the ecommerce package
use to rebuild the $_REQUEST arrays when implementing paged resultsets
and/or tablesorting. I know some other developers have ran into this
bug as well usually after form submissions.


Tablesort will now fully handle multi-dimensional $_REQUEST arrays and
convert them to a valid querystring and preserve user-submitted data.




------------------------------------------------------------------------

Fri, 08 Oct 2004 04:34:12 +0000 : moshe weitzman

very nicely done ... bonus points for converting pager to use the new
array2uri() function ... tablesort's poor handling of arrays frustrated
me in a recent client project.


this will simplify almost all cases  where a form post leads to a
sortable "results" table, such as in project.module.




------------------------------------------------------------------------

Fri, 15 Oct 2004 14:01:42 +0000 : mathias

Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_1.patch (2.99 KB)

Moved the new array2uri function to live with its array2* counterparts
in common.inc and cleaned up the function documentation a wee bit.




------------------------------------------------------------------------

Tue, 21 Dec 2004 16:43:33 +0000 : moshe weitzman

this is a nice code consolidation IMO




------------------------------------------------------------------------

Wed, 22 Dec 2004 10:50:33 +0000 : Dries

This patch adds more code than it removes.


That aside, it would be nice if the function's PHPdoc comments were
extended.  Looking at the patch, it is easy to see how/when this
function is being used, but reading the PHPdoc comments it is not.  I'd
add a simple example and some context information.  Also, it is unclear 
what $current_key is used for without inspecting the function's code. 
For readability, maybe rename $array to $attributes?  If that makes
sense, maybe rename 'array2uri' to 'attributes2uri'?  Which makes me
wonder: how does this stack with the existing drupal_attributes()
function?


Do we really need the recursive array handling?




------------------------------------------------------------------------

Sun, 13 Mar 2005 20:25:06 +0000 : killes at www.drop.org

Doesn't apply anymore.




------------------------------------------------------------------------

Tue, 12 Jul 2005 22:49:07 +0000 : mathias

Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_2.patch (3.19 KB)

Updating this patch since I've been bitten by this issue again. The new
array2uri() function really shines for tablesorting and/or paging
search results after a $_POST request. The only coding change involved
for developers is to use $_REQUEST['edit'] and $_REQUEST['op'] instead
of $_POST['edit'] and $_POST['op'] within their function callback where
this functionality is desired. As an added benefit these pages could
also be bookmarked.


Dries, I've updated the documentation to hopefully makes things a
little easier to understand. I don't think renaming the $array variable
to $attributes or calling the function attributes2uri instead of
array2uri would make the function and its purpose more transparent. 
It's not used for HTML attributes like the drupal_attributes function,
but rather for form submitted data (e.g., the $edit array) that needs
to be tablesorted/paged.


Project module and the ecommerce package still currently hack the
$_POST vars to circumvent this problem. Not sure if other modules are
also doing this.




------------------------------------------------------------------------

Fri, 15 Jul 2005 16:25:46 +0000 : Steven

If we're going to have an array2uri() function, we might as well add
urlencode() calls in there... this would centralize a lot of existing
urlencode() calls, I think.




------------------------------------------------------------------------

Mon, 18 Jul 2005 13:51:56 +0000 : Dries

Not going to commit this yet. 


1. Let's wait for Mathias' feedback on Steven's comment.  


2. I suggest using query or query_string, not querystring.


3. It is not clear why this can't be part of drupal_attributes($array)?
 If there is an important difference, it would be nice to make that
clear in the PHPdoc comments.




------------------------------------------------------------------------

Fri, 22 Jul 2005 03:18:19 +0000 : mathias

Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_3.patch (3.23 KB)

Agreed with Steven that this is a logical place to urlencode values. 
The patch has been updated accordingly.


There was some IRC discussion as to whether this function should be
called drupal_uri, drupal2uri or just plain ol' array2uri.  I think
since this code doesn't make any Drupal specific function calls,
array2uri is the best name.


Which brings up a whole new issue for a whole new thread: Perhaps
drupal_attributes should be renamed array2attributes?




------------------------------------------------------------------------

Mon, 25 Jul 2005 08:50:00 +0000 : Steven

Dries: drupal_attributes is used for outputting attributes of an HTML
tag. This patch is about putting data into the query (GET) part of an
URL. They are completely different.


mathias: couldn't we use this function in drupal_get_destination() as
well ?




------------------------------------------------------------------------

Mon, 25 Jul 2005 08:56:05 +0000 : Steven

By the way, what exactly is the point of this check:
+        if (!strstr($query_string, $key_as_uri)) {
+          $query_string .= '&'. $key_as_uri. '='. urlencode($value);
+        }


It seems to be there to avoid duplicates, but there is no similar check
below. If it is required, it needs a comment to explain it.




------------------------------------------------------------------------

Tue, 26 Jul 2005 17:02:50 +0000 : mathias

Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_4.patch (3.39 KB)

drupal_get_destination() now uses array2uri(). I discovered that the use
of strstr(), was to compensate for the function calling itself at times
when it didn't need to. This bug is fixed and strstr() is removed. Good
catch!




------------------------------------------------------------------------

Tue, 26 Jul 2005 18:57:26 +0000 : mathias

Attachment: http://drupal.org/files/issues/tablesort_and_REQUEST_arrays_5.patch (3.58 KB)

Allow a string, NULL or array to be passed into array2uri(), making life
easier on developers per Steven's feedback on IRC.







More information about the drupal-devel mailing list