confirm_form() broken when destination URL contains query string

I tried to find and only was able to find the following issue, where it seems the involved code was reworked during D5: #84250: Allow usage of query string on confirm forms

There's something that confirm_form doesn't cover well, yet.

I noticed this with a Delete node link in Views. Tried to debug the issue and found that the problem seems to live in confirm_form().

Explanation:

In Views, it's easy to see query strings in the view URL. If you have one of these, Node Delete links have a destination argument that contains all the arguments in the current URL of the view. It's a path and query string.

When you click on one of those Delete links, you see a confirmation form. This confirmation form uses the destination argument of set, to build the returning path of the form. But in this case, destionation contains a path and a query string! The confirm_form() function is invoked like this:

  return confirm_form($form,
    t('Are you sure you want to delete %title?', array('%title' => $node->title)),
    isset($_GET['destination']) ? $_GET['destination'] : 'node/' . $node->nid,
    t('This action cannot be undone.'),
    t('Delete'),
    t('Cancel')
  );

Note that $_GET['destination'] is taken if set.

Then, in system.module, function confirm_form, we can see this:

  // Prepare cancel link
  $query = $fragment = NULL;
  if (is_array($path)) {
    $query = isset($path['query']) ? $path['query'] : NULL;
    $fragment = isset($path['fragment']) ? $path['fragment'] : NULL;
    $path = isset($path['path']) ? $path['path'] : NULL;
  }
  $cancel = l($no ? $no : t('Cancel'), $path, array('query' => $query, 'fragment' => $fragment));

Note that $path here would be $_GET['destination']. It's not an array, so is_array($path) is NOT true. Then, function l() is used with a $path that contains a path and a query string. But this does not work well in function url(), and it also gets drupal_urlencode'd, so we end up with a broken URL for the cancel link of confirm_form().

I fixed this on our D6 site, like the following, but I'm not sure if this is the best way to do it.

This is a mini-patch for function confirm_form() in modules/system/system.module.

   $query = $fragment = NULL;
   if (is_array($path)) {
     $query = isset($path['query']) ? $path['query'] : NULL;
     $fragment = isset($path['fragment']) ? $path['fragment'] : NULL;
     $path = isset($path['path']) ? $path['path'] : NULL;
   }
+  elseif (strpos($path, '?') !== FALSE) {
+    list($path, $query) = explode('?', $path);
+    if (strpos($query, '#') !== FALSE) {
+      list($query, $fragment) = explode('#', $query);
+    }
+  }
   $cancel = l($no ? $no : t('Cancel'), $path, array('query' => $query, 'fragment' => $fragment));

I haven't tested in D7, but the code looks the same as in D6, so I guess this issue affects both versions.

Please, consider backporting the resulting fix to D6.