Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#52013 closed defect (bug) (fixed)

Duplicate wp_authorize_application_password_form actions

Reported by: johnbillion's profile johnbillion Owned by: timothyblynjacobs's profile TimothyBlynJacobs
Milestone: 5.6.1 Priority: normal
Severity: normal Version: 5.6
Component: Application Passwords Keywords: good-first-bug has-patch fixed-major
Focuses: docs Cc:

Description

There are two instances of the wp_authorize_application_password_form actions, but both are documented with different documentation and an apparent different use case. The documented parameters for the first instance are incorrect.

The first fires in the Authorize Application Password new password section, and the second fires in the Authorize Application Password form before the submit buttons.

It looks to me like the first action should be renamed as it does not fire within the password form, it fires when showing the user their new password.

Refs:

  1. https://core.trac.wordpress.org/browser/tags/5.6/src/wp-admin/authorize-application.php?marks=202-211#L200
  2. https://core.trac.wordpress.org/browser/tags/5.6/src/wp-admin/authorize-application.php?marks=227-241#L225

Attachments (2)

52013.patch (2.0 KB) - added by engahmeds3ed 3 years ago.
update wp_application_passwords_approve_app_request_success TO wp_application_passwords_approve_app_request_error and rename wp_authorize_application_password_form action to be wp_authorize_application_password_form_no_js
52013.3.patch (2.1 KB) - added by engahmeds3ed 3 years ago.
add missing passed arguments and adjust action name

Download all attachments as: .zip

Change History (10)

#1 @TimothyBlynJacobs
3 years ago

Yeah, not sure how I ended up doing that. There is another instance in auth-app.js where the second wp_application_passwords_approve_app_request_success should actually be wp_application_passwords_approve_app_request_error.

For clarity, the first PHP action will only end up firing in the no-JS version of that page. Maybe we should include that in the action name so developers aren't confused when they don't see hooks added to it working.

@engahmeds3ed
3 years ago

update wp_application_passwords_approve_app_request_success TO wp_application_passwords_approve_app_request_error and rename wp_authorize_application_password_form action to be wp_authorize_application_password_form_no_js

#2 @TimothyBlynJacobs
3 years ago

Thanks for the patch @engahmeds3ed!

Let's include in wp_authorize_application_password_form_no_js that this is the success variant, so wp_authorize_application_password_form_approved_no_js.

We also need to pass the $new_password to the no js hook, and the errorThrown var to the JS hook.

@engahmeds3ed
3 years ago

add missing passed arguments and adjust action name

#3 @hellofromTonya
3 years ago

  • Keywords has-patch added; needs-patch removed

#4 @johnbillion
3 years ago

  • Owner set to johnbillion
  • Status changed from new to reviewing

#5 @johnbillion
3 years ago

  • Owner johnbillion deleted

#6 @TimothyBlynJacobs
3 years ago

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

In 49920:

App Passwords: Correct authorize app action names and signatures.

When App Passwords was introduced, the wp_authorize_application_password_form and wp_application_passwords_approve_app_request_success hook were mistakenly duplicated and incorrectly documented. This commit corrects the hook names and ensures the correct parameters are passed.

Props johnbillion, engahmeds3ed.
Fixes #52013.

#7 @TimothyBlynJacobs
3 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport.

#8 @SergeyBiryukov
3 years ago

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

In 49998:

App Passwords: Correct authorize app action names and signatures.

When App Passwords was introduced, the wp_authorize_application_password_form and wp_application_passwords_approve_app_request_success hooks were mistakenly duplicated and incorrectly documented. This commit corrects the hook names and ensures the correct parameters are passed.

Props johnbillion, engahmeds3ed.
Merges [49920] to the 5.6 branch.
Fixes #52013.

Note: See TracTickets for help on using tickets.