When a handler alters the querystring for a field, render_as_link converts it to a (rawurlescaped) string using drupal_http_build_query, does token replacement, and converts it back to an array using drupal_get_query_array. This is problematic for links in views where there is an exposed filter that utilizes the querystring, as well as the destination value... specifically looking at OG's implementation in og/includes/views/handlers/og_field_og_membership_link_edit.inc, where there is a filter on screen that produces url's like so:

http://mysite.com/group/node/1/admin/people?state=1&uid=demo1

The edit link rendered by this handler has a 'destination' option, which allows the user to redirect back to the page they were on, with the filter values that were there previously - this is accomplished by building the appropriate path and query in the handler and adding them the the $alter array.

When the alteration takes place, though, drupal_http_build_query escapes the string, resulting in something like this:

destination%3Dgroup%2Fnode%2F1%2Fadmin%2Fpeople%3Fstate%3D1%26uid%3Ddemo1

In the token set, there are regularly tokens for %1 and %2 (in the case of the og admin view, %2 is the nid), resulting in portions of the urlencoded string being incorrectly replaced with token values from the view, then the query being rebuilt incorrectly, breaking the destination functionality. Offending code is here:

views/handlers/views_handler_field.inc, lines 1340...1346

    if (isset($alter['query'])) {
      // Convert the query to a string, perform token replacement, and then
      // convert back to an array form for l().
      $options['query'] = drupal_http_build_query($alter['query']);
      $options['query'] = strtr($options['query'], $tokens);
      $options['query'] = drupal_get_query_array($options['query']);
    }

This can be fixed by recursively token-replacing the keys/values in the altered query array, rather than encoding/modifying/decoding the value as a string. Patch forthcoming.

Comments

marktfrey’s picture

marktfrey’s picture

Status: Patch (to be ported) » Needs review
rooby’s picture

This is not limited to query strings.
It also happens in the path.

For example you have a path with spaces, those spaces get changed to %20, then the tokens run and %2 gets replaced by some token and instead of a space you have some token and then a zero.

rooby’s picture

Here's a quick attempt at a new patch.
It affects the path, query, and fragment but I have done very little testing on different values.

What are peoples thoughts on this sort of approach?

Also, I haven't thought much on whether we might be breaking tokens by doing this.

For example, if someone had "This%20text" as a value but they actually intended %2 to be a token and the zero to be a zero, this will break that, however that seems like a far less likely scenario than the bug we're fixing.

heddn’s picture

Status: Needs review » Needs work

The above patch might work in certain cases, but it breaks the node edit and delete operation links. There's some double encoding going on.

heddn’s picture