Make WordPress Core

Opened 8 years ago

Last modified 3 weeks ago

#38079 assigned enhancement

Add hooks before output for each action in wp-login.php

Reported by: jfarthing84's profile jfarthing84 Owned by: voldemortensen's profile voldemortensen
Milestone: Future Release Priority: normal
Severity: normal Version: 4.7
Component: Login and Registration Keywords: has-patch reporter-feedback changes-requested close
Focuses: Cc:

Description

7 years ago, in #9682, wp-login.php was made more pluggable. The thing is, if you just want to change the look (as my plugin "Theme My Login" does), you also have to replicate the logic.

Having an action that fires just before the login_header() call in each case of the action handler switch should be sufficient. One such hook is already present for one action: lost_password.

However, the hook register is already in use for the register link as is the format {$action}_form. So, I propose using pre_{$action}_form.

Patch incoming.

Attachments (2)

38079.patch (2.3 KB) - added by jfarthing84 8 years ago.
38079.2.patch (1.4 KB) - added by dingo_bastard 7 years ago.
Added action hooks inside login_header function

Download all attachments as: .zip

Change History (22)

@jfarthing84
8 years ago

#1 @jfarthing84
8 years ago

  • Keywords has-patch dev-feedback added
  • Summary changed from Add additional hooks for each handled action in wp-login.php to Add hooks before output for each action in wp-login.php

#2 @voldemortensen
7 years ago

  • Keywords commit added
  • Owner set to voldemortensen
  • Status changed from new to assigned

38079.patch looks good to me, the @since docs will need to be updated though.

#3 @voldemortensen
7 years ago

  • Milestone changed from Awaiting Review to 4.8

#4 @dingo_bastard
7 years ago

While I agree that this is cool thing to have, login_header function already has a lot of filters and action hooks that you can use to modify how the login form looks. So you can easily use login_header action for instance to place something before logo in the login form.
There is also login_messages filter that you can use to modify the login messages.

It would be better to place actions inside the login_header function, you could also filter them based on the error message.

Last edited 7 years ago by dingo_bastard (previous) (diff)

@dingo_bastard
7 years ago

Added action hooks inside login_header function

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


7 years ago

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


7 years ago

#7 @jbpaul17
7 years ago

  • Milestone changed from 4.8 to 4.8.1

Punting to 4.8.1 per today's bug scrub.

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


7 years ago

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


7 years ago

#10 @jbpaul17
7 years ago

  • Milestone changed from 4.8.1 to 4.9

Per today's bug scrub, we'll punt this as the focus for 4.8.1 is regressions only.

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


7 years ago

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


7 years ago

#13 @voldemortensen
7 years ago

  • Keywords dev-feedback removed

38079.patch still applies cleanly, @since docs just need to be updated.

#14 @SergeyBiryukov
7 years ago

  • Keywords reporter-feedback added; commit removed

I agree with @dingo_bastard that existing hooks in login_header() can be used to customize the login page, so I don't see how 38079.patch would make it more customizable.

38079.2.patch makes more sense to me, but I'd like know a use case for these new actions that cannot be achieved with existing hooks.

#15 @SergeyBiryukov
7 years ago

On second thought, hooks before any output might indeed be helpful for redirects or more extensive customizations.

Perhaps they need better names though. pre_lostpassword_form sounds like it fires after the header and right before the form, not before any output on the page.

If the goal is to short-circuit login_header(), maybe pre_login_header should be added instead.

#16 @SergeyBiryukov
7 years ago

  • Milestone changed from 4.9 to Future Release

#17 @SergeyBiryukov
7 years ago

There's a login_form_{$action} dynamic hook, so maybe pre_login_form_{$action}_header would be a better name for these new hooks from 38079.patch, which would expand to:

  • pre_login_form_lostpassword_header
  • pre_login_form_resetpass_header
  • pre_login_form_register_header
  • pre_login_form_login_header

#18 @dingo_bastard
7 years ago

The do_action hooks I added could be proven useful to add some additional messages (I think I was working for a client 6 months ago, and I could have benefitted from such hooks).

#19 @hellofromTonya
3 weeks ago

  • Keywords changes-requested added

The request is to add a hook _before_ each action's output (i.e. the HTML sent out to the browser).

Having an action that fires just before the login_header() call in each case of the action handler switch should be sufficient.

Why doesn't 38079.patch cover all of the actions? It's adding 5 pre hooks. But there are 9 output points for the actions and each of those invokes login_header(). Looking at the switch ( $action ) cases:

  • 1x in 'confirm_admin_email' action.
  • 1x in the 'lostpassword' and 'retrievepassword' actions.
  • 2x in the 'resetpass' and 'rp' actions.
  • 1x in the 'register' action.
  • 1x in the 'checkmail' action.
  • 1x in the 'confirmaction' action.
  • 2x in the 'login' and default actions.

These actions do not output HTML:

  • 'postpass'
  • 'logout'

Notice: login_header() is invoked 9 times and there 9 paths for the actions that output HTML.

So I'm wondering ... instead of adding individual do_action() just before each login_header() instance, why not add just one do_action() at the start of function itself?

There are a fews of ways to do this:

Option 1: The new hook follows "login_form_{$action}" convention:

function login_header( $title = 'Log In', $message = '', $wp_error = null ) {
	global $error, $interim_login, $action;

	do_action( "pre_login_form_{$action}", $title, $message, $wp_error );

Option 2: The new hook uses the function's name and also passes the $action.

function login_header( $title = 'Log In', $message = '', $wp_error = null ) {
	global $error, $interim_login, $action;

	do_action( "pre_login_header", $title, $message, $wp_error, $action );

Both of the above options accomplish the ticket's request and @SergeyBiryukov's observation:

On second thought, hooks before any output might indeed be helpful for redirects or more extensive customizations.

#20 @hellofromTonya
3 weeks ago

  • Keywords close added

Is it still needed today?

My first impressions lean towards "no" for these reasons:

  • The ticket being stale this long can indicate not enough interest or use cases.
  • As previously noted, there are plenty of filter and action hooks available for customization including style, messaging, redirects, etc.

In considering adding additional hooks, benefits vs impacts need to also be considered.

  • Impacts:
    • Performance: each new hook adds processing time and memory usage.
    • Maintenance: initial changes, long term backwards-compatibility, documentation, etc.
  • Benefits:
    • Are there enough use cases?
    • is there enough interests and value to users and extenders?

If benefits outweigh the impacts, then adding the hooks makes sense.

For the reasonings I shared, I'll mark the ticket as a close candidate. If there's no follow-up or continued discussion within 2 months, this ticket can be closed. Please note, if it gets closed, it could be reopened when there's enough interest and benefits to move it forward.

Note: See TracTickets for help on using tickets.