Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#38781 closed defect (bug) (fixed)

Change args in `after_signup_user` action

Reported by: maximeculea's profile MaximeCulea Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 4.8 Priority: normal
Severity: normal Version: 4.4
Component: Users Keywords: has-patch 2nd-opinion
Focuses: multisite Cc:


Hi !

In 4.4.0 WP's version, 34112's ticket has been merged, including after_signup_user's action. The phpdoc is saying that $meta is an array :
* @param array $meta Additional signup meta. By default, this is an empty array.

It's not really true, because just before, l.723, we can find this : $meta = serialize($meta);

So or we update the phpdoc, saying it's a string (serialized array), or we change this 4th args' format.

This last option it's my favorite one, that's why I am proposing to change after_signup_user's action's 4th args from $meta into unserialize( $meta ), according to the phpdoc, which I agree with. It's much easier to use data which is already unserialized !

In fact for efficiency purpose, I prefer not to overwrite $meta value with serialized one, as l.723, but directly doing it inside wpdb->insert() args. This way, the $meta variable is not serialized and can be passed as it to after_signup_user's action's args.

By changing this behaviour, we could also fix how wpmu_signup_user_notification's method use given $meta.

Attachments (1)

patch-38781.diff (797 bytes) - added by MaximeCulea 7 years ago.

Download all attachments as: .zip

Change History (10)

#1 @MaximeCulea
7 years ago

  • Keywords has-patch 2nd-opinion added

This ticket was mentioned in Slack in #core by helen. View the logs.

7 years ago

#3 @helen
7 years ago

  • Version changed from trunk to 4.4

#4 @jeremyfelt
7 years ago

  • Milestone changed from Awaiting Review to Future Release

Thanks for opening a ticket and submitting a patch, @MaximeCulea. I'm going to milestone this as a future release as we should either fix the docs or the default here.

#5 @MaximeCulea
7 years ago

Nice to hear it, with pleasure !
@jeremyfelt, so nothing I can do further ?

#6 @Mista-Flo
7 years ago

+1 on this change, as an argument, wpmu_activate_signup function have a similar action :

do_action( 'wpmu_activate_user', $user_id, $password, $meta );

But just before this action, there is a $meta = maybe_unserialize($signup->meta);, so in my opinion we have to keep consistency with use of $meta array and don't pass a serialized string.

By the way, this other ticket #39223 will benefit from this one

Version 2, edited 7 years ago by Mista-Flo (previous) (next) (diff)

#7 @SergeyBiryukov
7 years ago

  • Milestone changed from Future Release to 4.8

#8 @SergeyBiryukov
7 years ago

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

In 39886:

Users: In wpmu_signup_blog() and wpmu_signup_user(), pass unserialized signup meta data to after_signup_site and after_signup_user filters introduced in [34112], to match the documented value.

Props MaximeCulea.
Fixes #38781.

#9 @SergeyBiryukov
7 years ago

In 39887:

Docs: Make $meta parameter description in multisite signup and registration functions more consistent.

See #38781.

Note: See TracTickets for help on using tickets.