Make WordPress Core

Opened 14 months ago

Closed 13 months ago

Last modified 13 months ago

#57436 closed feature request (fixed)

Add hook to wp_set_password

Reported by: tanner-m's profile tanner m Owned by: audrasjb's profile audrasjb
Milestone: 6.2 Priority: normal
Severity: normal Version:
Component: Users Keywords: has-patch needs-dev-note add-to-field-guide has-unit-tests commit
Focuses: Cc:

Description

In WordPress 4.4 an action was added for the password reset option, however, I am working on a Single Sign-On application and need to sync the password every time it is updated. Many plugins call wp_set_password directly, so I'd like to propose adding an action to the end of the function to catch all instances of password setting.

Attachments (3)

wp_set_password.patch (480 bytes) - added by tanner m 14 months ago.
Patch 1
wp_set_password-2.patch (707 bytes) - added by tanner m 14 months ago.
wp_set_password_action.patch (706 bytes) - added by tanner m 14 months ago.
Patch - V3

Download all attachments as: .zip

Change History (26)

@tanner m
14 months ago

Patch 1

#1 @audrasjb
14 months ago

  • Keywords has-patch changes-requested added
  • Milestone changed from Awaiting Review to 6.2
  • Owner set to audrasjb
  • Severity changed from trivial to normal
  • Status changed from new to reviewing
  • Type changed from enhancement to feature request

Hello and thanks for the ticket!

Looks like a simple and relevant feature and it has a patch.
Let's move this for 6.2 consideration.

However, the patch should be updated to add a Docblock for this new action :)

#2 @tanner m
14 months ago

Thanks! I'll update with the Docblock

#3 @audrasjb
14 months ago

Thanks for this new patch @tanner-m!

Looks almost good :)

Sorry for nitpicking but we have two small things to address before committing this, to make sure it's compliant to WordPress Coding Standards:

  • Add a period at the end of the short description
  • Put the @since mention between the shortdesc and the parameters

Just like this:

/**
 * Fires after the password is set.
 *
 * @since 6.2.0
 *
 * @param string $password The plain text password just set.
 * @param mixed  $user_id  The ID of the user whose password was just reset.
 */     
do_action( 'wp_set_password', $password, $user_id );

#4 @tanner m
14 months ago

Thanks! I'll get the hang of it eventually. :P

Just updated the patch.

@tanner m
14 months ago

Patch - V3

#5 @audrasjb
14 months ago

  • Keywords needs-dev-note add-to-field-guide commit added; changes-requested removed

Alright, this new patch looks good to me, thanks!

Adding needs-dev-note workflow keyword to not forget to mention this change in a WP 6.2 Dev Note :)

#6 @audrasjb
14 months ago

I was wondering whether the action should be triggered before or after clean_user_cache() but your implementation (after) looks like the best option, since it's better to clean the cached data before allowing operations to plugin authors. They can always re-clean cached data in their function, depending on what they are doing with the hook.

👆 Perhaps something worth mentioning in the dev note?

#7 @audrasjb
14 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 55056:

Users: Add an action hook on wp_set_password().

This changeset introduces the wp_set_password action hook, triggered after a password is set for a given user. As several plugins are calling wp_set_password() directly, adding an action to the end of the function will help plugin authors to catch all instances of password setting.

Props tanner-m, audrasjb.
Fixes #57436.

#8 @audrasjb
14 months ago

  • Keywords needs-unit-tests added; commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Hmm. Reopening as it would be nice to add unit tests for this new hook.

This ticket was mentioned in PR #3844 on WordPress/wordpress-develop by @audrasjb.


14 months ago
#9

  • Keywords has-unit-tests added; needs-unit-tests removed

#10 @SergeyBiryukov
14 months ago

In 55057:

Docs: Correct the type of the $user_id parameter in wp_set_password action.

Follow-up to [6600], [55056].

See #57436.

#11 @audrasjb
14 months ago

PR3844 adds a unit test for this new hook.

By the way, @SergeyBiryukov thanks for the docblock fix!

@SergeyBiryukov commented on PR #3844:


14 months ago
#12

Thanks for working on the test!

We should be able to test that the action works as expected by checking the call count via MockAction:

public function test_wp_set_password_action() {
	$action = new MockAction();

	add_action( 'wp_set_password', array( $action, 'action' ) );

	wp_set_password( 'A simple password', self::$user_id );

	$this->assertSame( 1, $action->get_call_count() );
}

This approach is used in other tests and seems simpler that writing a meta value 🙂

@SergeyBiryukov commented on PR #3844:


14 months ago
#13

Thanks for working on the test!

We should be able to test that the action works as expected by checking the call count via MockAction:

public function test_wp_set_password_action() {
	$action = new MockAction();

	add_action( 'wp_set_password', array( $action, 'action' ) );

	wp_set_password( 'A simple password', self::$user_id );

	$this->assertSame( 1, $action->get_call_count() );
}

This approach is used in other tests and seems simpler that writing a meta value 🙂

@audrasjb commented on PR #3844:


13 months ago
#14

Oh ok, thanks @SergeyBiryukov 👍

I need to keep myself more up to date on the functions that can be used. Do you have a starting point resource in mind to discover the functions that can be used within WP core tests?

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


13 months ago

#16 @costdev
13 months ago

  • Keywords changes-requested added

This ticket was reviewed during the recent bug scrub. I've reviewed the PR and left a couple of small additions. Adding changes-requested to note the current status of the ticket and PR.

Additional props: @mukesh27

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


13 months ago

This ticket was mentioned in PR #4020 on WordPress/wordpress-develop by @costdev.


13 months ago
#18

_This is an update of #3844 to change "Test" to "Tests" in the docblock and to add a @covers annotation_

Trac ticket: https://core.trac.wordpress.org/ticket/57436

#19 @costdev
13 months ago

I've submitted PR 4020 to make small adjustments to PR 3844.

@audrasjb Is this one ready for commit?

Additional props: @mukesh27

#21 @audrasjb
13 months ago

  • Keywords commit added; changes-requested removed

Thanks for the new PR!

#22 @audrasjb
13 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 55250:

Build/Test Tools: Add unit tests for wp_set_password hook.

Follow-up to [55056].

Props audrasjb, SergeyBiryukov, costdev.
Fixes #57436.

Note: See TracTickets for help on using tickets.