Password confirm field type should have support for #maxlength attribute

I haven't tested, but the use case and the code seem good to me.

I don't know what the API backport policy is, but I see no reasons for this not to be backported to D7.

I may be missing something, but how does missing #maxlength make the field unusable?

Talked to RaviJ in IRC, and this is indeed a pretty big Drupal WTF.

The problem is that a password field on it's own DOES support maxlength, while the password_confirm field type (which renders as two password inputs) does NOT.

That's the problem to solve here, and RaviJ's patch fixes the problem well.

This should be backported to 7.x, and potentially 6.x, but I'm not sure.

Okay, we need an automated test then. Thanks @cweagans and @Ravi.J!

Here is a testcase verifying the problem.

Note that even with the patch the full-length value is passed to the submit handler and some notices occur - at least locally.

Edit: When doing a reroll, note this typo:

+++ b/core/modules/simpletest/tests/form.testundefined
@@ -1628,3 +1628,43 @@ class FormCheckboxTestCase extends DrupalWebTestCase {
+    // Ensure validation fields if the input is too long.

Should be: Validation fails.

Excellent. Pending testbot, I think the tests look great, aside from the comment typo @Niklas Fiekas points out. Edit: and "secound."

Edit 2: Sorry, misread the comment above. This is the error message:
Warning: mb_strlen() expects parameter 1 to be string, array given in drupal_strlen() (line 444 of /home/niklas/Projekte/drupal-8/core/includes/unicode.inc).

So presumably we'll see that fail on the combined patch.

The exceptions are generated because in _form_validate the password field is an single element with multiple #values in the form of an array so we need to some how make an exception. The obvious option is add additional logic to see if #value is an array and if so we then do the max length check on each element within the array when submitting.

Lets see how this goes

Missed the comment above first time round this patch just fixes the comment

meh

Thanks @marcingy, and thanks @Niklas Fiekas for providing a complete test so we can make sure we get the right solution. Looks like it should pass based on #14.

Meanwhile, one suggestion:

+++ b/core/includes/form.incundefined
@@ -1290,8 +1290,17 @@ function _form_validate(&$elements, &$form_state, $form_id = NULL) {
+      if (isset($elements['#maxlength'])) {
+        if (!is_array($elements['#value']) && drupal_strlen($elements['#value']) > $elements['#maxlength']) {
+          form_error($elements, $t('!name cannot be longer than %max characters but is currently %length characters long.', array('!name' => empty($elements['#title']) ? $elements['#parents'][0] : $elements['#title'], '%max' => $elements['#maxlength'], '%length' => drupal_strlen($elements['#value']))));
+        }
+        else if (is_array($elements['#value'])) {
+          foreach ($elements['#value'] as $element_key => $element_value) {
+            if (drupal_strlen($element_value) > $elements[$element_key]['#maxlength']) {
+              form_error($elements, $t('!name cannot be longer than %max characters but is currently %length characters long.', array('!name' => empty($elements['#title']) ? $elements['#parents'][0] : $elements['#title'], '%max' => $elements[$element_key]['#maxlength'], '%length' => $element_value)));
+            }
+          }
+        }

Rather than duplicating the code here, I think it would be better to do something like:

$foo = is_array($elements['#value']) ? $elements['#value'] : array($elements['#value']);

and then foreach $foo.

(Edited to correct the pseudocode.)

Edit 2: Wait, do we really want to set an error for every item? Wouldn't one error be sufficient? Maybe we should break;?

Tagging for UX sanity check once we have a working solution. :)

Per the definitions at http://drupal.org/node/45111 this is not a major bug. It's arguably not a bug at all (since it's documented behavior), although for consistency it definitely makes sense to change.

Couple issues:

1. I agree with xjm that it should be possible to simplify/combine the code.
2. In addition, the code makes the assumption that when $elements['#maxlength'] is set, each $elements[$element_key]['#maxlength'] will be set also, but nowhere is that enforced. Perhaps as part of fixing #1 this would just wind up using $elements['#maxlength'] in all cases anyway? Or is there actually a use case where we need to support different maxlengths for different sub-elements of the array?
3. '%length' => $element_value should be '%length' => drupal_strlen($element_value) so that the correct thing is printed.

4. +        else if (is_array($elements['#value'])) {
   

   Note sure if it's an official coding standard, but I think we usually use "elseif" in PHP code?

5. Edit 2: Wait, do we really want to set an error for every item? Wouldn't one error be sufficient? Maybe we should break;?

   I think we do want the red border around each item, although it looks like the code will probably do that even if only one error is set... When I tested the patch, I also expected to see a problem where two error messages were displayed. For some reason, though, only one error message is displayed (even though two errors are set); I didn't track down why. We definitely only want one error message. I think this ties into my comment #2 above; are we actually validating them separately or really just validating the whole thing as a group?

To partially answer my question in #2, I guess the deal is that each sub-element has to have its own #maxlength in order for it to actually be printed in the HTML...

Still seems odd at least in the password confirm use case, since you'd never want the two elements to have different #maxlengths there.

Note sure if it's an official coding standard, but I think we usually use "elseif" in PHP code?

Yep: http://drupal.org/coding-standards#controlstruct

And regarding #2, changing all of _form_validate() for this does seem a bit odd, and there's certainly no reason you'd have a different maxlength for the confirm password field vs. the password. I don't even know what it would mean outside this context.

Edit: xpost.

As I got into this to be honest I agree it seems much more like a task/feature request than a bug, and if you need to validate field length in password hook validate can be utilised. And after the comments above it seems like we really need to refractor how the password element works.

Updating tags per http://drupal.org/node/1517250

Related issue: #111322: Create #confirm property to ensure equality of two input fields

I think we need to look at this to see if things have changed in 8.x
And we need to decide on an approach to take.
Updating the issue summary to reflect that needs screenshots is waiting on a new patch.

Issue summary also needs updating to summarize different possible approaches to take.

Reformatted body text to make the issue easier to understand

Actually, screenshot could be made of the page that will change. We can get the *before* screenshot to show what it looks like right now. That would also be a good chance to document the steps to reproduce of how to get to that page.

clarified nothing right now to do screenshots of

Looks like we need to redo this whole patch, cuz we don't have these test files and the function "_form_validate" in the core/includes/form.inc

So I added a new one.

@aleevas patch should be applicable with `patch -p1`! your one is `-p0`

patch #36 was updated

Patch is not working on 9.1
Please check

Resolving failed test cases from #40

This issue doesn't exists for drupal 9.1. Max limit for Confirm Password and Password field is set to 128 characters and it automatically crops character to it.
Screenshot is attached

Steps:
1. Visit /admin/people/create
2. Enter a long text of 200 characters on Password field.
3. When cursor is out from Password field, it will automatically crop the text by 128 characters.
4. Enter a long text of 200 characters on Confirm Password field.
5. Again cursor is out from Confirm Password field, it will automatically crop the text by 128 characters.
6. Similarly, we can try with User profile update form- /user/4/edit, but result will be same as in pt. 3 & 4

there is no issue exists for drupal 9.3. Max limit for Confirm Password and Password field is set to 128 characters and it automatically crops character to it.
Screenshot is attached .................

Verified and tested this issue on the 9.3.x-dev version.
Working as Expected.

Testing Steps:
# Goto: Appearance -> Apply Seven Theme
# Goto: /admin/people/create
# Enter a long text of 200 characters on the Password field.
# When the cursor is out from the Password field, it will automatically crop the text by 128 characters.
# Enter a long text of 200 characters on Confirm Password field, it will automatically crop the text by 128 characters.
# Observe the results

Expected Results:
# Verified that Max limit for Confirm Password and Password field is set to 128 characters and it will automatically crop characters to it.

Note: This issue is already FIXED on this (9.3.x-dev) version, So we can close this issue.

Please refer attached screenshots for the same.
Looks good to me.
Can be a move to RTBC.

This issue doesn't exists for drupal 9.5.x-dev.
1. Password confirm field type now have support for #maxlength attribute
2. Max limit for Confirm Password and Password field is set to 128 characters and it automatically crops character to it.
3. User is able to login using 128 character password (even when 200 is entered while creating user account)

No patch required, moving to RTBC

I've taken a look in 9.5.0-dev and I can confirm that the password fields have a maxlength attribute of 128. In contrast the latest patch in this issue added a maxlengh of 12 only? In regards of the issue status I wouldn't set it to RTBC but Closed (Outdated).

But in regards of the observations @sonam.chaturvedi made in #55.2 & #55.3. I have manually tested and have a few differing observations:

• I've copy and pasted a password with a length of 138 to the password and confirm password field. None of the two fields notified me that I've exceeded the maximum password length. I was able to successfully save the password changes afterwards. I've then logged out and tried to relogin. First with the first 128 characters of the 138 character password, that failed so the password isn't cropped. I was able to login using the 138 characters password. That is a consider a good thing. Cropping a password without any notification at all silently i would consider a ux no-go. but still the maxlength attribute has no effect at all and should be fixed.
• I've then went again to the user edit page and copy and pasted the 138 characters password in the current password, password and confirm password field. I've then added one or two extra characters to the confirm password field. The password match remained to yes. Same when I've then added extra characters to the password field. It looks like as soon as the password length exceeds 128 characters the password match check isn't applied anymore and remains to yes all the time?

i've tested in the latest safari, firefox and edge - all behave the same. I am setting the issue to needs work to get feedback on #55 and my comment. Uncertain what the best approach would be. Closing this issue as outdated and opening a new one or repurposing it by updating the issue summary and deal with described issues in here?

In our case, we use client side validation that has its own maxlength validation and we need the patch to override the maxlength on the confirm password element.

Examples:

    $form['current_password'] = [
      '#title' => $this->t('Current Password'),
      '#title_display' => 'none',
      '#type' => 'password',
      '#size' => 64,
      '#maxlength' => 64,
      '#required' => TRUE,
    ];

    $form['password'] = [
      '#title' => $this->t('New Password'),
      '#title_display' => 'none',
      '#size' => 64,
      '#maxlength' => 64,
      '#type' => 'password_confirm',
      '#required' => TRUE,
    ];

After applying the patch I got the error:

mb_strlen(): Argument #1 ($string) must be of type string, array given in mb_strlen() (line 334 of /var/www/html/docroot/core/lib/Drupal/Core/Form/FormValidator.php) 

Because the confirm password element has two values, and the Form validator expects to validate an int.

The single value is set on Password Confirm Validation, but after the maxlength validation (FormValidator).

  public static function processPasswordConfirm(&$element, FormStateInterface $form_state, &$complete_form) {
    $element['#element_validate'] = [[static::class, 'validatePasswordConfirm']];
.....

  public static function validatePasswordConfirm(&$element, FormStateInterface $form_state, &$complete_form) {
    $pass1 = trim($element['pass1']['#value']);
    $pass2 = trim($element['pass2']['#value']);
    if (strlen($pass1) > 0 || strlen($pass2) > 0) {
      if (strcmp($pass1, $pass2)) {
        $form_state->setError($element, t('The specified passwords do not match.'));
      }
    }
    elseif ($element['#required'] && $form_state->getUserInput()) {
      $form_state->setError($element, t('Password field is required.'));
    }

    // Password field must be converted from a two-element array into a single
    // string regardless of validation results.
    $form_state->setValueForElement($element['pass1'], NULL);
    $form_state->setValueForElement($element['pass2'], NULL);
    $form_state->setValueForElement($element, $pass1);

    return $element;
  }

Form validator.php

  protected function doValidateForm(&$elements, FormStateInterface &$form_state, $form_id = NULL) {
....
    if (!isset($elements['#validated']) || !$elements['#validated']) {
      // The following errors are always shown.
      if (isset($elements['#needs_validation'])) {
        $this->performRequiredValidation($elements, $form_state);
      }
....
      elseif (isset($elements['#element_validate'])) {
        foreach ($elements['#element_validate'] as $callback) {
          $complete_form = &$form_state->getCompleteForm();
          call_user_func_array($form_state->prepareCallback($callback), [&$elements, &$form_state, &$complete_form]);
        }
      }

  protected function performRequiredValidation(&$elements, FormStateInterface &$form_state) {
    // Verify that the value is not longer than #maxlength.
    if (isset($elements['#maxlength']) && mb_strlen($elements['#value']) > $elements['#maxlength']) {
      $form_state->setError($elements, $this->t('@name cannot be longer than %max characters but is currently %length characters long.', ['@name' => empty($elements['#title']) ? $elements['#parents'][0] : $elements['#title'], '%max' => $elements['#maxlength'], '%length' => mb_strlen($elements['#value'])]));
    }

Changed the value callback to get the right validation on maxlength.

Patch from MR.

Still working on the patch because of the error on install:

 [warning] Trying to access array offset on value of type null PasswordConfirm.php:106
 [warning] Trying to access array offset on value of type null PasswordConfirm.php:107

In install.core.inc line 971:
                               
  Password field is required.  

Patch for 11x.

Solve errors on the site install.

Reviewing tests failed:

Testing Drupal\Tests\user\Functional\UserLoginTest
....E.                                                              6 / 6 (100%)

Time: 00:30.595, Memory: 4.00 MB

There was 1 error:

1) Drupal\Tests\user\Functional\UserLoginTest::testPasswordLengthLogin
Behat\Mink\Exception\ResponseTextException: The text "The changes have been saved." was not found anywhere in the text of the current page.

Drupal\Tests\Core\Render\Element\PasswordConfirmTest
fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-557.xml:
PHPUnit Test failed to complete; Error: PHPUnit 9.6.8 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\Core\Render\Element\PasswordConfirmTest
FFFFFF                                                              6 / 6 (100%)

Time: 00:00.054, Memory: 6.00 MB

There were 6 failures:

1) Drupal\Tests\Core\Render\Element\PasswordConfirmTest::testValueCallback with data set #0 (array('', ''), array(), null)
Failed asserting that '' is identical to Array &0 (
    'pass1' => ''
    'pass2' => ''
).

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/IsIdentical.php:79
/var/www/html/core/tests/Drupal/Tests/Core/Render/Element/PasswordConfirmTest.php:22
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:144
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:97

2) Drupal\Tests\Core\Render\Element\PasswordConfirmTest::testValueCallback with data set #1 (array('', ''), array(array('value')), null)
Failed asserting that '' is identical to Array &0 (
    'pass1' => ''
    'pass2' => ''
).

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/IsIdentical.php:79
/var/www/html/core/tests/Drupal/Tests/Core/Render/Element/PasswordConfirmTest.php:22
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:144
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:97

3) Drupal\Tests\Core\Render\Element\PasswordConfirmTest::testValueCallback with data set #2 (array('value', ''), array(array('value')), false)
Failed asserting that '' is identical to Array &0 (
    'pass2' => 'value'
    'pass1' => ''
).

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/IsIdentical.php:79
/var/www/html/core/tests/Drupal/Tests/Core/Render/Element/PasswordConfirmTest.php:22
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:144
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:97

4) Drupal\Tests\Core\Render\Element\PasswordConfirmTest::testValueCallback with data set #3 (array('123456', 'qwerty'), array(), array('123456', 'qwerty'))
Failed asserting that '123456' is identical to Array &0 (
    'pass1' => '123456'
    'pass2' => 'qwerty'
).

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/IsIdentical.php:79
/var/www/html/core/tests/Drupal/Tests/Core/Render/Element/PasswordConfirmTest.php:22
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:144
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:97

5) Drupal\Tests\Core\Render\Element\PasswordConfirmTest::testValueCallback with data set #4 (array('123', '234'), array(), array(123, 234))
Failed asserting that '123' is identical to Array &0 (
    'pass1' => '123'
    'pass2' => '234'
).

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/IsIdentical.php:79
/var/www/html/core/tests/Drupal/Tests/Core/Render/Element/PasswordConfirmTest.php:22
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:144
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:97

6) Drupal\Tests\Core\Render\Element\PasswordConfirmTest::testValueCallback with data set #5 (array('', '234'), array(), array(array('array'), 234))
Failed asserting that '' is identical to Array &0 (
    'pass1' => ''
    'pass2' => '234'
).

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/IsIdentical.php:79
/var/www/html/core/tests/Drupal/Tests/Core/Render/Element/PasswordConfirmTest.php:22
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:144
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:97

