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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mattyoung’s picture

mattyoung CreditAttribution: mattyoung commented

sub

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Active » Needs review
FileSize
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
CreditAttribution: tobiasb commented
FileSize
drupal_582456.patch2.67 KB

my version

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner 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 CreditAttribution: 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
CreditAttribution: sun 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
CreditAttribution: sun commented
FileSize
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
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
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
CreditAttribution: sun 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
CreditAttribution: dawehner commented
FileSize
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
CreditAttribution: sun 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
CreditAttribution: andypost 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
CreditAttribution: sun 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
CreditAttribution: sun commented

Any takers?

sun’s picture

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

RTBC if bot passes.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

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

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

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

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost 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 CreditAttribution: 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, +D7 API clean-up

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