Cannot log in with OpenID due to "required" attribute

+++ b/core/modules/openid/openid.module
@@ -134,12 +134,13 @@ function openid_form_user_login_alter(&$form, &$form_state) {
 function _openid_user_login_form_alter(&$form, &$form_state) {
...
+  $form['name']['#required'] = FALSE;
+  $form['pass']['#required'] = FALSE;

Hm. This means that the login form is always altered and required validation is disabled both on the server-side as well as on the client-side.

I'm concerned about unexpected security consequences of this.

I.e., what happens if you submit the login form with no values at all -- are we sure that all code is smart enough to handle that case? Likewise, what happens if you only submit a password but no username? [...]

Lastly, an OpenID module test should have prevented the HTML5 required patch from landing in the first place. ;)

I came across this looking at some other things in OpenID and this patch fixed the problem. I've clicked through all of the OpenID functionality and think this patch takes care of it all.

I don't fully understand all the changes to states.js though, so someone may need to look over that before committing.

Sorry, this needs more reviews.

The changes to states.js look a bit spooky to me as well. Don't have time to review them in detail right now. Perhaps @nod_ or @xjm or someone else familiar with the states code can look into them.

Also, these are massive UI and CSS changes - needs manual testing.

Let's also look in the html5 folks to make 'em aware of this gotcha.

When adding #required we were using the 'formnovalidate' attribute to work with #states. We could do the same thing here. That keeps all the server side validation and the required attribute itself (for screenreaders, etc.) ...

Edit: Reading more of the issue: Refactoring the whole thing could be a seperate issue. @c960657's patch would probably be a good start.

#10: openid-required-4.patch queued for re-testing.

We'll reroll this and work on testing it during MWDS.

To test the #states behaviors:

1. Download the #states exercise module to your 8.x installation's sites/all/modules directory and install it.
   http://drupal.org/project/1269964/git-instructions
2. Note the existing bugs with the #states system:
   http://drupal.org/node/735528#comment-5473144
   http://drupal.org/node/735528#comment-5499236
3. Visit the path states-exercise in your local D8 install. (A menu link should appear in the Navigation menu.)
4. Test all the states in a given section without the patch applied. (Make sure to test all sequences/combinations).
5. Apply the patch, and clear your site and browser caches.
6. Re-test the states in your section and note any changes in the behavior.

We'll organize people to each test a certain section of the module during today's sprint.

@xjm, if you don't want to do it manually, consider extending the testswarm tests

Removing reroll tag.

Thanks for the patch and the rerolls, c960657!

I applied it locally and could confirm that the login block is fixed (in that you can login via OpenId after clicking the link to switch) and also that the seperate OpenId login page is working.

However I don't understand some parts in the code.

+++ b/core/misc/states.jsundefined
@@ -107,13 +107,25 @@ states.Dependent.prototype = {
+        var arg;
+        if (selector === 'window') {
+          arg = window;
+        }
+        else if (selector === 'document') {
+          arg = document;
+        }
+        else {
+          arg = selector;
+        }
+        var element = $(arg);

What is this about?

+++ b/core/modules/openid/openid.moduleundefined
@@ -16,6 +16,13 @@ function openid_menu() {
+  $items['user/login/openid'] = array(
+    'title' => 'Log in using OpenID',
+    'page callback' => 'drupal_get_form',
+    'page arguments' => array('openid_login_form'),
+    'access callback' => 'user_is_anonymous',
+    'type' => MENU_LOCAL_TASK,
+  );

And much more high level, to see if were heading in the right direction: Having a seperate page might indeed be cleaner. Not sure about the pros and cons. Tagging for usability review.

+++ b/core/modules/openid/openid.moduleundefined
@@ -41,7 +48,7 @@ function openid_menu() {
-  if ($menu_site_status == MENU_SITE_OFFLINE && user_is_anonymous() && $path == 'openid/authenticate') {
+  if ($menu_site_status == MENU_SITE_OFFLINE && user_is_anonymous() && in_array($path, array('openid/authenticate', 'user/login/openid'))) {

Not sure why we need this one. Was the equivalent available in offline mode before?

Thanks for explaining everything, @c960657! On to a more nitpicky round ;)

+++ b/core/misc/states.jsundefined
@@ -107,13 +107,25 @@ states.Dependent.prototype = {
+        var arg;

Maybe add a short inline comment stating your explanation here.

+++ b/core/modules/openid/lib/Drupal/openid/Tests/OpenIDTestBase.phpundefined
@@ -19,6 +19,7 @@ abstract class OpenIDTestBase extends WebTestBase {
+    $modules[] = 'node';
     $modules[] = 'openid';
     parent::setUp($modules);
 
@@ -38,6 +39,9 @@ abstract class OpenIDTestBase extends WebTestBase {

@@ -38,6 +39,9 @@ abstract class OpenIDTestBase extends WebTestBase {
       ))
       ->execute();
 
+    // Use a different front page than login page.
+    config('system.site')->set('page.front', 'node')->save();
+
     // Create Basic page and Article node types.
     if ($this->profile != 'standard') {
       $this->drupalCreateContentType(array('type' => 'page', 'name' => 'Basic page'));
@@ -51,7 +55,7 @@ abstract class OpenIDTestBase extends WebTestBase {

@@ -51,7 +55,7 @@ abstract class OpenIDTestBase extends WebTestBase {
   function submitLoginForm($identity) {
     // Fill out and submit the login form.
     $edit = array('openid_identifier' => $identity);
-    $this->drupalPost('', $edit, t('Log in'));
+    $this->drupalPost('', $edit, t('Log in'), array(), array(), 'openid-login-form');

Why are these changes nescessary? (To be 100% sure I understand your comment there correctly.)

+++ b/core/modules/openid/openid.moduleundefined
@@ -120,58 +127,77 @@ function openid_user_logout($account) {
+ * Form constructor for the OpenID login form.

We should have a @see reference to the submit handler.

+++ b/core/modules/user/user.moduleundefined
@@ -1036,33 +1036,22 @@ function user_account_form_validate($form, &$form_state) {
+function user_login_block() {

While were refactoring a large part of this, maybe add a paragraph of doxygen?

I continued testing manually, this time without JavaScript and block and form work fine. Yay!

The UI change is this:

(OpenID now on a seperate tab.) I think this looks pretty clean. Let's go with that, unless there are some terrible effects of this I am missing.

I'll try to ping @nod_ in IRC, because I am not too comfortable with the JavaScript parts. If no objections, then I'd say RTBC after the above changes.

I'm not really happy to add window and document thing to #states.

the # link should be changed to #nogo at least, it's annoying to be sent to the top when toggling. Why can't we use a button to do that? Buttons can live outside of forms.

It's two different forms i'm sure we can find something a bit better than was we used to do in D7.

The states API is a convenience, JS has events, that's what we should be using for this. it makes sense to use #states inside of a form, not so much outside of it. Adding window and document opens things up to messy horrible things people will want to come up with. And #states is still bugged adding that would just add problems. People will expect that "document, input[type='text']" will match. It wont.

the link doesn't actually goes anywhere, and with JS off, it simply doesn't show up. Clear case for a button and not a link.

localStorage can be used for storing the value, we'll be able to do neat things with that, not so much with pushState. localStorage is already used in tabledrag :D

Check out #1719640: Use 'button' element instead of empty links.

agreed for local storage. there is the same thing for the toolbar state. easy enough to fix somewhere else.

fair enough. I'll keep an eye on the different issues an follow up.

thanks.

Removing tag, bumping to critical.

Good to me.

I'd love to move forward here, and really want to say THANKS to @c960657 for keeping this alive!

This patch looks good to me. I only have two issues with it, which I hope can be addressed quickly:

+++ b/core/modules/user/user.module
@@ -733,33 +733,28 @@ function user_validate_current_pass(&$form, &$form_state) {
-function user_login_block($form) {
...
+function user_login_block() {

@@ -840,7 +835,7 @@ function user_block_view($delta = '') {
         $block['subject'] = t('User login');
-        $block['content'] = drupal_get_form('user_login_block');
+        $block['content'] = user_login_block();

I have to admit that I always wondered why the user login form and the user login block form are two separate forms, but I think this refactoring should be discussed in a separate issue -- this patch does not seem to depend on that, unless I overlooked something.

+++ b/core/modules/user/user.module
@@ -1196,6 +1191,12 @@ function user_menu() {
+  // Other authentication methods may add pages below user/login/.
+  $items['user/login/default'] = array(
+    'title' => 'Log in using username and password',
+    'type' => MENU_DEFAULT_LOCAL_TASK,
+    'weight' => -1,
+  );

The weight of the default local task should be -10.

I'm not sure whether it makes sense to repeat "Log in" in the local task, since it is in the parent/root tab already... which would also shorten the other tab to just "OpenID".

#37 looks good for me
didn`t review css styles

Wasn't happy with the JS. Rerolled.

interdiff with #37

Manually tested this - it works!
I found a new tab for openid login a bit confusing but now it's possible to make it primary login form!

PS: About login_form refactoring see #1772584: Get rid of user_login_block() & user_login() in favour of user_login_form()

Awesome. Thanks so much for the hard work on this, folks!

Committed and pushed to 8.x. Thanks.