Make WordPress Core

Opened 13 years ago

Closed 2 years ago

Last modified 2 years ago

#19898 closed enhancement (fixed)

Create an is_login() function similar to is_admin()

Reported by: dcowgill's profile dcowgill Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.1 Priority: normal
Severity: normal Version: 3.3
Component: Login and Registration Keywords: has-patch 2nd-opinion has-dev-note
Focuses: Cc:

Description

It would be useful for developers to have better detection of being on the wp-login.php and wp-register.php pages. Sure, this can currently be done by using the $pagenow global variable but having a similar function like is_admin() would make things easier/cleaner.

One use case:

-Using a hosted javascript file but then requiring SSL (FORCE_SSL_LOGIN).

function javascript_init() {
    if ( $pagenow == 'wp-login.php' || is_admin() )
        return;

    wp_deregister_script( 'jquery' );
    wp_register_script( 'jquery', 'http://ajax.googleapis.com/ajax/libs/jquery/1.6/jquery.min.js');
    wp_enqueue_script( 'jquery' );
}
add_action( 'wp_print_scripts', 'javascript_init' );

Proposed solution:
Create a new function that does all the checking.

function is_login() {
    return in_array( $GLOBALS['pagenow'], array( 'wp-login.php', 'wp-register.php' ) );
}

Attachments (6)

19898.patch (530 bytes) - added by dcowgill 12 years ago.
19898.2.diff (745 bytes) - added by scribu 12 years ago.
19898.3.diff (2.9 KB) - added by scribu 12 years ago.
19898.4.diff (2.9 KB) - added by scribu 12 years ago.
19898.5.diff (924 bytes) - added by scribu 12 years ago.
19898.6.diff (662 bytes) - added by donmhico 5 years ago.

Download all attachments as: .zip

Change History (56)

#1 @husobj
12 years ago

  • Cc ben@… added

#2 @stephenh1988
12 years ago

  • Cc contact@… added

#3 @scribu
12 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 3.5

#4 @scribu
12 years ago

The login pages are a grey area; you're not in wp-admin, but you're not on the front-end either.

#5 @wonderboymusic
12 years ago

  • Keywords has-patch added; needs-patch removed

The attached patch does exactly what WP_ADMIN does for WP_LOGIN - adds is_login() which will be true for any page rendered by wp-login.php or any action handled by it

@dcowgill
12 years ago

#6 follow-up: @nacin
12 years ago

  • Keywords 2nd-opinion added

I feel like the name is_login() could cause confusion in that it could be referring to the current user (i.e. what is_user_logged_in() is for). This is already rather confusing with is_admin(), given is_super_admin() and a number of other functions (is_blog_admin, is_user_admin, is_site_admin).

wp-register.php no longer exists (and for older installs, it simply redirects to wp-login.php). That means that is_login() simply returns $GLOBALS['pagenow'] == 'wp-login.php, which is not really much of a shortcut. Doesn't seem all that needed, in that case.

#7 @nacin
12 years ago

  • Keywords close added

#8 @nacin
12 years ago

  • Milestone changed from 3.5 to Future Release

#9 in reply to: ↑ 6 @scribu
12 years ago

  • Keywords close removed

Replying to nacin:

That means that is_login() simply returns $GLOBALS['pagenow'] == 'wp-login.php, which is not really much of a shortcut. Doesn't seem all that needed, in that case.

It would be more useful if it had a filter, which could be used for defining custom login areas for themes.

@scribu
12 years ago

#10 follow-up: @scribu
12 years ago

Actually, we don't even need a new filter. is_login() can just compare wp_login_url() agains the current URL (which I really think deserves a get_current_url() helper, btw).

Behold: 19898.2.diff

@scribu
12 years ago

#11 @scribu
12 years ago

  • Keywords needs-unit-tests added; 2nd-opinion removed

19898.3.diff:

  • renames is_login() to is_login_page() and introduces is_register_page()
  • adds necessary get_current_url() and wp_register_url() - see #17950

@scribu
12 years ago

#12 @meloniq
12 years ago

  • Cc meloniq@… added

#13 @nacin
12 years ago

While get_current_url() might be a nice-to-have, it isn't needed here — remove_query_arg( array( 'action', 'redirect_to', 'loggedout' ) will automatically act against the current URL.

#14 follow-up: @scribu
12 years ago

  • Keywords needs-unit-tests removed

To be exact, remove_query_arg() returns the current URL, sans the domain.

Either way, remove_query_arg() isn't reliable for is_register_page(): ?action=register.

It turns out, we can just use did_action( 'login_form_login' ) and did_action( 'login_form_register' ).

The only hitch is wp-signup.php.

@scribu
12 years ago

#15 @scribu
12 years ago

  • Keywords 2nd-opinion added

There: 19898.5.diff

At this point, we're back to comment 6.

#16 @robmiller
12 years ago

#22852 was marked as a duplicate.

#17 in reply to: ↑ 10 @nacin
12 years ago