FAILURES!
Tests: 6, Assertions: 6, Failures: 6.

Fixing tests, value callback now returns only the password 1 because the second password is only for validation, and the password confirm already validates it, and also it makes the max length validation from the core fail because it expects a string but the value callback returned an array with both values.

All tests passed, ready to review.
The changes on MR https://git.drupalcode.org/project/drupal/-/merge_requests/4290/diffs are the same that the patch https://www.drupal.org/files/issues/2023-07-03/core-password_confirm_sup...

Hi @Eduardo Morales Alberti, thanx for the patch

I Reviewed and tested the patch. Confirmed that the following changes have been made and works as expected as:
In the valueCallback() method, returns an empty string instead of an array with empty values if no input is provided.
In the processPasswordConfirm() method, the explicit setting of for element pass1 and pass2 has been removed to depends on the value callback. Additionally, the '#maxlength' attribute is now correctly set for both fields when the attribute is specified on the PasswordConfirm element.
After thorough testing, all changes are working as expected and the functionality working fine. Moving to RTBC

Not the same issue, but could be related because are altering the same field.

I am doing triage on the core RTBC queue.

Thanks for working on this old issue!

This issue has no issue summary! The issue summary is the first thing a reviewer or committer will see as they start their work. In #5, xjm asks the question I am asking as well, why is this needed. That information should be in the issue summary. Issue summaries matter.

And I see that this is tagged for "Needs issue summary update" and "Needs screenshots". I am setting this o needs work for the issue summary update and screenshots.

I see that there is a recent MR and a patch. Which one is to be reviewed?

- I've updated the issue summary
- fixed the issue
- added test

Seems to be multiple MRs and patches these need to be cleaned up for clarity.

Was previously tagged for screenshots but the User interface changes section is empty.

Have not reviewed.

Adding screenshot #maxlength set to 15. UI doesn't allow more than 15 characters.

hidden older MR !4290

Pipeline has not run for MR !7188. How can I trigger this?