Problem

At this moment we have two implementation for the same functionality: user_login and user_login_block
As I see there is no reason for that as they are mostly the same.

Solution

1. Removed user_login_block function
2. Removed redirect also mentioned in #1168514: Remove drupal_goto from user_login()
3. Links got named classes for better theming
4. Move links under "Log in" button:
login-user.png ===>> login-user.png

CommentFileSizeAuthor
#58 1772584-user_login_form-58.patch15.74 KBgumanist
#56 1772584-user_login_form-56.patch15.92 KBgumanist
#55 1772584-user_login_form-55.patch15.81 KBandypost
#55 1772584-interdiff-52vs55.txt2.52 KBandypost
#52 1772584-user_login_form-52.patch14.94 KBgumanist
#44 1772584-user_login_form-44.patch15.28 KBandypost
#44 1772584-interdiff-44.txt2.88 KBandypost
#41 1772584-user_login_form-41.patch15.76 KBandypost
#41 1772584-interdiff-41.txt18.06 KBandypost
#39 core-login_block_openid-1772584-38.patch18.47 KBnod_
#37 1772584-user_login_form-36.patch15.87 KBandypost
#37 1772584-interdiff-36.txt11.85 KBandypost
#35 1772584-user_login-35-do-not-test.patch7.06 KBandypost
#34 1772584-user_login-34-do-not-test.patch7.21 KBandypost
#34 1772584-interdiff-34.txt3.39 KBandypost
#26 login-user.png6.59 KBandypost
#26 login-openid.png6.45 KBandypost
#25 1772584-core-user_login-25.patch6.94 KBandypost
#25 1772584-interdiff-21vs25.txt4.46 KBandypost
#25 login-user.png6.59 KBandypost
#25 login-openid.png6.64 KBandypost
#21 1772584-core-user_login-21.patch5.36 KBandypost
#21 1772584-interdiff.txt4.45 KBandypost
#16 1772584-core-user_login-16.patch5.72 KBgumanist
#15 1772584-core-user_login-15.patch5.51 KBandypost
#15 1772584-core-user_login-11vs15.interdiff.txt1.27 KBandypost
#11 1772584-core-user_login-11.patch6.24 KBandypost
#11 1772584-interdiff-8vs11.txt5.4 KBandypost
#8 replace-user_login_block-with-user_login-8.patch6.19 KBgumanist
#6 replace-user_login_block-with-user_login-6.patch5.38 KBgumanist
#3 replace-user_login_block-with-user_login-2.patch4.22 KBgumanist
replace-user_login_block-with-user_login.patch4.2 KBgumanist
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, replace-user_login_block-with-user_login.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review

+1 to unify login form!

