Login form template function

[ryan]

Create a standard login_form() function.

[beaulebens]

If this function was called, and the user was already logged in, what would be displayed? I see a few options:

• Show the form again anyway
• Show a message like "Logged in as: [display_name]"
• Return an empty string/show nothing

I guess we should try to handle all of them, and make it take args to handle it all: echo, before, after, logged_in_msg (e.g. "Logged in as: %s", false to show form either way, null to show nothing if logged in?)

I'm happy to tackle this one if we can get some consensus on this case.

[Denis-de-Bernardy]

Replying to beaulebens:

If this function was called, and the user was already logged in, what would be displayed? I see a few options:

I'd add another: redirect to the user's profile page if it's called from the login page.

[beaulebens]

Very simple version of this, which does *not* override the wp-login.php form. It's for use "everywhere else". Don't know if this is what we want to do. Pretty basic HTML output with some filters. Open to suggestions on what other filters/actions should be in here.

[aaroncampbell]

I love it. The only thing I'd like to see added is a filter and or a couple actions. I'd like to see actions put after the opening form tag and before the remember me stuff (maybe also just before the closing form tag).

Also a filter on the entire $form as it's returned (this could be enough, but having the actions would be cleaner ways of adding additional content). They could be used to add additional graphics or text that have to do with the login, or even additional fields in certain situations.

[aaroncampbell]

beaulebens patch with actions and filters

[aaroncampbell]

The patch I uploaded adds 3 filters (login_form_redirect, login_form_remember, and login_form) and 3 actions (login_form_top, login_form_middle, and login_form_bottom) to the already existing 4 filters (login_form_username, login_form_password, login_form_remember_me, and login_form_log_in).

It might be a little excessive, but I think you get the idea.

[filosofo]

I think having a pluggable login form is a good idea, but I think we need to put some more work into this implementation.

• Usually if a WP function returns a string rather than printing it, it's named something like /function (wp_)?get.*/
• We should decouple the test for whether someone is logged in from the logic that generates the form markup. It would be maddening to call a function that's supposed to generate form markup and have it return an empty string, because it decides based on global variables that you don't actually need the form markup you requested.
• All of those id attributes should be at least filterable. Even better, just give the form itself an id and remove all the others (let classes handle styling).
• Use a filter instead of a ternary operator for the "remember me" bit.
• Better yet, just let all of that stuff---ids, whether to show the remember me checkbox, etc., be set by an array of default arguments. Then you can have just one, final filter for everything.
• Use more descriptively precise filter names. For example, 'login_form_username' is not very helpful here, because it filters the label, not the username itself.
• Have more semantically-rich class names for the form elements' wrappers. E.g. <div class="form-element username-wrapper"> or something like that. Then you're freed to do more with the styling.

• Shouldn't the username value and remember-me checkbox have the possibility of being set?
• This looks like it's ripe for a security problem:

  $redirect = ( is_ssl() ? 'https://' : 'http://' ) . $_SERVER['HTTP_HOST'] . $_SERVER['REQUEST_URI'];
  

[hakre]

And the various SSL options need to be properly incorporated: #9207. I do not see how the current suggestion provides the default functionality of an SSL login.

[beaulebens]

Thanks for your feedback everyone. I've attached a new patch (11172.003.diff) which does the following:

• Moves everything to an args array, which is filtered, and overrideable when the function is called. Makes most ids, labels etc filterable in one place.
• Added in the 3 actions (top/middle/bottom) suggested by aaroncampbell
• Removed the vulnerability when using HTTP_HOST, don't know if REQUEST_URI could still be an issue as it's passed through site_url()?
• Removed the logged in test, so developers should do that themselves outside the function
• Added some classes to the P tags to help isolate individual elements via CSS

Also, @hakre, I believe the SSL options are all handled by using site_url() to generate URLs, but please correct me if I'm wrong.

Hopefully this is a step forward. Once people are happy with this, I'll use some of the same ideas over on #10910 as well.

[nacin]

This looks very promising.

I would like to see a wp_login_form() function that echoes wp_get_login_form(), if we are to have both. With all of those hooks, I'm not sure wp_get_login_form() is necessary at all.

I'm wondering why we shouldn't replace the form in wp-login.php with this as well? Eating our own dog food would make the most sense.

And would it make sense for this to be pluggable?

[filosofo]

Replying to nacin:

I would like to see a wp_login_form() function that echoes wp_get_login_form(), if we are to have both.

You have WP precedent behind your suggestion, but it's a silly precedent I wish we'd not perpetuate.

Why bloat the code with extra function definitions when any of the following could work just as well?

echo wp_get_login_form();
print wp_get_login_form();
wp_get_login_form(array('echo' => true));

[nacin]

Replying to filosofo:

Replying to nacin:

I would like to see a wp_login_form() function that echoes wp_get_login_form(), if we are to have both.

You have WP precedent behind your suggestion, but it's a silly precedent I wish we'd not perpetuate.

Why bloat the code with extra function definitions when any of the following could work just as well?

Part of that was me asking why we even need wp_get_login_form() at all. It sounds like a function that would pretty much always be echoed when used. At most, I would think it should be a wp_login_form() function that allows you to pass array('echo' => false), not a get_* function that supports echoing.

