Add a Delete profile button

Hi manoloka, Please find attached patch for feature to Provide Delete own profile to user.

@joachim, I guess this is one of most useful feature that needed to be in module. Please check and review and merge the patch attached. It would be great to see this feature in action.

Thanks

I don't really understand the use case for deleting a profile. It doesn't really match the intended architecture of the module.

@joachim, Yes, this patch lets you delete only your own profile unless otherwise set in permissions. admin on the other hand will always be able to delete all the user's profile information as set by permissions.

Thanks

@manoloka, Please check attached updated patch. This should cover both the options for this feature.

@joachim, would really love to see this feature in module. Please review and merge this patch. Let me know if any queries.

Thanks

> Please review and merge this patch. Let me know if any queries.

Reviewed, but there are a whole load of problems with this patch... please check your patch at least complies with Drupal coding standards before you invoke a maintainer ;)

1. +++ b/contrib/profile2_page.inc
   @@ -54,6 +54,19 @@ function profile2_form($form, &$form_state, $profile) {
   +  } else {
   

   Coddled else.

2. +++ b/contrib/profile2_page.inc
   @@ -54,6 +54,19 @@ function profile2_form($form, &$form_state, $profile) {
   +    foreach (profile2_get_types() as $type) {
   +      $type_name = check_plain($type->type);
   

   Why iterate over all the types? This is the form for a particular profile entity, so you should only be considering its type.

3. +++ b/contrib/profile2_page.inc
   @@ -54,6 +54,19 @@ function profile2_form($form, &$form_state, $profile) {
   +        $form['actions']['delete'] = array(
   

   (And this makes no sense anyway: you're repeatedly clobbering the form element in the loop.)

4. +++ b/contrib/profile2_page.inc
   @@ -80,8 +93,19 @@ function profile2_form_submit_delete($form, &$form_state) {
   +    $confirm_question = t('Are you sure you want to delete your own profile ?');
   

   Include the profile type name in this. And perhaps make it clear to users that this is NOT deleting their account!

5. +++ b/contrib/profile2_page.inc
   @@ -80,8 +93,19 @@ function profile2_form_submit_delete($form, &$form_state) {
   +  } elseif (user_access('administer profiles')) {
   

   Coddled.

6. +++ b/contrib/profile2_page.inc
   @@ -80,8 +93,19 @@ function profile2_form_submit_delete($form, &$form_state) {
   +    $uid = arg(1);
   

   Get this out of the profile, not the arg().

7. +++ b/contrib/profile2_page.inc
   @@ -80,8 +93,19 @@ function profile2_form_submit_delete($form, &$form_state) {
   +      return confirm_form($form, $confirm_question, $profile->path());
   

   Move this out of the condition, and only set $confirm_question in the condition.

8. +++ b/profile2.module
   @@ -124,10 +124,56 @@ function profile2_menu() {
   +  foreach (profile2_get_types() as $type) {
   +    $type_name = check_plain($type->type);
   +    $items['user/profile/%/delete'] = array(
   

   Again, you're clobbering an item in a loop. Makes no sense.

9. +++ b/profile2.module
   @@ -124,10 +124,56 @@ function profile2_menu() {
   +        'page arguments' => array('profile2_delete_confirm_form', $type_name, 2),
   

   Is there a %profile menu loader we can use here?

10. +++ b/profile2.module
    @@ -124,10 +124,56 @@ function profile2_menu() {
    +function profile2_delete_confirm_form($form, &$form_state, $type_name, $uid) {
    

    Move this to a .inc file.

11. +++ b/profile2.module
    @@ -124,10 +124,56 @@ function profile2_menu() {
    +  } else {
    

    Coddled.

12. +++ b/profile2.module
    @@ -124,10 +124,56 @@ function profile2_menu() {
    +    drupal_access_denied();
    +    module_invoke_all('exit');
    +    drupal_exit();
    

    Not the right way to use this. For starters, AFAIK you return drupal_access_denied().

    And anyway, use a menu access callback!!

13. +++ b/profile2.module
    @@ -124,10 +124,56 @@ function profile2_menu() {
    +  } else {
    

    Coddled!

14. +++ b/profile2.module
    @@ -124,10 +124,56 @@ function profile2_menu() {
    +    drupal_exit();
    

    Pointless. If you have no access to the form, you can't ever get here.

15. +++ b/profile2.module
    @@ -376,6 +425,36 @@ function profile2_form_user_profile_form_alter(&$form, &$form_state) {
    +      $type_name = check_plain($type->type);
    

    No need to check_plain() here. Types are machine names.

16. +++ b/profile2.module
    @@ -376,6 +425,36 @@ function profile2_form_user_profile_form_alter(&$form, &$form_state) {
    +  foreach (profile2_get_types() as $type_name => $type) {
    +    $profile = profile2_load_by_user($user->uid,$type_name);
    +    if (isset($profile) && is_object($profile)) {
    

    Why all this? Get the profile you want from the form state!

17. +++ b/profile2.module
    @@ -376,6 +425,36 @@ function profile2_form_user_profile_form_alter(&$form, &$form_state) {
    +      $form_state['redirect'] = 'user/profile/'.$profile->uid.'/delete';
    

    Spaces around . operator.

18. +++ b/profile2.module
    @@ -376,6 +425,36 @@ function profile2_form_user_profile_form_alter(&$form, &$form_state) {
    +    drupal_set_message(t('Some error in deleting profile.'));
    

    ???!

@joachim,

Please find attached revised patch as per suggestions.

Make a note, "file" does not apply to other callbacks (only page callback) and since our page callback is 'drupal_get_form', so seems not matching the ideal conditions to move it to .inc file.

For menu access callback, however tried every possible way, but strangely its not applying as it should so couldn't use.

Let me know if any further advice to fine tune the patch, otherwise, good to go ahead and merge.

@manoloka, Please use attached updated patch. This one is after taking care of suggestions from joachim.

Thanks

1. +++ b/contrib/profile2_page.inc
   @@ -32,6 +32,7 @@ function profile2_page_view($profile) {
   +  global $user;
   
   @@ -80,7 +86,15 @@ function profile2_form_submit_delete($form, &$form_state) {
   +  $account = $GLOBALS['user'];
   

   I don't mind which form you use for this, but be consistent ;)

2. +++ b/contrib/profile2_page.inc
   @@ -46,10 +47,15 @@ function profile2_form($form, &$form_state, $profile) {
   +    $str_button_value = t('Delete profile');
   

   Avoid abbreviations in variables. $delete_button_label will do.

3. +++ b/contrib/profile2_page.inc
   @@ -46,10 +47,15 @@ function profile2_form($form, &$form_state, $profile) {
   +  } elseif (user_access("delete own $profile->type profile") && $user->uid === $profile->uid) {
   

   Uncoddle please.

4. +++ b/contrib/profile2_page.inc
   @@ -80,7 +86,15 @@ function profile2_form_submit_delete($form, &$form_state) {
   +    $confirm_question = t("Are you sure you want to delete your own $profile->type profile ?");
   

   Can we have more detail here?

5. +++ b/contrib/profile2_page.inc
   @@ -80,7 +86,15 @@ function profile2_form_submit_delete($form, &$form_state) {
   +    $user_account = isset($profile->uid) ? user_load($profile->uid) : '';
   

   How would $profile->uid be empty? What are you doing in that case?

6. +++ b/profile2.module
   @@ -123,7 +123,15 @@ function profile2_menu() {
   +  // Menu access callback is not behaving as expected
   +  // Controlling access in function
   

   Can you explain the problem?

7. +++ b/profile2.module
   @@ -1088,3 +1123,40 @@ function profile2_ctools_plugin_directory($owner, $plugin_type) {
   +  if (empty($profile)) {
   +    $profile = profile2_create(array('type' => $type_name, 'uid' => $uid));
   +  }
   

   Why create it only to delete it?

8. +++ b/profile2.module
   @@ -1088,3 +1123,40 @@ function profile2_ctools_plugin_directory($owner, $plugin_type) {
   +  if ($user->uid === $profile->uid) {
   

   You're not checking the permission here.

9. +++ b/profile2.module
   @@ -1088,3 +1123,40 @@ function profile2_ctools_plugin_directory($owner, $plugin_type) {
   +    $confirm_question = t("Are you sure you want to delete your own $type_name profile ?");
   

   Use the label, not the machine name.

10. +++ b/profile2.module
    @@ -1088,3 +1123,40 @@ function profile2_ctools_plugin_directory($owner, $plugin_type) {
    \ No newline at end of file
    

    Missing terminal newline.

Still LOTS of coddled elses in this patch!! :(

@joachim,

Your reviews are covered and below are updates for the same.

01. Done. Removed the second usage.
02. Done. I think, using "str" is kinda self explanatory that it is string type variable. Seems not needed. Updated the variable name.
03. Done.
04. "more detail" as in ? Please explain.
05. Done. Thought to be more precise, but again, seems not needed. Updated the code.
06. No matter what I write in "access callback" and "access arguments", it always returns "Access denied" even for super admin.
07. To ensure, that even if user hits a URL directly through browser then he won't get error in case of profile not already created.
08. Well, permission is already checked in form_alter so no need to check twice.
09. Done.
10. Done.

Please check and let me know if any further work needed.

Thanks

Not been done:

- Move this to a .inc file.
- Is there a %profile menu loader we can use here?

Could you check the other things I've mentioned in my 2 previous reviews?

> 04. "more detail" as in ? Please explain.

As I said in my first review, I think this needs to explain more to the user.

// Menu access callback is not behaving as expected
  // Controlling access in function

I think this is because you're inheriting the access callback from a parent menu item. Try declaring it explicitly.

  $items['user/profile/%/%/delete'] = array(

Change this path to fit in with the scheme of the rest of the module.

- Move this to a .inc file.

Sandip: "file" does not apply to other callbacks (only page callback) and since our page callback is 'drupal_get_form', so seems not matching the ideal conditions to move it to .inc file.

- Is there a %profile menu loader we can use here?

Sandip: As far as I know, we dont have so can not.

As I said in my first review, I think this needs to explain more to the user.

Sandip: "Are you sure you want to delete your own $profile->type profile ?" Line itself speaks for it. Give me exact words you want it to be displayed otherwise seems correct to me.

I think this is because you're inheriting the access callback from a parent menu item. Try declaring it explicitly.

Sandip: If declared it explicitly, I would need to set 'access callback' => 'profile2_profile2_access' and for this function, it needs profile object as second parameter, which is not available in hook_menu. So it doesn't work for 'administrator' user to whom every profile should be available to delete.

$items['user/profile/%/%/delete'] = array( Change the path

Sandip: Well, had to set this path so that I can pass both uid as well as profile type to function for further access checking. If you think of any other path, which still can pass uid and profile type, then sure, please share it with me and will implement it.

Thanks

> Sandip: "file" does not apply to other callbacks (only page callback) and since our page callback is 'drupal_get_form', so seems not matching the ideal conditions to move it to .inc file.

It's completely fine to put form callbacks in an .inc file like this. Look at the admin settings form in most modules.

> - Is there a %profile menu loader we can use here?
> Sandip: As far as I know, we dont have so can not.

/**
 * Menu load callback.
 *
 * Returns the profile object for the given user. If there is none yet, a new
 * object is created.
 */
function profile2_by_uid_load($uid, $type_name) {

I think it's fine to move this into profile module.

> Sandip: If declared it explicitly, I would need to set 'access callback' => 'profile2_profile2_access' and for this function, it needs profile object as second parameter, which is not available in hook_menu. So it doesn't work for 'administrator' user to whom every profile should be available to delete.

Should be profile2_access(). Don't call hook implementations directly. Should work once you use the menu loader from profile2_page. And also, writing a new menu access callback would still be better than having to return denied in the form builder!

> If you think of any other path, which still can pass uid and profile type, then sure, please share it with me and will implement it.

I would say follow the same pattern as profile2_page. Which looks like user/UID/profile-TYPE/delete

Hello joachim,

Please find attached patch for review.

- Move this to a .inc file.

Sandip: Done

- Is there a %profile menu loader we can use here? I think it's fine to move this into profile module.

Sandip: Done

Should be profile2_access(). And also, writing a new menu access callback would still be better than having to return denied in the form builder!

Sandip: Done

I would say follow the same pattern as profile2_page. Which looks like user/UID/profile-TYPE/delete

Sandip: Done

Thanks

@joachim, any comments on uploaded patch and its review ? If everything is fine, would love to see the same merged and committed.

Thanks and appreciate your prompt response.

@sandip27

I noticed that the delete profile button exists when no profile exists (i.e. creating a new profile for an existing user).

@sandip27,

I also noticed there isn't a 'delete any TYPE profile' permission. I think this should be added for administration purposes and to match the permissions already provided.

@jantoine, thanks for the comments.

1. Well, the delete button exists in order to match its current status. If you check the default module by joachim, its behaving the same ways. So its good to not deviate from existing functionality hence is the button.

2. Again, this patch is more of like providing the delete button to profile and not to manage the permissions etc. So not handling it in this one. Sure, will take care of it once this patch gets merged.

Thanks and again, appreciate your comments.

Sandip

@joachim, have spent lots efforts and time on this enhancements. Would really love to see it in action. Since, its been over a month now, so Can you please take it further ahead and let me know if and when can we see this change merged and live ?

Thanks and appreciate your efforts on the same.

+++ b/profile2.module
@@ -124,6 +124,16 @@ function profile2_menu() {
+  $items['user/%/%/delete'] = array(

You're not using the menu loader in your menu item.

@joachim, Sorry, my bad ! Got skipped somehow. Fixed the same and here's updated patch. Please do review and if everything is fine, would love to see it merged.

Thanks again !!

Can any other maintainer of module review and merge this please ? Its been really long and would really appreciate if anybody could help me achieve this.

Thanks

@joachim, OR Any maintainers, it's been 4 months or so for the patch submission. Really not possible to review it ? Awaiting for the response.

Thanks

@joachim, still no word on merging this patch. :(

Thought, by this time, we would have already done it. It would be really great to see it in action.

Thanks.

This all seems to work, and is compatible with 7.x-1.4.

Not everyone will want it (I don't on my sites), but by default the permission is not enabled so the Delete button doesn't appear, which is fine.

Can anyone else comment, and move it to RTBC?

Would this support a delete action for rules or strictly only the delete button on page?

There was one mis-feature in this patch, in that it showed a Delete button when creating a new profile, This didn't happen in the original administrator-delete in Profile2-page. I've corrected this, see attached patch. I've also changed the text "Delete your profile" to "Delete this profile". Where a user has multiple profiles the former could be confusing, and imply deleting all profiles.

Confirmed working with 7.x-1.4.

I think this patch requires the profile2_page module (is it really necessary?), and admin can't delete his own profile from what I see in the patch.

@jcisio

It doesn't depend on profile2_page, the patch changes both versions of the profile edit form - the one in the main module and the one in the profile2_page module.

However, I can see that there is a slight hole in the logic, in that a user with administer-profiles permission still needs delete-own-profile permission to be able to delete their own.

I'll aim to fix that in a future release.

Thanks for your sharp observation!