Replying to scribu:

Actually, we don't even need a new filter. is_login() can just compare wp_login_url() agains the current URL (which I really think deserves a get_current_url() helper, btw).

is_admin() is frequently used to decide whether we are in the admin or not for purposes of, say, loading code. Allowing is_login() to be true for a theme page doesn't really make sense, nor does it solve that wp-login.php is in no man's land. It would be better to offer a login.php template (along with signup.php, activate.php, register.php, and probably other login actions) if this was about themes.

The original use case can be solved by either checking is_ssl() or just using a protocol-relative URL (//), FWIW.

#18 @wonderboymusic
12 years ago

I currently add Apache rewrites to allow different theme'd login pages - so +1 on the theme vibe

#19 @scribu
12 years ago

A login.php template would be great, but wouldn't you need a conditional flag for template-redirect.php then anyway?

And also, for example, themes might want to do something like this in header.php:

if ( is_login() ) {
  // enqueue some custom js 
}

#20 @toscho
12 years ago

  • Cc info@… added

#21 in reply to: ↑ 14 @kitchin
12 years ago

The proposed patch Attachment 19898.5.diff​ does not work when an add_action('init', ...) callback needs to detect wp-login. In that case @$GLOBALS['pagenow'] == 'wp-login.php' does work.

My particular situation is that a plugin author used add_action('init', ...) to call session_start(). The session creation is failing due to file errors on the server, and I am disabling sessions on the admin side to debug.

Replying to scribu:

It turns out, we can just use did_action( 'login_form_login' ) and did_action( 'login_form_register' ).

#22 @chriscct7
9 years ago

  • Keywords needs-refresh added; has-patch 2nd-opinion removed
  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from new to closed
  • Version changed from 3.3.1 to 3.3

So no activity has happened on this ticket for 2+ years now. The latest patch doesn't really save anyone who really wanted to use a function like this that much code, and as pointed out there's issues in the current patch. Closing as wontfix

#23 @SergeyBiryukov
8 years ago

#37483 was marked as a duplicate.

#24 follow-up: @tazotodua
8 years ago

My topic (https://core.trac.wordpress.org/ticket/37620) was closed marked as "DUPLICATE" of this topic.

However, MY REQUEST IS COMPLETELY DIFFERENT!

WHAT ON EARTH, if you add
define('LOGIN_PAGE_EXECUTED', true);
or something like that in wp-login.php

why dont you make a great help to programmers?

we cant determine when login page is executed (when we output login page on other page then wp-login.php url, like many security plugins do that)

Version 0, edited 8 years ago by tazotodua (next)

#25 in reply to: ↑ 24 @SergeyBiryukov
8 years ago

Replying to tazotodua:

My topic (#37620) was closed marked as "DUPLICATE" of this topic.

That ticket is still open. That said, it's unlikely a new constant will be introduced here. Constants are not a great way to check for context, as they are not filterable and hard to use in unit tests, see the discussion in #25669.

#37483 was closed, because the IS_LOGIN_PAGE constant proposed there does seem similar to the is_login() function proposed here. Why is it "completely different"?

#26 follow-ups: @iandunn
5 years ago

  • Keywords 2nd-opinion added
  • Resolution wontfix deleted
  • Status changed from closed to reopened

I think this is worth reconsidering.

is_login() simply returns $GLOBALSpagenow? == 'wp-login.php, which is not really much of a shortcut

latest patch doesn't really save anyone who really wanted to use a function like this that much code

It doesn't save a lot of lines of code, but what it does save is a lot of time spent investigating the most reliable way to solve this problem.

As the various patches and WPSE answers demonstrate, there are a handful of ways that devs have tried to solve this problem, and many of them are buggy. I personally just spent an hour of research and testing before settling on a solution I'm happy with, and it's still possible that I missed some edge cases.

It'd save devs a lot of time and bugs if Core provided a reliable solution.

we can just use did_action( 'login_form_login' )

I think the flaw with 19898.5.diff is that login_form_{$action} executes after plugins have loaded, so it wouldn't solve the use case of a bootstraper that's trying to load code exclusively on the login screen.

I would do this instead:

function is_login_screen() {
	return false !== stripos( wp_login_url(), $_SERVER['SCRIPT_NAME'] );
}

#27 in reply to: ↑ 26 @SergeyBiryukov
5 years ago

  • Milestone set to 5.3

Replying to iandunn:

I would do this instead:

function is_login_screen() {
	return false !== stripos( wp_login_url(), $_SERVER['SCRIPT_NAME'] );
}

Makes sense to me.

@donmhico
5 years ago

#28 in reply to: ↑ 26 @donmhico
5 years ago

  • Keywords has-patch added; needs-refresh removed

Replying to iandunn:

I would do this instead:

function is_login_screen() {
	return false !== stripos( wp_login_url(), $_SERVER['SCRIPT_NAME'] );
}

Looks good to me too. I created a patch, 19898.6.diff , using @iandunn's solution. I inserted the function in wp-includes/load.php as other similar functions such as is_admin() resides there,

#29 @SergeyBiryukov
5 years ago

  • Owner set to SergeyBiryukov
  • Status changed from reopened to reviewing

#30 @davidbaumwald
5 years ago

  • Milestone changed from 5.3 to Future Release

With version 5.3 Beta 1 releasing shortly, the deadline for enhancements is now passed. This is being moved to Future Release. @SergeyBiryukov if you feel this can be included in 5.4, feel free to move up the milestone.

#31 @johnbillion
4 years ago

@iandunn Is this something you're interested in picking up again?

#32 @iandunn
4 years ago

I still think it's worth doing, but not something I personally have the bandwidth for right now.

#33 @SergeyBiryukov
2 years ago

  • Component changed from Administration to Login and Registration
  • Milestone changed from Future Release to 6.1
  • Summary changed from Create a is_login() function similar to is_admin() to Create an is_login_screen() function similar to is_admin()

#34 @SergeyBiryukov
2 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 53884:

Login and Registration: Introduce is_login_screen() function.

This should help determine whether the current request is for the login screen.

While it does not save a lot of lines of code, including this function in core aims to save developers some time that would otherwise be spent investigating the most reliable way to solve this problem.

Implementation details:

  • By checking wp_login_url(), the function accounts for custom login locations set via the login_url filter.
  • By checking $_SERVER['SCRIPT_NAME'] directly, instead of did_action( 'login_form_login' ) or $pagenow global, the function can work as early as possible, for example in a must-use plugin.

Follow-up to [2481], [6412], [12393], [12732], [15558], [15481], [15746].

Props dcowgill, scribu, donmhico, iandunn, wonderboymusic, nacin, robmiller, kitchin, chriscct7, tazotodua, davidbaumwald, SergeyBiryukov.
Fixes #19898.

#35 @SergeyBiryukov
2 years ago

  • Keywords needs-dev-note added; 2nd-opinion removed

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


2 years ago

#38 follow-ups: @azaozz
2 years ago

  • Keywords 2nd-opinion added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Going to reopen this as it seems the is_login_screen() function name may be a bit confusing in some cases and perhaps can be better. Note that this is only for reconsidering part of the name, can be fixed during beta.

Would the function name be better as is_login() or maybe is_login_request(), perhaps?

What the screen suffix means doesn't seem particularly clear imho. In plain language the return value of that function means that:

  1. The request is from an user that has not been authenticated.
  2. The request may be a redirect from another wp-admin page.
  3. The WordPress login screen will likely be outputted as a result of this request. However if the request was from something like the WP Login widget, or from AJAX, the screen will not be outputted.

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


2 years ago

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


2 years ago

#41 @SergeyBiryukov
2 years ago

Just noting that this is also being discussed in comment:8:ticket:56400 and the naming of this new function may depend on the decisions there.

#42 in reply to: ↑ 38 @SergeyBiryukov
2 years ago

Replying to azaozz:

Would the function name be better as is_login() or maybe is_login_request(), perhaps?

Thanks for flagging this!

With the considerations raised here and in comment:8:ticket:56400, I agree is_login_request() makes more sense. Uploaded a new patch on #56400.

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


2 years ago

#44 @SergeyBiryukov
2 years ago

Some further naming considerations: comment:22:ticket:56400.

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


2 years ago

#47 in reply to: ↑ 38 ; follow-up: @SergeyBiryukov
2 years ago

Replying to azaozz:

Would the function name be better as is_login() or maybe is_login_request(), perhaps?

My concern with is_login_request() is that it could be mistaken for a request that does the actual authentication, i.e. wp_signon(). So I think I'm leaning towards is_login() here.

#48 in reply to: ↑ 47 @azaozz
2 years ago

Replying to SergeyBiryukov:

I think I'm leaning towards is_login() here.

+1, sounds good.

#49 @SergeyBiryukov
2 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 54447:

Login and Registration: Rename is_login_screen() function to is_login().

As the function can be used in a variety of contexts, the _screen suffix may not always be appropriate.

This commit aims to reduce confusion by renaming the newly added is_login_screen() function to is_login(), which better aligns with is_admin() and the related is_*_admin() function family.

While it does not save a lot of lines of code, this function aims to save developers some time that would otherwise be spent investigating the most reliable way to determine whether the current request is for the login screen.

Implementation details:

  • By checking wp_login_url(), the function accounts for custom login locations set via the login_url filter.
  • By checking $_SERVER['SCRIPT_NAME'] directly, instead of did_action( 'login_form_login' ) or $pagenow global, the function can work as early as possible, for example in a must-use plugin.

Follow-up to [53884].

Props azaozz.
Fixes #19898. See #56400.

#50 @SergeyBiryukov
2 years ago

  • Summary changed from Create an is_login_screen() function similar to is_admin() to Create an is_login() function similar to is_admin()
Note: See TracTickets for help on using tickets.