+++ b/core/modules/user/user.moduleundefined
@@ -1567,34 +1537,35 @@ function user_set_authmaps($account, $authmaps) {
-    '#attributes' => array(
-      'autofocus' => 'autofocus',
-    ),

suppose this part should stay as is.

gumanist’s picture

re-rolled patch

gumanist’s picture

This was removed to avoid conflicts with other forms. For example search page needs to have active search box, not login

Status: Needs review » Needs work

The last submitted patch, replace-user_login_block-with-user_login-2.patch, failed testing.

gumanist’s picture

Status: Needs work » Needs review
FileSize
5.38 KB

Updated test SimpleTest's BrowserTest test case with different URL

andypost’s picture

Status: Needs review » Needs work
+++ b/core/modules/user/user.moduleundefined
@@ -1567,34 +1537,36 @@ function user_set_authmaps($account, $authmaps) {
+  // Adding action links.
+  if (config('user.settings')->get('register') != USER_REGISTER_ADMINISTRATORS_ONLY) {
+    $items[] = l(t('Create new account'), 'user/register', array('attributes' => array('title' => t('Create a new user account.'))));

Suppose this links should be displayed conditionally. To do so just add a parameter for form builder

gumanist’s picture

Status: Needs work » Needs review
FileSize
6.19 KB

Added settings for #2 and #7 + some reordering in function.

podarok’s picture

#8 looks good for me
RTBC ?

andypost’s picture

Status: Needs review » Needs work
+++ b/core/modules/simpletest/lib/Drupal/simpletest/Tests/BrowserTest.phpundefined
@@ -30,14 +30,14 @@ class BrowserTest extends WebTestBase {
-    $url = 'user/login';
+    $url = 'user/password';
 
     $this->drupalGet($url);
     $absolute = url($url, array('absolute' => TRUE));
     $this->assertEqual($absolute, $this->url, t('Passed and requested URL are equal.'));
     $this->assertEqual($this->url, $this->getAbsoluteUrl($this->url), t('Requested and returned absolute URL are equal.'));
 
-    $this->drupalPost(NULL, array(), t('Log in'));
+    $this->drupalPost(NULL, array(), t('E-mail new password'));

mysterious hunk

+++ b/core/modules/user/user.moduleundefined
@@ -839,7 +809,7 @@ function user_block_view($delta = '') {
-        $block['content'] = drupal_get_form('user_login_block');
+        $block['content'] = drupal_get_form('user_login');

is there no other way to return a render-array - see http://drupal.org/node/1777324

+++ b/core/modules/user/user.moduleundefined
@@ -1563,38 +1533,59 @@ function user_set_authmaps($account, $authmaps) {
+  $settings += array(
+    'display_links' => TRUE,
+    'autofocus' => FALSE,
+  );

Should they be moved to user_block_configure()

andypost’s picture

Status: Needs work » Needs review
FileSize
5.4 KB
6.24 KB

Settings for form are moved to block settings, defaults are mimic current core's
Test now is not affected.
Parallel issue which tries to kill the same redirect for form #1168514-6: Remove drupal_goto from user_login()

gumanist’s picture

Looks good, thank you for rerolling patch. Should we mark it as RTBC?

podarok’s picture

Status: Needs review » Reviewed & tested by the community

#11 looks good!
thanks for reroll

catch’s picture

Status: Reviewed & tested by the community » Needs work

Cleaning this up is great, but why this?

+    case 'login':
+      $form['user_block_display_links'] = array(
+        '#type' => 'checkbox',
+        '#title' => t('Display links'),
+        '#default_value' => variable_get('user_block_display_links', TRUE),
+      );
+      $form['user_block_display_links'] = array(
+        '#type' => 'checkbox',
+        '#title' => t('Auto-focus on login'),
+        '#default_value' => variable_get('user_block_autofocus', FALSE),
+      );
+      return $form;

This feature is something that can be easily done by contrib or custom code, I don't see that it's necessary to make these optional in core, or in general to add new admin interface elements here.

andypost’s picture

Status: Needs work » Needs review
FileSize
1.27 KB
5.51 KB

@catch this settings should be added as follow-up patch. I think it could be a good addition for new block-plugins. So here's a patch without this

gumanist’s picture

Added names to links array to allow clean altering
Added classes to links to have styling possibility

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Suppose this small enhancement should pass a gates

+++ b/core/modules/user/user.moduleundefined
@@ -1547,38 +1521,73 @@ function user_set_authmaps($account, $authmaps) {
+      $items['create_account'] = l(t('Create new account'), 'user/register', array(
+        'attributes' => array(
+          'title' => t('Create a new user account.'),
+          'class' => array('create-account-link'),
+        ),
+      ));
+    }
+    $items['request_password'] = l(t('Request new password'), 'user/password', array(
+      'attributes' => array(
+        'title' => t('Request new password via e-mail.'),
+        'class' => array('request-password-link'),

just adds a named class on this links! This should really improve styling out-of-box

catch’s picture

Status: Reviewed & tested by the community » Needs work

Sorry now there's settings without a UI:

+ * @param $settings
+ *   (optional) additional settings for form builder:
+ *   - display_links: display additional links or not. Default FALSE;
+ *   - autofocus: should this form get focus on page. Default TRUE;
+ *   - set_destination: set action URL to value of drupal_get_destination().
+ *     Default FALSE.

Can we remove all this too and just keep the functionality the same as now?

andypost’s picture

Status: Needs work » Needs review

@catch no, this settings are required to configure a form:
1) display or not a links
2) set destination of form submit (probably could be removed) see also #1168514: Remove drupal_goto from user_login()
3) autofocus on user name is required for login page

catch’s picture

@andypost - we don't have those settings now, so we don't need to add them in a refactoring issue. I'm OK with them being discussed/reviewed in a follow-up but as the patch stands it's adding additional code for something we don't even know if we want.

andypost’s picture

Thanx @catch for idea of form customization

1) Login form is customized within user_block_view()
2) Links are moved to separate renderable, and gets classes with theme_links()

diffstat 1772584-core-user_login-21.patch
 openid/openid.module |   11 ------
 user/user.module     |   85 +++++++++++++++++++++++++--------------------------
 2 files changed, 43 insertions(+), 53 deletions(-)
gumanist’s picture

From my point of view it would be better to have most common items in place and it is up to user to use them or no. It is much easier to explain that get form with that parameters will lead you to that results.
For login it is:
User + password inputs and password reset + user register links.

But it is fine by me to move those updates as a follow-up issue. Let's mark it as RTBC?

podarok’s picture

Status: Needs review » Reviewed & tested by the community

agree with follow-ups
#21 looks good for me

sun’s picture

Title: Replace user_login_block with user_login » Replace user_login_block() with user_login()
Status: Reviewed & tested by the community » Needs work

Needs to be re-rolled due to #1538462: Cannot log in with OpenID due to "required" attribute

This could also use some more architectural reviews before it gets back to RTBC again.

I'd also like to see a follow-up issue to rename user_login() into user_login_form().

andypost’s picture

Status: Needs work » Needs review
Issue tags: +Needs usability review
FileSize
6.64 KB
6.59 KB
4.46 KB
6.94 KB

Merged conflicts and changed back a way like links rendered

This change require usability review because I think that links (create account ...) should be placed after form - form inputs are grouped with submit button and links just additional fuctionality

Screens here
login-user.pnglogin-openid.png

EDIT Yes, we need a follow-up to rename user_login() to user_login_form()

andypost’s picture

FileSize
6.45 KB
6.59 KB

Currently it looks like

login-user.pnglogin-openid.png

Bojhan’s picture

Issue tags: -Needs usability review

Interesting, I think andy is right. It is a little weird that the closest action to two forms, is to create a new account. It should be the form.

andypost’s picture

Also I think that login form is important part of UI so let's get accessibility team here to!

Maybe this issue should be renamed to "Replace user_login_block() & user_login() with user_login_form()" ?

andypost’s picture

Issue tags: +Usability, +Accessibility

tagging

Everett Zufelt’s picture

From what I understand this issue doesn't change the UI or functionality, just refactors code (didn't look at patch). If so, then there are no concerns from an accessibility perspective. If UI or functionality changes exist please update issue title.

andypost’s picture

Issue summary: View changes

Updated issue summary.

andypost’s picture

@Everett Zufelt, Patch changes a login form - moves links under "Log in" button and changes a style of links

EDIT: probably this could affect this patch #1591806: Change block "subject" so that it's called a (admin_)label like everything else on the theme layer

Everett Zufelt’s picture

Status: Needs review » Needs work

Needs work for clear title and updated summary.

I don't understand what move links under button means, Can a more precise description of the change please be given?

andypost’s picture

Status: Needs work » Needs review
Issue tags: -Needs issue summary update

I've update summary (4) and added screenshots

andypost’s picture

Issue summary: View changes

Updated issue summary.

andypost’s picture

Patch with moving links out of form

andypost’s picture

Another round

andypost’s picture

Title: Replace user_login_block() with user_login() » Get rid of user_login_block() & user_login() in favour of user_login_form()
Status: Needs review » Needs work

@chx suggest to make rename right in the issue

andypost’s picture

Status: Needs work » Needs review
FileSize
11.85 KB
15.87 KB

Patch for bot, let's see how many tests got broken

Status: Needs review » Needs work

The last submitted patch, 1772584-user_login_form-36.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
FileSize
18.47 KB

Made the JS work with a single set of links below the forms.

Status: Needs review » Needs work

The last submitted patch, core-login_block_openid-1772584-38.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
18.06 KB
15.76 KB

Added @nod_ js and fixed tests

Status: Needs review » Needs work
Issue tags: -Usability, -Accessibility, -Needs accessibility review

The last submitted patch, 1772584-user_login_form-41.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
Issue tags: +Usability, +Accessibility, +Needs accessibility review

#41: 1772584-user_login_form-41.patch queued for re-testing.

andypost’s picture

Patch returns back #openid-login hash navigation
@nod_ please review the changes...

andypost’s picture

@nod_ probably new openid-link require some styling because there's no href in link {cursor: pointer}

nod_’s picture

Status: Needs work » Needs review

no, no we don't need this hash thing, it's horrible. It's just because you can't make a link from Drupal with an empty href (and that's legit in html5) since we're now creating it from JS there is no need for it.

The css needs some work to make it look like before, but the code shouldn't put the hash thing back.

And well, I've already said it, this link should be a button to begin with.

nod_’s picture

Status: Needs review » Needs work
sun’s picture

Status: Needs review » Needs work

The fragment/hash usage actually looks interesting here. It means that you can carefully prepare and craft a link that goes directly to the OpenID login form instead of the regular; i.e., /user/login#openid vs. /user/login#user (the latter being equivalent to no fragment/hash; would be great if the name would match the "provider", so #user instead of #nogo).

nod_’s picture

Isn't that why we have a user/login/openid ? The use case seems pretty convoluted.

andypost’s picture

@nod_ so was there any reason to introduce this hash tags in openid patch? #1538462-39: Cannot log in with OpenID due to "required" attribute
I just trying to make no regression with current functionality.
The css code was already adjusted to make no visual regression - cos now we use the same link for show/hide forms the cancel link inherits openid-icon
Another question here - should we make register/request password links use theme_links() as I made in #21?

+++ b/core/modules/user/user.moduleundefined
@@ -836,8 +806,41 @@ function user_block_view($delta = '') {
+          'links' => array(
+            '#theme' => 'links',
+            '#links' => $links,

like this

nod_’s picture

The hash thing was introduced in #160163: Login with OpenID link has mis-encoding of # a year ago without a solid supporting use-case.

gumanist’s picture

- Link for OpenID moved to links list
- Updated JavaScript
- Removed theming on frontend side

gumanist’s picture

Status: Needs work » Needs review

for bot

andypost’s picture

Going to make manual testing

+++ b/core/modules/user/user.moduleundefined
@@ -841,8 +806,40 @@ function user_block_view($delta = '') {
+            '#weight' => 20,

I think we could skip this in block_user_view() and set in openid

EDIT RTL styles needs to be ajusted too

andypost’s picture

Changes:
- fixed RTL-styling
- moved #weight for links to openid
- removed unused $errors variable in JS
- add focusing on openID input after switching

gumanist’s picture

re-rolled, because of
http://drupal.org/node/1168514

Status: Needs review » Needs work

The last submitted patch, 1772584-user_login_form-56.patch, failed testing.

gumanist’s picture

Status: Needs work » Needs review
FileSize
15.74 KB

Re-rolled again

andypost’s picture

Status: Needs review » Reviewed & tested by the community

I think it's ready, let's get this in

podarok’s picture

#58 +1 RTBC

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Hm. There's an awful lot going on in this patch for such a simple-sounding title. I checked it over and the non-JS parts seem fine (I'm not sure how to evaluate the JS parts), and this has been sitting at RTBC for a few days now though, and I haven't seen anyone pipe up about it, so...

Committed and pushed to 8.x. Thanks!

EclipseGc’s picture

Follow up issue to this to clean up how it was implemented: #1826452: Do not alter forms after calling drupal_get_form()

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.