Password confirm field type should have support for #maxlength attribute

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 apply 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.

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.

Eduardo Morales Alberti → closed merge request !4289

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 and string but the value callback returned an array with both values.

All tests passed, ready to review

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.

sukr_s → changed the visibility of the branch 1344552-password-confirm-field-11x to hidden.

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?

Hiding patches for clarity.

Issue summary appears to be updated so removing that tag.
Doubt we backport anymore so removing that tag too

Issue summary UI section is missing before/after screenshots so leaving that tag.

Ran test only feature but nothing actually failed https://git.drupalcode.org/issue/drupal-1344552/-/jobs/1222354

Not sure what's going on there. Maybe a rebase while UI section is updated? Moving to NW for that.

Believe this is close

- Not sure if the test only changes is working. found 📌 Add support for 'test only' changes to gitlab CI Needs review
- Uploading screenshot for before fix "before-fix-max-length-15.png"
- Screenshot uploaded in #85 "confirm_password maxlength 15.png" is after the fix
- fork has been updated.

Manually ran the test-only job again and it passed without the changes https://git.drupalcode.org/issue/drupal-1344552/-/jobs/1524113. So tests aren't covering the change

Also fyi screenshots mean before/after screenshots need to be added to the issue summary User Interface section. Went ahead and did that just FYI.

Moving to NW for test coverage.

Thanks for updating the screenshots in IS. Noted for future.

Test updated and locally tested resulting following error without changes

PHPUnit 9.6.19 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.6
Configuration: /var/www/html/web/core/phpunit.xml.dist

Testing Drupal\Tests\Core\Form\FormValidatorTest
...................E                                              20 / 20 (100%)

Time: 00:00.116, Memory: 6.00 MB

There was 1 error:

1) Drupal\Tests\Core\Form\FormValidatorTest::testPerformRequiredValidation with data set #5 (array('confirm_password', 12, array('oo06xzt742t4mcm', 'cv4vdy3f6xnxk6g')), 'Test cannot be longer than <e... long.', false)
TypeError: mb_strlen(): Argument #1 ($string) must be of type string, array given

/var/www/html/web/core/lib/Drupal/Core/Form/FormValidator.php:332
/var/www/html/web/core/lib/Drupal/Core/Form/FormValidator.php:246
/var/www/html/web/core/lib/Drupal/Core/Form/FormValidator.php:238
/var/www/html/web/core/lib/Drupal/Core/Form/FormValidator.php:118
/var/www/html/web/core/tests/Drupal/Tests/Core/Form/FormValidatorTest.php:397
/var/www/html/web/vendor/phpunit/phpunit/src/Framework/TestResult.php:729
/var/www/html/web/vendor/phpunit/phpunit/src/Framework/TestSuite.php:685
/var/www/html/web/vendor/phpunit/phpunit/src/Framework/TestSuite.php:685
/var/www/html/web/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:146
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:99

ERRORS!
Tests: 20, Assertions: 35, Errors: 1.

Seeing the same in https://git.drupalcode.org/issue/drupal-1344552/-/jobs/1531400, in the MR there's a test-only feature anyone can run.

Tested manually too and getting expected behavior of the change

Believe this one to be good.

Thanks for working on this one. I have a suggestion in the MR to simplify this.

Replied to MR suggestion.

Doesn't that happen automatically? I really don't think we should introduce that extra code just for this.

Doesn't that happen automatically?

Do you mean that the UI blocks after #maxlength? If so yes.

We could remove the code from formvalidator and depend on the UI to not allow more than #maxlength, though that would not be a good practice to depend on the front end to do the validation. Someone using the REST api say from mobile app will have to do the validation themselves.

Let me know if you want to go with UI only validation, I'll revert the changes in formvalidator.

Doesn't Drupal validate the children separately automatically?
As framework manager in trying to avoid adding extra complexity to the form validator just for this case

This is the only place in the core where maxlength check is being done.

I've simplified the code to check only the first field's value if there are multiple fields (or values) since confirm_password is the only known use case and both values need to match. Hope that helps.

I don't think we can assume that in an array the first value is the one that applies.

FYI @sukr_s it is not accepted issue-queue etiquette to RTBC your own issue. Please use the needs review status.

Doesn't Drupal validate the children separately automatically?

Yes it does, the issue here is the test being used (FormValidatorTest) is a unit test, not an actual test and as such does not even execute the process callback which unsets the #multiple on the outer (confirm password) element.

You need a test like \Drupal\KernelTests\Core\Element\PathElementFormTest to test this properly

FYI @sukr_s it is not accepted issue-queue etiquette to RTBC your own issue. Please use the needs review status.

@larowlan I changed it to RTBC since it was already reviewed and set to RTBC in #90. Since you asked for changes, I assumed that it needs to be set back to RTBC for your attention. It was not a self endorsement of the solution.

Will check the test that you pointed out.

- Added functional form based tests as required in #98
- Confirmed the need for fix in formvalidator.php as required #98. See job https://git.drupalcode.org/issue/drupal-1344552/-/pipelines/179107 which was run after reverting the form validator changes.

Pushed a new approach that limits changes to the form validator.
You were right, the automatic 'validate the children' doesn't work, confirm password is odd in so far as it modifies the values from an array to a single value during validation, and as such the children appear empty when its their time to be validated.
Moved the test to a kernel test to reduce boilerplate

So the new test fails show that the #type password element already supports a max length.
Are we sure we need this on confirm password too?

See \Drupal\Tests\user\Functional\UserLoginTest::testPasswordLengthLogin

I'm leaning towards closed works as designed here

This is a functional test - the link I sent to copy (PathElementTest) is a kernel test.

For future use, can you help me understand on how to choose between functional & kernel test?

I'm leaning towards closed works as designed here

In principle I would also agree. A password field should ideally have min length and not max length restriction. Since multiple people were following the issue, I went ahead and took a shot at it.

They do not guarantee a working Drupal UI, but they do allow us to bootstrap selected portions of Drupal in order to run our test. For this reason, kernel tests are often best for testing the programmatic APIs of your module. If it relies on Drupal, but you're not testing the UI, you want a kernel test.

From an article I found on google, hopefully helps some.

So is the consensus this is works by designed? Sorry loosely been following.

So since so much work went into this I'm going to move credit over to the bugsmash meta so those that helped meaningfully through the years get something.