In confirmation forms generated by confirm_form :
The "cancel" redirection link can only be set with a drupal path, without querystrings or fragments.

This for instance causes this behaviour :
- on admin/node, go to page 2
- check a node and "delete" it
- on confirmation, click cancel
- you're back on admin/node page 1 (and not page 2)

The attached patch accepts cancel destinations in the form :
foo/bar/baz?arg1=value&arg2=other#fragment
(as generated by drupal_get_destination...)

It does not alter the "admin/node deletion cancelled" decribed above (that's a mere example), but provides the needed functionnality to do so...

CommentFileSizeAuthor
#21 node.module_49.patch1.52 KByched
#20 system.module_45.patch1.81 KByched
#17 system.module_2_0.patch1.81 KByched
#15 system.module_44.patch1.81 KByched
#12 system.module_42.patch1.76 KByched
#6 system.module_41.patch1.39 KByched
#3 patch_1481.64 KBmoshe weitzman
#2 system.module_37.patch1.7 KByched
system.module_31.patch1.7 KByched

Comments

yched’s picture

yched commented
Status: Active » Needs review

status...

yched’s picture

yched commented
StatusFileSize
new system.module_37.patch1.7 KB

re-rolled against current cvs

moshe weitzman’s picture

moshe weitzman commented
Version: x.y.z » 5.x-dev
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new patch_1481.64 KB

works as advertised

reroll to remove fuzz

dries’s picture

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

Committed to CVS HEAD. Thanks.

Steven’s picture

Steven commented
Status: Fixed » Needs work

This patch is horribly broken. We do not overload menu paths with an extra syntax like this. Paths that includes '?' ':' or '#' cannot be used as confirm destinations after this patch.

Perhaps a more sensible solution is to move towards passing in destinations as keyed arrays (e.g. 'path', 'query', 'fragment') and treat string destinations are pure paths. This is backwards compatible and doesn't introduce on forbidden characters.

yched’s picture

yched commented
StatusFileSize
new system.module_41.patch1.39 KB

I'm not sure I get your reservations, but I'll take you word on it.
Here's a patch against current 5.0 code, (since my previous patch got in), that uses the approach you propose (the text documenting the parameter could probably be enhanced).

yched’s picture

yched commented
Status: Needs work » Needs review
dries’s picture

dries commented

I rolled back to patch but don't like the patch proposed in #6 either.

yched’s picture

yched commented

Alright - any hint on what is wrong ? coding style ? the array argument ? the whole idea ? I still think the example I provided in my original post above about admin/node is a usability issue...

Anyway, here's the patch in #6 rerolled and more secured.

Steven’s picture

Steven commented

yched's: The rerolled patch is missing?

I think the idea of passing in destinations as arrays has lots of promise. Many functions are now bogged down by having to include $path, $query, $fragment as arguments, even though they are rarely used. With the proposed solution, they can all be replaced by a single $destination argument, which takes a path string, or a keyed array (path, query, fragment). We could even allow query to be an array too, and transparently perform query string encoding.

It also matches with the principle of keeping data in the most natural form (i.e. a structured array of plain text) rather than baked into an encoded string (i.e. a URL).

drumm’s picture

drumm
he/him
Location NY, US
commented
Version: 5.x-dev » 6.x-dev
Category: bug » task

Looks like an API change now.

yched’s picture

yched commented
Version: 6.x-dev » 5.x-dev
Category: task » bug
StatusFileSize
new system.module_42.patch1.76 KB

@Steven : Er, right. Here it is.

@drumm : It is not an actual API change - no compatibility break, existing calls to confirm_form remain unchanged.
The purpose here is a usability enhancement (have 'op cancel' work well with paged lists), so I think this is still for 5.0.

drumm’s picture

drumm
he/him
Location NY, US
commented
Status: Needs review » Needs work

NULL should always be uppercased.

$query and $fragment will need to be initialized to something or not used if $path isn't an array().

yched’s picture

yched commented
Status: Needs work » Needs review

updated per drumm's comments

yched’s picture

yched commented
StatusFileSize
new system.module_44.patch1.81 KB

and the patch.

RobRoy’s picture

RobRoy commented
Status: Needs review » Needs work

The third default arg sould be array(), not NULL for l().

Also, I think we should be consistent with _submit handler return values that just return a non-associative array like return array('pathval', array(), 'queryval', 'fragval'); not array('path' => 'pathval', ...); or did that change in 5.x?

yched’s picture

yched commented
Status: Needs work » Needs review
StatusFileSize
new system.module_2_0.patch1.81 KB

That did not change in 5.0 - but that is not (or completely) documented :
the doc for drupal_submit_form reads
"@return : A string containing the path of the page to display when processing is complete."

The non-associative way seems less clean. It's sort of ok for hook_submit return values because
they are fed directly to drupal_goto with call_user_func_array('drupal_goto', $goto);
We can't do this here, so it means code like :

$query = isset($path[2]) ? $path[2] : NULL;
$fragment = isset($path[3]) ? $path[3] : NULL;

which seems awkward. Plus what would we put in the doc for the $path parameter ?
'a drupal path as à string, or an array in the form array('drpath', array(), 'query', 'fragment').
Not nice. The little consistency we gain does not balance that IMO.

I tend to agree with Steven in comment #10 above : we should move the other way around and
generalize (in drupal 6) 'paths as associative array' in the l / drupal_goto / hook_submit / etc functions

NULL default value : right - attached patch corrects this.

RobRoy’s picture

RobRoy commented

Yeah, I agree with the associative array. You're right. IMO we should make this consistent the other way then and have associative arrays returned from _submit() FAPI callbacks. Issue for that at http://drupal.org/node/100730.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Needs work

i'd like to see one instanced of this in the patch so we know that the new syntax works.

yched’s picture

yched commented
StatusFileSize
new system.module_45.patch1.81 KB

Er, there was a typo my in #17 patch ($path['$path'] instead of $path['path']).
Here it is re-rolled.

yched’s picture

yched commented
StatusFileSize
new node.module_49.patch1.52 KB

And per moshe's request, here is an _example_ use case, as a patch for node.module
It implements a fix for the behaviour I described in my original message in this thread
(let the user get back to the right page if he cancels a delete operation on admin/content/node)

To test this, you naturally need to have enough nodes, so that admin/content/node is paged...

Please note : of course this is for demonstration only - it might be not be the right and clean way to do this.
If and when the confirm_form patch is committed, I'll submit a proper patch for review (for this and for the related user and comments delete operations).

yched’s picture

yched commented
Status: Needs work » Needs review
Steven’s picture

Steven commented
Title: querystring on confirm forms » Allow usage of query string on confirm forms
Status: Needs review » Fixed

I committed #20 to HEAD. Yes it's an API change, but better than the alternative, and a useful ability to have.

We need to look at the confirm forms in core and see if we need to include a query string. However, the propsed example uses icky stuff like substr(..., 12) which is weird. I suggest you open up another issue if this is important, and link it here.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)
moshe weitzman’s picture

moshe weitzman commented

did anyone fix the rest of the forms for this new syntax?