Make WordPress Core

Opened 14 years ago

Closed 19 months ago

#15384 closed enhancement (maybelater)

wp-login.php refactor

Reported by: nacin's profile nacin Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Login and Registration Keywords: dev-feedback needs-unit-tests needs-patch close
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 14 years ago.
15384-2010-12-20.diff (61.7 KB) - added by norbertm 14 years ago.
wp-login.php (1006 bytes) - added by norbertm 14 years ago.
class-wp-login.php (32.7 KB) - added by norbertm 14 years ago.
class-wp-login-20101221.php (35.9 KB) - added by norbertm 14 years ago.
php5 oop version
class-wp-login-20101222-r17107.php (36.1 KB) - added by norbertm 14 years ago.
included comment:10 and merged everything from trunk up till r17107
15384.patch (79.7 KB) - added by jfarthing84 9 years ago.
15384.2.patch (80.2 KB) - added by jfarthing84 9 years ago.
Fixed customize login and refreshed against trunk.

Download all attachments as: .zip

Change History (47)

#1 @holizz
14 years ago

  • Cc tom@… added

#3 @norbertm
14 years ago

  • Cc norbert@… added

#4 @norbertm
14 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
14 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?

14 years ago

#6 @norbertm
14 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
14 years ago

No need to be PHP 4 compat in 3.2.

#8 @norbertm
14 years ago

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

14 years ago

php5 oop version

#9 @norbertm
14 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
14 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
14 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.

14 years ago

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

#12 @neoxx
13 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
13 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
13 years ago

  • Cc stboisvert@… added

#16 @johnbillion
12 years ago

  • Cc johnbillion added

#17 @jeffstieler
12 years ago

  • Cc jeff@… added

#18 @beaulebens
12 years ago

  • Cc beau@… added

#20 @scribu
12 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
12 years ago

  • Cc scott@… added

#22 @anonymized_8680371
12 years ago

  • Cc zombrows added

#23 @iseulde
11 years ago

  • Cc avrylnoxke@… added

#24 @iandunn
11 years ago

  • Cc ian.dunn@… added

#25 @jeremyfelt
10 years ago

  • Component changed from Administration to Login and Registration

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

10 years ago

#27 @chriscct7
9 years 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 years ago

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

#29 @jfarthing84
9 years ago

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

9 years ago

#30 @jfarthing84
9 years 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 9 years ago by jfarthing84 (previous) (diff)

#31 @jfarthing84
9 years 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
9 years 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
9 years 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.

9 years ago

Fixed customize login and refreshed against trunk.

#34 @jfarthing84
9 years 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.

9 years ago

#36 follow-up: @sebastian.pisula
8 years ago

I think that this should be in WordPress 4.6.0

#37 in reply to: ↑ 36 @jfarthing84
8 years ago

Replying to sebastian.pisula:

I think that this should be in WordPress 4.6.0

I think that this should be in WordPress 4.7.0 :-)

#38 @desrosj
20 months ago

  • Keywords needs-patch close added; has-patch removed
  • Milestone set to Awaiting Review
  • Owner valendesigns deleted

Re-adding a milestone and a close suggestion since it's been 7+ years without meaningful activity.

#39 @JeffPaul
19 months ago

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

I concur, closing as maybelater in case someone else comes back here and has a strong desire otherwise.

Note: See TracTickets for help on using tickets.