Make WordPress Core

Opened 5 years ago

Last modified 4 days ago

#15384 assigned enhancement

wp-login.php refactor

Reported by: nacin Owned by: valendesigns
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Login and Registration Keywords: dev-feedback needs-unit-tests has-patch
Focuses: Cc:


wp-login.php needs some serious work. When looking to do some improvements in #5919, I realized I literally needed a goto in order to achieve the goals outlined in this comment:


I am thinking a WP_Login class with some methods that can handle various different forms, POST handling, and rerouting.

Attachments (8)

15384-2010-12-17.diff (59.3 KB) - added by norbertm 5 years ago.
15384-2010-12-20.diff (61.7 KB) - added by norbertm 5 years ago.
wp-login.php (1006 bytes) - added by norbertm 5 years ago.
class-wp-login.php (32.7 KB) - added by norbertm 5 years ago.
class-wp-login-20101221.php (35.9 KB) - added by norbertm 5 years ago.
php5 oop version
class-wp-login-20101222-r17107.php (36.1 KB) - added by norbertm 5 years ago.
included comment:10 and merged everything from trunk up till r17107
15384.patch (79.7 KB) - added by jfarthing84 8 days ago.
15384.2.patch (80.2 KB) - added by jfarthing84 6 days ago.
Fixed customize login and refreshed against trunk.

Download all attachments as: .zip

Change History (43)

#1 @holizz
5 years ago

  • Cc tom@… added

#3 @norbertm
5 years ago

  • Cc norbert@… added

#4 @norbertm
5 years ago

Included an early version of the refactored WP_Login class.

The generic idea is to encapsulate all initialization, a dispatch controller, several action handlers (handle_*, process_*), some service layer methods (do_*) and separate view rendering methods (render_*) from the original wp-login.php script.

Ideally, I'd like to separate requests that do not change state (GET) from those that do (POST) but the current action handlers do not let that happen easily. The idea is if it's a POST request handle that part first, collect any errors, then proceed with the GET part to compose a response by passing the errors to that method.

handle_login() needs some rethinking, especially at the end.

I'd like improve smaller things, for example, the success branch should return at the end of a function vs exit in the middle buried within if()s.

Interested to see if redirects are worth extracting out into their own methods.

In the end, I plan to update the entire file to follow the WP coding standards.

Extracted $aliases to serve as a map for duplicate (deprecated?) action names.

Any feedback is greatly appreciated.

#5 @norbertm
5 years ago

Another update.

The class is composed like this:

  • "Dispatch" routes the request to the proper action.
  • "Process" functions do the action processing and return errors if any. They call "Do" functions to do the actual service level work.
  • Finally the errors returned are passed to a "Render" function, which in turn calls one or many "Output" functions to output the HTML.

The class is organized so that PHP code is at the top and HTML at the bottom unless there's a better place for HTML.

It's still a work in progress but I'd love to get some feedback. What do you think so far?

5 years ago

#6 @norbertm
5 years ago

Taken a more object-oriented approach that is still PHP 4 compatible.

Even though I don't want to go too far with OOP, I really want to extract all HTML rendering functions into their own view classes because it's still a terrible mess.

Attached full files instead of a diff because it's a large set of changes.

#7 @nacin
5 years ago

No need to be PHP 4 compat in 3.2.

#8 @norbertm
5 years ago


Indeed. That's awesome! Let me switch gears then...

5 years ago

php5 oop version

#9 @norbertm
5 years ago

Created a PHP 5 OOP version that provides clarity and straightforwardness:

  • separates controllers and views
  • separates PHP and HTML code
  • one step closer to testability.

Simplified and reduced the kinds of methods compared to the previous version.

If you think it's too much OOP we can iterate but I would really prefer going forward internally while still providing hooks for the end users.


#10 @norbertm
5 years ago

Re: ticket description, it would be easy now to display the login form with a message after password reset simply by making the class at the bottom extend WP_Login_View_Login like this:

 * Outputs the login form with the password successfully reset message.
