drupal_get_destination, pager and tablesort array handling

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.

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

- browse to the Drupal.org issue search page.
- 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.

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

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:

You mean:

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

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.

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

Patch doesn't apply anymore.

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.

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.

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

this is a nice code consolidation IMO

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?

Doesn't apply anymore.

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.

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.

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.

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?

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 ?

By the way, what exactly is the point of this check:

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.

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!

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

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

Err, passing strings to array2uri() is pretty non-sensical and not at all what I meant. I'll see if I can make a better patch later :P.

* Further code optimizations.
* Made drupal_get_destination() make better use of array2uri().

+1 for this patch

Having the built in ability to sort tabular data is incredibly useful.

Syncing with HEAD and setting to commit-worthy as I've been using it in production mode for awhile now.

just curious: How does this patch break (the -) or interfere with the AJAX table sorting patch?

The two patches don't conflict as far as I can see. This patch introduces a function to convert arrays (such as the $edit array) to expanded querystrings.

A byproduct of this function is you can now have tablesortable and paged resultsets after submitting a POST form. Project module and the ecommerce package are currently using hacks to circumvent this.

+1 Tested and functioning exactly as described.
+1 Because this is a life-saver - and couldn't have come at a better time. I just recently started wrestling wth ?edit=Array and couldn't believe that there wasn't better handling for that inside of the drupal framework.

CVS reviewers - is there anything holding up this patch? please say so so we can fix it.

Yes patch applies cleanly and I like how it works. Instead of the useless array() now I have some variables I can work with :-D Please commit this, my module is broken without it and it seems this problem has existed for way too long! I can see certainly how project.module could benefit right away...

This patch causes drupal_get_destination() to emit a leading ampersand, which ends up becoming '?&destination=' when used in a clean URL. Typically the leading separator is added by url() or something similar, so it should IMO not be part of array2uri()'s final result.

This would allow simplifying code too:

I also don't see the point of allowing non-arrays to be passed to array2uri(): it doesn't seem to be used anywhere in this patch anyhow.

Updating to HEAD, This is a combination of http://drupal.org/node/54305 and this issue.

Maybe there are other places where drupal_query_string_encode() could be used to simplify code.

Note: This also makes the pager work with tablesort without losing the sort/order.

we need to get this in. recent changes to drupal_get_destination() make it fail when there are arrays in $_GET like edit[og_groups][]=12.

other people are experiencing this problem too: http://drupal.org/node/54305

i will review the patch ASAP.

Method of testing: I created a PHP block which has a very minimalistic form, with #redirect FALSE. Before and after patch I still get http://localhost/head/?q=admin&edit=Array&op=v . I am looking into it.

I take over.

This is somewhat closer. theme_pager_link needed patching. I am by far not 100% sure of this patch.

This is a lot cleaner of the same.

Moshe? Matt?

moshe discovered that some debug code got into the patch.

I made a tiny change to chx' patch in drupal_get_destination() because the path needs urlencoding too. i tested table sort, pager, destinations using arrays on querystring. seems to work.

• Right now, $exclude only applies to top level items, and it is set to array() for nested calls. But, if we move the exclude check to after where $parent is added to $key, we can exclude any item e.g; by excluding edit[foo].
• I don't see any reason not to urlencode. The lack of urlencode in theme('pager_link') is IMO a bad omission. $parameters is mostly passed in from tablesort_pager() which comes from $_GET or $_POST. This means, non-urlencoded values.

Other than that, I like the patch htough.

Patch for the above two things.

I again got a link like http://localhost/head/?q=admin&edit%5Bt%5D=t&edit%5Bform_id%5D=t&op=v&pa... if clicked the next page has links like http://localhost/head/?q=admin&edit%5Bt%5D=t&edit%5Bform_id%5D=t&op=v&pa... :(

Urlencoding issues asside, ebert's patch was still bogus.

Before all these patches, tablesort_pager() would be used to properly fill in theme('pager')'s $parameters argument with the GET/POST data. Whoever called theme('pager') was expected to use tablesort_pager() to pass these arguments along and keep the page state (such as tablesorting) when moving around.

After his patch, pager_query() remembers the query URL on its own and retrieves it in theme('pager'), meaning all query arguments are already there. This means that anyone who uses tablesort_pager() will be passing in duplicate data.

Having pager remember its own URL is no problem. There is no reason why the pager should know about tablesort or receive special treatment. Tablesort and pager both care only about their own variables, there is no need for an artificial passthrough. The way it is in the latest patch, we have a redundant API sitting around.

I'm all for removing tablesort_pager(), but then a lot of contrib modules need to be updated. But this is a pretty mindless upgrade, as all tablesort_pager() calls in contrib are the 5th argument to theme('pager', ...) (that shows you how weird it is to have an API function for this, if it's only ever used in one way).

So the attached patch addresses this. Both pager and tablesort now construct their own query string, doing so from $_REQUEST without the $_COOKIE data (anything that needs to be kept, is kept). If you need it, you can still pass in $parameters to theme('pager'), but this should be very rare. Anyone who uses only tablesort_pager() can now drop that argument instead.

Sorry, that was not the final one.

Now I do not get ugly URLs...

I gave this a thorough exercising and all worked well. I did notice on peculiarity when passing array notation in the querystring. Here is how I reproduce it

- enable the switch user block of devel.module (and assure you have its permission). this is just useful because it appends a drupal_get_destination in member link.
- now browse to an url like node?edit[og_groups][]=66. i know that url is meaningless - it is just illustrative.
- notice the href in the switch user links now have a 0 key in an element: edit[og_groups][0]=66.

This peculiarity doesn't break anything that I could find so I say RTBC.

applied

Some clea
t.

Hm, what is this new patch about?

eberts: Please don't post random patches without explanation.

The movement of the ampersand in my earlier patch ensures better consistency between the pager and tablesort code and makes more sense. The ampersand only applies when you combine the query string with existing arguments. It is not an intrinsic part of it.

I'll apply the urlencode change.

I stupid managed to delete half of the text while submitting, sorry about it.

But still I don't see the need of the additional apersand if there is no query string appended anyway. It's just not tasty.

With today's cvs I get pager links such as "
http://localhost/alongtail05/?q=advice/content-movie&page=12&edit[quicksearch_title]=Lost+Highway+-+1997&edit[form_id]=content-movie_autocomplete&op=go&nid=100&vote=2"

I'm sorry, you may kick me if it's wrong issue. :)

It happens only when the form is submitted to a page with theme_pager ( $output .= theme('pager', NULL, $ipp, 0); )

Form looks like this:

This is by design. The proper way of dealing with this is to have a drupal_goto() to a clean URL after submitting the form (like search does sort of).

But, if I use drupal_goto(), I loose all $_POST variables, isn't it?