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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hunmonk’s picture

Assigned: Unassigned » hunmonk
Priority: Normal » Critical
Status: Active » Needs review
FileSize
6.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

Status: Needs review » Reviewed & tested by the community

tested.

pwolanin’s picture

patch applies cleanly - affected confirm forms' cancel link works

hunmonk’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
12.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

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

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

Status: Needs work » Needs review
FileSize
12.15 KB

stupid $form_state... :)

attached patch fixes.

hunmonk’s picture

FileSize
10.77 KB

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

webchick’s picture

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

Status: Reviewed & tested by the community » Needs work

Committed to HEAD. Update docs need to be added.

hunmonk’s picture

Status: Needs work » Fixed

docs added

Anonymous’s picture

Status: Fixed » Closed (fixed)