class WP_Login_View_ResetPass_Completed extends WP_Login_View_Login {
	 * Class constructor.
	public function __construct() {
		parent::__construct( '<p class="message reset-pass">' . __('Your password has been reset.') . '</p>' );

That's it.

Alternatively, you could create a new view and return it from any controller at any point to implement custom functionality.

#11 @norbertm
5 years ago

The same can be accomplished with a single WP_Login class and many functions of course. It would still make sense to keep a separate dispatch and many process_* and render_* functions to separate controller and view logic.

Any of this works for me, let me know.

5 years ago

included comment:10 and merged everything from trunk up till r17107

#12 @neoxx
5 years ago

  • Cc neo@… added

Nice to see a rework of good old wp-login.php. What about a few filters for the output as I've mentioned in #10006 ?

Another thought: Shouldn't we stop sending passwords in plain-text emails? (as we now also don't do for lost password requests #5919?) I mentioned that already in comment:39:ticket:2870.

#13 @Viper007Bond
5 years ago

It'd be cool if you could pre-fill the lost password form using a query argument. It's a simple change -- just change $_POST to $_REQUEST:

$user_login = isset($_POST['user_login']) ? stripslashes($_POST['user_login']) : '';
$user_login = isset($_REQUEST['user_login']) ? stripslashes($_REQUEST['user_login']) : '';

#14 @sboisvert
4 years ago

  • Cc stboisvert@… added

#16 @johnbillion
4 years ago

  • Cc johnbillion added

#17 @jeffstieler
4 years ago

  • Cc jeff@… added

#18 @beaulebens
4 years ago

  • Cc beau@… added

#20 @scribu
3 years ago

  • Type changed from defect (bug) to enhancement

I think having two classes (a controller and a renderer) for each view is a bit much.

We should have a single set of functions for outputting the various HTML bits, which are called by each controller with the appropriate parameters.

#21 @scottconnerly
3 years ago

  • Cc scott@… added

#22 @zombrows
3 years ago

  • Cc zombrows added

#23 @iseulde
2 years ago

  • Cc avrylnoxke@… added

#24 @iandunn
2 years ago

  • Cc ian.dunn@… added

#25 @jeremyfelt
21 months ago

  • Component changed from Administration to Login and Registration

This ticket was mentioned in IRC in #wordpress-dev by johnbillion. View the logs.

20 months ago

#27 @chriscct7
9 days ago

  • Keywords needs-patch added; 3.2-early removed
  • Milestone Future Release deleted
  • Resolution set to maybelater
  • Status changed from new to closed

Closing as maybelater. Complete lack of interest on the feature on the ticket over the last 3 years. Feel free to reopen when more interest re-emerges (particularly if there's a patch).

#28 @johnbillion
9 days ago

  • Milestone set to Awaiting Review
  • Resolution maybelater deleted
  • Status changed from closed to reopened

#29 @jfarthing84
9 days ago

I'm actually working on a proposal right now. Patch will be coming before the end of the day.

8 days ago

#30 @jfarthing84
8 days ago

  • Keywords has-patch dev-feedback added; needs-patch removed

My patch bascially moves all of the logic into a new file, wp-includes/class-wp-login.php, where it is encapsulated in a class. Most logic is abstracted into separate methods. All javascript is also moved into a new javascript file, wp-includes/js/wp-login.js.

This would be super awesome for my plugin, Theme My Login, to finally have access to these methods so I can avoid duplicating them. I'm sure other developers would benefit as well.

Last edited 8 days ago by jfarthing84 (previous) (diff)

#31 @jfarthing84
8 days ago

Also, please note that, for some reason, interim login from within the customizer doesn't work with this patch. Perhaps someone with more knowledge of the customizer would know why...

#32 @valendesigns
6 days ago

  • Keywords needs-patch needs-unit-tests added; has-patch removed
  • Owner set to valendesigns
  • Status changed from reopened to assigned

This ticket has the potential to greatly impact the Two Factor feature plugin. I can do a thorough review of 15384.patch in the next day or two and discuss my findings here and at the weekly meeting on Thursday in core-passwords.

In order to merge 2fa into Core for the 4.5 release, we would have to do something along these lines anyhow. So I'm for changing how we access these functions and build the wp-login.php page. Though making sure we are thinking about backwards compatibility, potential plugin breakage, and all possible use cases is extremely important for such a critical system.

@jfarthing84 Great effort so far, though because this patch is unfinished and doesn't work with interim login in the Customizer, I feel it should still be set to needs-patch.

Does anyone have objections to setting the milestone to 4.4?

#33 @jfarthing84
6 days ago

So, the rejection is happening when then AJAX action customize_refresh_nonces is called. The hook that handles this request is attached to wp_ajax_customize_refresh_nonces. Of course, all of the wp_ajax_ methods require you to be logged in. So, the call is rejected. Thing is, I don't understand why it doesn't happen that way with the original wp-login.php.

BTW, I'm testing by logging in, accessing the customizer, clearing all cookies and then attempting to customize.

6 days ago

Fixed customize login and refreshed against trunk.

#34 @jfarthing84
6 days ago

  • Keywords has-patch added; needs-patch removed

Silly me. With the original wp-login.php, the customize javascript wasn't loaded until after a successful login. I was loading it before. So, I fixed that. Also refreshed against the most current commit.

This ticket was mentioned in Slack in #core-passwords by valendesigns. View the logs.

4 days ago

Note: See TracTickets for help on using tickets.