Opened 14 years ago
Closed 23 months ago
#15384 closed enhancement (maybelater)
wp-login.php refactor
Reported by: | nacin | Owned by: | |
---|---|---|---|
Milestone: | Priority: | normal | |
Severity: | normal | Version: | |
Component: | Login and Registration | Keywords: | dev-feedback needs-unit-tests needs-patch close |
Focuses: | Cc: |
Description
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:
http://core.trac.wordpress.org/ticket/5919#comment:39
I am thinking a WP_Login class with some methods that can handle various different forms, POST handling, and rerouting.
Attachments (8)
Change History (47)
#4
@
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
@
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?
#6
@
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.
#8
@
14 years ago
http://codex.wordpress.org/Switching_to_PHP5
Indeed. That's awesome! Let me switch gears then...
#9
@
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.
Thoughts?
#10
@
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
@
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.
#12
@
14 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
@
14 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']) : '';
#20
@
13 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.
This ticket was mentioned in IRC in #wordpress-dev by johnbillion. View the logs.
11 years ago
#27
@
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
@
9 years ago
- Milestone set to Awaiting Review
- Resolution maybelater deleted
- Status changed from closed to reopened
#29
@
9 years ago
I'm actually working on a proposal right now. Patch will be coming before the end of the day.
#30
@
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.
#31
@
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
@
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
@
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.
#34
@
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
#37
in reply to:
↑ 36
@
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 :-)
+∞