WordPress.org

Make WordPress Core

Opened 10 years ago

Closed 7 years ago

Last modified 7 years ago

#15173 closed defect (bug) (fixed)

duplicate calls to 'add_new_user_to_blog'

Reported by: sboisvert Owned by: nacin
Milestone: 3.6 Priority: normal
Severity: minor Version:
Component: Multisite Keywords: 3.2-early dev-feedback
Focuses: Cc:

Description

It seems that there is a default filter in ms-default-filters:
add_action( 'wpmu_activate_user', 'add_new_user_to_blog', 10, 3 );

That calls the add user to blog on wpmu_activate_user action. That in itself is fine but wpmu_activate_signup() has this code:

add_new_user_to_blog( $user_id, $user_email, $meta );
do_action('wpmu_activate_user', $user_id, $password, $meta);
(line 831-832 ms-functions.php current trunk);

That means that the default behavior is to call add_new_user_to_blog twice. This triggers the actions attached to add_user_to_blog to fire twice.
While removing the action works for me, I question if this is the intended or expected behavior of most users and would offer that this behavior should be debated.

Thank you,

Stéphane

Attachments (2)

15173.diff (548 bytes) - added by PeteMall 10 years ago.
15173.2.diff (971 bytes) - added by wonderboymusic 8 years ago.

Download all attachments as: .zip

Change History (11)

@PeteMall
10 years ago

#1 @PeteMall
10 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 3.1
  • Type changed from enhancement to defect (bug)

#2 follow-up: @scribu
10 years ago

I think we should remove the direct call instead.

Also, the second parameter should be deprecated, since it's not used.

#3 in reply to: ↑ 2 @wpmuguru
10 years ago

Replying to scribu:

I think we should remove the direct call instead.

Also, the second parameter should be deprecated, since it's not used.

I agree. Removing the direct call allows the activation process to be altered by a plugin simply by removing the default hook.

#4 @nacin
10 years ago

I spoke about Pete with this.. The issue is that we're currently passing the $user_email in one case and $password in the other, as the second parameter. Ideally we should be making that $user_email -- passing $password around is insane and useless. In that case, yes we can use the hook.

#5 @PeteMall
10 years ago

Changing the hook may break a plugin that is using it but I agree with nacin that we should not be passing the password at all. Some things to keep in mind:

  • Deprecate the email paramater for add_new_user_to_blog() since it is not used anymore?
  • Update the hook to use email instead of password?
  • Add new hook to use add_new_user_to_blog to avoid breaking a plugin?

#6 @PeteMall
9 years ago

  • Keywords 3.2-early dev-feedback added; has-patch removed
  • Milestone changed from 3.1 to Future Release

This has been in MU since before 2.0. We can address this in the next release.

#7 @wonderboymusic
8 years ago

  • Milestone changed from Future Release to 3.6

It's actually safer to remove the direct call. In add_new_user_to_blog( $user_id, $email, $meta ), $email is unused. So, even though passing password sucks, it is ignored by the function when this is what is passed: add_new_user_to_blog( $user_id, $password, $meta )

#8 @nacin
7 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In 23535:

Remove direct call to add_new_user_to_blog() as this is already hooked into wpmu_activate_user.

The second argument passed by that hook is not $email, so let's change that. It is not used.

props wonderboymusic.
fixes #15173.

#9 @nacin
7 years ago

Looking closer at this, the same three arguments (user_id, password, meta) are being passed to wpmu_welcome_user_notification(), passed to the wpmu_activate_user(), and then returned from wpmu_activate_signup(). So, not as alarming that $password is something actually used by a generic API function, which add_new_user_to_blog() is not. (Rather, there is add_user_to_blog() for that.)

If someone is removing that hook, they may have still been getting the user added anyway, but I'm not concerned about the change there. Since maybe_add_existing_user_to_blog() already goes through a hook ('init', blah), let's keep the consistency.

In all, this whole mess of signup code eventually needs cleanup one of these days. At that point, I imagine new functions and program flow.

Note: See TracTickets for help on using tickets.