Incorrect call to urldecode()

Patch applies cleanly, and it makes sense, but I couldn't find a path to the scenario that executes that line of code is system.module
function confirm_form
in the case where
isset($_GET['destination')
evaluates to true, to get to the changed line.

confirm_form() fires whenever you do something like, say, delete a node or a URL alias or similar and it displays an "are you sure?" message.

Note that if you ever want to know where core uses a particular function, you can go to a page like http://api.drupal.org/api/drupal/modules%21system%21system.module/functi... and scroll down to the "XX calls to function_name()" part. This takes you to a page like http://api.drupal.org/api/drupal/modules%21system%21system.module/functi... that lets you see the function used in context.

Hope that helps!

Thanks for the link, that is helpful. The question here is how to get a manual test which covers the code change; tracing into system.confirm_form() is straight forward, but the condition guarding the code change is always false (for me).

Here's the whole statement:

  // Prepare cancel link.
  if (isset($_GET['destination'])) {
    $options = drupal_parse_url($_GET['destination']);
  }
  elseif (is_array($path)) {
    $options = $path;
  }
  else {
    $options = array('path' => $path);
  }

the changed line is
$options = drupal_parse_url($_GET['destination']);

the guard condition is
if (isset($_GET['destination']))

So far is I can tell,

grep -r "\$_GET\['destination'\] =" .

$_GET['destination'] will only be set if our path has taken us through a MENU_ACCESS_DENIED or MENU_NOT_FOUND (common.inc drupal_deliver_html_page), and one scenario in batch.inc.

If there is no way to get to the code, maybe it could be removed, rather than fixed to comply with the docs. In any case this is a small matter, maybe we should just clear this change as it is...

On the page http://d7.local/admin/content, click delete on a node. The URL will be something like http://d7.local/node/52/delete?destination=admin/content

Confirmed that use of urldecode() is indeed unnecessary. Needs backport to 7.

fixing tag

Committed/pushed to 8.x, moving to 7.x for backport.

This is the exact same change that went into D8.

10: unnecessary-urldecode-1569580-10.patch queued for re-testing.

10: unnecessary-urldecode-1569580-10.patch queued for re-testing.

This is not only unnecessary, it is totally incorrect. I don't understand why this was committed to 8.x without tests.

This fix went into Drupal 7.52 as a security fix.