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:

  1. 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',
    ),
  )
);
  2. Something wonky like: 'yes', 'yes_destination', 'no', 'no_destination'.
  3. 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

CommentFileSizeAuthor
#8 dapi_path_fix_2.patch10.77 KBhunmonk
#7 dapi_path_fix_1.patch12.15 KBhunmonk
#4 dapi_path_fix_0.patch12.08 KBhunmonk
#1 dapi_path_fix.patch6.42 KBhunmonk

Comments

hunmonk’s picture

hunmonk commented
Assigned: Unassigned » hunmonk
Priority: Normal » Critical
Status: Active » Needs review
StatusFileSize
new dapi_path_fix.patch6.42 KB

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.

adrian’s picture

adrian commented
Status: Needs review » Reviewed & tested by the community

tested.

pwolanin’s picture

pwolanin commented

patch applies cleanly - affected confirm forms' cancel link works

hunmonk’s picture

hunmonk commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new dapi_path_fix_0.patch12.08 KB

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...

hunmonk’s picture

hunmonk commented

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.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs review » Needs work

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.

hunmonk’s picture

hunmonk commented
Status: Needs work » Needs review
StatusFileSize
new dapi_path_fix_1.patch12.15 KB

stupid $form_state... :)

attached patch fixes.

hunmonk’s picture

hunmonk commented
StatusFileSize
new dapi_path_fix_2.patch10.77 KB

...and a fix for path alias deletions. also removed the whitespace stuff.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs review » Reviewed & tested by the community

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.

Steven’s picture

Steven commented
Status: Reviewed & tested by the community » Needs work

Committed to HEAD. Update docs need to be added.

hunmonk’s picture

hunmonk commented
Status: Needs work » Fixed

docs added

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)