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: Dries Status: patch 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. Dries Previous comments: ------------------------------------------------------------------------ 1074874057 : 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. ------------------------------------------------------------------------ 1075546820 : 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. ------------------------------------------------------------------------ 1075555476 : 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 ------------------------------------------------------------------------ 1075568516 : 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 ?> ------------------------------------------------------------------------ 1075569177 : 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). ------------------------------------------------------------------------ 1075647745 : 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. ------------------------------------------------------------------------ 1075988536 : moshe weitzman After feedback from Matias and Kjartan, I now consider this patch to be desireable ------------------------------------------------------------------------ 1092323430 : killes@www.drop.org Patch doesn't apply anymore. ------------------------------------------------------------------------ 1097208870 : 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. ------------------------------------------------------------------------ 1097210052 : 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. ------------------------------------------------------------------------ 1097848902 : 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. ------------------------------------------------------------------------ 1103647413 : moshe weitzman this is a nice code consolidation IMO ------------------------------------------------------------------------ 1103712633 : 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? ------------------------------------------------------------------------ 1110745506 : killes@www.drop.org Doesn't apply anymore. ------------------------------------------------------------------------ 1121208547 : 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. ------------------------------------------------------------------------ 1121444746 : 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.