I've seen many confirm forms in my life.

I've yet to see a confirm form that does NOT implement this ugly workaround:

    $output = confirm_form($form,
      t('Are you sure you want to delete path alias %title?', array('%title' => $path['dst'])),
      isset($_GET['destination']) ? $_GET['destination'] : 'admin/config/search/path'
    );

(taken from path.admin.inc. Many more examples in core.)

Easy one:

1) Implement that logic directly into confirm_form().

2) Grep core for 'destination' (apparently not as many instances as you imagine) and replace those belonging to such ugly confirm_form() constructs.

CommentFileSizeAuthor
#18 drupal.confirm-form-destination.patch2.78 KBsun
#12 drupal.confirm-form.patch2.4 KBdawehner
#9 drupal.confirm-form-destination.patch2.43 KBdawehner
#7 drupal.confirm-form-destination.patch1.33 KBsun
#3 drupal_582456.patch2.67 KBtobiasb
#2 drupal-confirm_form-get.patch1.71 KBdawehner

Comments

mattyoung’s picture

mattyoung commented

sub

dawehner’s picture

dawehner
Primary language German
commented
Status: Active » Needs review
StatusFileSize
new drupal-confirm_form-get.patch1.71 KB 
$ grep "destination" . -rn | grep "GET"
./modules/path/path.admin.inc:189:      isset($_GET['destination']) ? $_GET['destination'] : 'admin/config/search/path');
./modules/node/node.pages.inc:464:    isset($_GET['destination']) ? $_GET['destination'] : 'node/' . $node->nid,

Here are the two instances, i filterd out by other results.

tobiasb’s picture

tobiasb
Primary language German
Location Berlin
commented
StatusFileSize
new drupal_582456.patch2.67 KB

my version

dawehner’s picture

dawehner
Primary language German
commented 
+++ modules/path/path.admin.inc	20 Sep 2009 10:21:49 -0000
@@ -185,8 +185,7 @@
+      t('Are you sure you want to delete path alias %title?', array('%title' => $path['dst'])), 'admin/config/search/path');

This is definitive better then before.

tobiasb moved up the "?", because it saves going into is_array, if there is a destination. I don't think it makes sense to set query and argument, when having an destination.

wmostrey’s picture

wmostrey commented

I double-checked if all instances in core were covered in Tobias' patch at #3 and they are. From what I can see it's good to go.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented 
+++ modules/path/path.admin.inc	20 Sep 2009 10:21:49 -0000
@@ -185,8 +185,7 @@
     $output = confirm_form($form,
-      t('Are you sure you want to delete path alias %title?', array('%title' => $path['dst'])),
-      isset($_GET['destination']) ? $_GET['destination'] : 'admin/config/search/path');
+      t('Are you sure you want to delete path alias %title?', array('%title' => $path['dst'])), 'admin/config/search/path');

We should still keep the parameters on separate lines. Ideally, also move the closing ); with proper indentation onto an own line.

+++ modules/system/system.module	20 Sep 2009 10:21:50 -0000
@@ -2265,7 +2265,7 @@
-
+  $path = isset($_GET['destination']) ? $_GET['destination'] : $path;
   // Prepare cancel link
   $query = $fragment = NULL;

- Please do not remove the blank line here.

- If we have a destination, then we probably need to parse $query and $fragment out of it, because url() only accepts those separately for internal URLs.

I'm on crack. Are you, too?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new drupal.confirm-form-destination.patch1.33 KB

Please merge this with your patch.

Status: Needs review » Needs work

The last submitted patch failed testing.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Needs review
StatusFileSize
new drupal.confirm-form-destination.patch2.43 KB

Thanks.

Fixed the style problems form #6 and merged the patch from #7.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review

Note that I'm writing a more generic helper function #578520: Make $query in url() only accept an array currently.

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new drupal.confirm-form.patch2.4 KB

This version works again...

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Issue tags: +D7 API clean-up

Tagging absolutely critical clean-ups for D7. Do not touch this tag. Please either help with this or one of the other patches having this tag.

andypost’s picture

andypost
he/him
Primary language Russian
commented

Suppose #14 should check for page=X argument right not destination with path=X is broken

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

This patch is actually trivial to do, now that we have drupal_parse_url().

But to pass tests, #600554: drupal_parse_url() doesn't work with clean URLs disabled is required. But anyhow, this patch could be prepared for that already.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Any takers?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new drupal.confirm-form-destination.patch2.78 KB

RTBC if bot passes.

andypost’s picture

andypost
he/him
Primary language Russian
commented

+1 as commited #600554: drupal_parse_url() doesn't work with clean URLs disabled

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

@andypost: Did you review this patch? Is it RTBC?

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Reviewed & tested by the community

tested on a clean install - it works now with page=X argument too!

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -API clean-up, -D7 API clean-up

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue tags: +API clean-up, +

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.