Posted by dww on June 23, 2007 at 7:56am
6 followers
| Project: | Drupal core |
| Version: | 6.x-dev |
| Component: | base system |
| Category: | task |
| Priority: | critical |
| Assigned: | hunmonk |
| Status: | closed (fixed) |
Issue Summary
Over at http://drupal.org/node/154033#comment-263588 I finally closely reviewed a DAPI-related patch. I found that the array you pass to drupal_delete_confirm() has a few ambiguous, confusing keys, in particular: 'path' vs. 'destination'. :(
We have:
- yes
- Text for the "yes" button
- no
- Text for the "no" button
- destination
- Path to redirect to after submitting the form
- path
- Path to go to when you hit "cancel"
Obviously, "path" is the outcast here. In IRC, hunmonk, UnConeD and I discussed a few alternatives:
- A nested array, something like:
drupal_confirm_delete(
array(
'yes' => array(
'button' => t('Do it now!'),
'path' => '/some/path',
),
'no' => array(
'button' => t('No thanks'),
'path' => '/somewhere/else',
),
)
); - Something wonky like: 'yes', 'yes_destination', 'no', 'no_destination'.
- Just rename 'path' to 'cancel'.
Choice (3) here seems like the easiest, and probably best option. Any other bright ideas before someone rolls a patch? It'd be great to do something about this before a) the code freeze and b) too many more DAPI conversion patches are done.
Thanks,
-Derek
Comments
#1
attached patch converts all references from 'path' to 'cancel' in both the deletion API and the confirm form. has been tested on both regular confirm forms and dapi confirm forms, and works perfectly. would be nice to have one more person have a look before we RTBC it.
moving to critical, b/c this is holding up conversions to the deletion API.
#2
tested.
#3
patch applies cleanly - affected confirm forms' cancel link works
#4
steven suggested we reorganize things a bit, and change the $path arg in confirm form to $destination, and default the cancel path to $destination -- since the submit destination and the cancel destination are usually the same, this is a good practical solution which will make things less confusing overall. you can still set a custom cancel path by passing a 'cancel' key in the $options array.
apologies that some other whitespace correction also slipped into this patch -- there's no easy way for me to get it out of there...
#5
p.s.: tested this on a few confirm forms, and it seems to work just fine for both dapi confirm forms and custom confirm forms.
#6
The cancel link on the single user delete confirm page takes you to user/[name] instead of user/[uid] ... And it also says "Are you sure you want to delete the account ?" instead of "Are you sure you want to delete the account [name]?" My guess is the calling function is passing in function parameters a bit wonky as the code looks correct. For some reason I wasn't able to track down where user_confirm_delete was called from, though.
#7
stupid $form_state... :)
attached patch fixes.
#8
...and a fix for path alias deletions. also removed the whitespace stuff.
#9
Tested both delete and cancel on the following:
- Deleting comments (single from node page, multiple from admin/content/comment, "top-level" comments with sub-comments)
- Nodes (single from node page, multiple from admin/content/node)
- Path aliases
- Users (single and multiple from admin/user/user)
All looks good. RTBC.
#10
Committed to HEAD. Update docs need to be added.
#11
docs added
#12