[scribu]

Replying to nacin:

Part of that was me asking why we even need wp_get_login_form() at all. It sounds like a function that would pretty much always be echoed when used. At most, I would think it should be a wp_login_form() function that allows you to pass array('echo' => false), not a get_* function that supports echoing.

+1

[filosofo]

Replying to nacin:

Part of that was me asking why we even need wp_get_login_form() at all. It sounds like a function that would pretty much always be echoed when used. At most, I would think it should be a wp_login_form() function that allows you to pass array('echo' => false), not a get_* function that supports echoing.

That's a reasonable compromise, but I think wp_get_login_form() gives the advantage of having a name that makes explicit its default behavior. How about wp_print_login_form()? There is naming precedent in a few functions such as wp_print_head_scripts()

[beaulebens]

Modified function name to wp_login_form() and added "echo" parameter, which defaults to true.

[ryan]

(In [12696]) wp_login_form(). Props beaulebens. see #11172

[dd32]

Version x.x are we :)

[westi]

(In [12701]) @since version for wp_login_form. See #11172 props dd32.

[markmcwilliams]

Would it be possible to add a is_user_logged_in() check, so that when the user is logged in, it still doesn't appear as if they need to login! Yes I know the user could do is_user_logged_in() with their template, but not everyone knows what they're doing, it's just a thought! :) Other than that, it works pretty well for me at the moment!

[beaulebens]

Replying to markmcwilliams:

Would it be possible to add a is_user_logged_in() check, so that when the user is logged in, it still doesn't appear as if they need to login! Yes I know the user could do is_user_logged_in() with their template, but not everyone knows what they're doing, it's just a thought! :) Other than that, it works pretty well for me at the moment!

Please see ​http://core.trac.wordpress.org/ticket/11172#comment:10 where the previous logged_in check was explicitly removed to avoid confusion/undesired effects.

[markmcwilliams]

Replying to beaulebens:

Please see ​http://core.trac.wordpress.org/ticket/11172#comment:10 where the previous logged_in check was explicitly removed to avoid confusion/undesired effects.

I'm not 100% sure how it removes confusion, I'd be more confused if it wasn't included, and like I said not everyone knows what to do to add this check in the actual theme template! - So I;d still say this needs to be included! :)

[filosofo]

Replying to markmcwilliams:

I'm not 100% sure how it removes confusion, I'd be more confused if it wasn't included, and like I said not everyone knows what to do to add this check in the actual theme template! - So I;d still say this needs to be included! :)

It's good programming practice as much as is possible not to have functions do multiple things, especially when the logic of what they're doing is based on global variables. When you fail to do that, you end up with functions that are difficult to debug and test and that are rife with backwards-compatibility issues.

Besides, don't you think it's somewhat ridiculous to write code for a "developer" who doesn't know what he's doing? What he needs is education not functionality. So educate him with good documentation and obvious function names, etc. But don't reduce the code itself to the lowest common denominator.

[nacin]

Everything that filosofo said, plus, sometimes you might want a login form when the user is already logged in. dd32 has written a plugin that adds a sudo layer to WordPress, for example.

[nacin]

Possible improvements: docblock changes, extracting args, and a catch-all filter.

[nacin]

Patch attached suggests the following:

• Clarifies some of the phpdoc and adds a note about checking whether the user is logged in.
• Extracts args to help the readability of the code.
• Introduces a wp_login_form filter. It looks like this was suggested early on but then omitted from later patches, and I don't see a reason stated in the ticket for why it shouldn't be there.

[nacin]

Also, esc_attr( $redirect ) should probably be esc_url( $redirect );

[ptahdunbar]

Took a stab at 11172.005.diff to improve on what you guys started. I took some ideas from comment_form().

Diff:

• prefixed all hooks with 'wp_' in addition to passing the $args variable.
  
• adds a new fields param as a second argument to add or replace form fields.
  
• hooks before, on, and after each field.
  
• changed 'login_form_top' to 'wp_login_form_before'
  
• changed 'login_form_bottom' to 'wp_login_form_after'
  
• removed login_form_middle'
  
• updated the docblock
  

[nacin]

I'm weary of having action hooks inside the formatting (as it is now, as well as in the patch). More or less, we don't want a plugin to hook in and echo directly -- we want output returned.

I wonder if we should eliminate the ability to return the HTML. We don't have the ability to return the output for comment_form() either. It makes plugin manipulation that much easier, and frankly there aren't many use cases for receiving a blob of HTML.

My very old patch and ptah's patch should be merged and massaged, and we should see what else needs to be improved here. If someone wants to pick this up, please do so, otherwise I will circle back around.

[ryan]

I'm for either changing those actions to be apply_filters with an empty string, or removing return. Regexing blobs of html is not very useful or future proof. At this point, converting to apply_filters is probably easiest though.

[ryan]

(In [14804]) Use filters instead of action in wp_login_form(). see #11172

[westi]

This looks like a suitable compromise.

Maybe we can revisit this later and add improvements then.

Marking as fixed for 3.0