User entity validation misses form validation logic

I think I could talk someone with some plugins experience through this

Actually, I think we have all of this covered?

User name: Check
User unique: Check
Mail unique: Check
Signature: Check (this is using the field definition max_length, so...)

We also have tests for this in UserValidationTest

However, there are some parts here that better hidden and are not so easy to do. Specifically, that is the part about having to provide the existing password when you want to change the password, username or e-mail. That's dynamic, optional validation depending on the fact whether another field changes or not. I don't think we can deal with that at the moment, and we can also not map that concept to REST at all as far as I can see?

I saw an issue open about this, but this example might be even complexer than that? Assigning to @fago for feedback.

See also how https://www.drupal.org/node/2227381 removes most of the validation, not yet the signature, but I assume that can be removed as well.

Can we ping @fago about it?

Initial patch that adds a validation for signature length and modifies AccountForm::validate() to use the entity validation instead of it's own. I don't think this should be blocked by #2227381: Apply formatters and widgets to User base fields 'name' and 'email' since when that goes through the validation will still be needed and we can just remove the extra lines from AccountForm::validate() that call the entity validations.

+++ b/core/modules/user/src/Entity/User.php
@@ -496,7 +512,8 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
     $fields['signature'] = BaseFieldDefinition::create('string')
       ->setLabel(t('Signature'))
       ->setDescription(t('The signature of this user.'))
-      ->setTranslatable(TRUE);
+      ->setTranslatable(TRUE)
+      ->setConstraints(array('UserSignature' => array()));

Why add a custom constraint that is doing exactly the same as the one we already have by default? You even updated the test to assert for both?

That makes a lot of sense, I was just trying to keep the error messages consistent with the existing messages. I suppose that would be easy to do with a constraint that uses validatedBy()?

Removed the custom validator for now, I wish it was easier to customize those error messages, but this should work for now.

Not sure what's up with the UserRegistrationTest, I updated the strings, but it's still failing for me.

Bah, forgot to rebase the old branch. Here's the correct interdiff.

Not sure why I didn't see that earlier.

+++ b/core/modules/user/src/Tests/UserRegistrationTest.php
@@ -138,13 +138,13 @@ function testRegistrationEmailDuplicates() {
     $this->drupalPostForm('user/register', $edit, t('Create new account'));
-    $this->assertText(t('The email address @email is already registered.', array('@email' => $duplicate_user->getEmail())), 'Supplying an exact duplicate email address displays an error message');
+    $this->assertText(t('The email address @email is already taken.', array('@email' => $duplicate_user->getEmail())), 'Supplying an exact duplicate email address displays an error message');
 
     // Attempt to bypass duplicate email registration validation by adding spaces.
     $edit['mail'] = '   ' . $duplicate_user->getEmail() . '   ';
 
     $this->drupalPostForm('user/register', $edit, t('Create new account'));
-    $this->assertText(t('The email address @email is already registered.', array('@email' => $duplicate_user->getEmail())), 'Supplying a duplicate email address with added whitespace displays an error message');
+    $this->assertText(t('The email address @email is already taken.', array('@email' => $duplicate_user->getEmail())), 'Supplying a duplicate email address with added whitespace displays an error message');

Interesting, I looked at my patch in the widget issue and I did the same. The problem is that registration and existing users use a different validation message in HEAD. I'm not sure if taken or registered is better, registered kind of feels better to me?

Also, looks like we have no validation on the signature length as no fixes were necessary for that, maybe we should add a simple check for that to one of the tests, just to make sure that everything is wired together correctly?

re #15: Yeah the only place the old string was used in core was in AccountForm.php.

re #16: We already have the test in UserValidationTest. Did you want to add something to UserEditTest?

I think it would be good to have a web test for that as well yes, UserEditTest sounds fine. UserValidationTest does not and can not test that we are calling the validation of the signature in the form and we're calling it on the right values (e.g., making sure that we assign the values first, as we currently have to do that manually).

I agree with berdir that a dedicated UI test is helpful.

1. +++ b/core/modules/user/src/AccountForm.php
   @@ -368,64 +376,23 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
   +    foreach ($violations as $violation) {
   +      $form_state->setErrorByName('name', $violation->getMessage());
        }
   ...
   +    $violations = $account->mail->validate();
   +    foreach ($violations as $violation) {
   +      $form_state->setErrorByName('mail', $violation->getMessage());
        }
   ...
   +    $violations = $account->signature->validate();
   +    foreach ($violations as $violation) {
   +      $form_state->setErrorByName('signature', $violation->getMessage());
        }
   

   I love how nice this looks like!

2. +++ b/core/modules/user/src/Entity/User.php
   @@ -291,6 +291,14 @@ public function getSignature() {
   +  public function setSignature($signature) {
   +    $this->get('signature')->value = $signature;
   +    return $this;
   
   @@ -298,6 +306,14 @@ public function getSignatureFormat() {
   +  public function setSignatureFormat($signature_format) {
   +    $this->get('signature_format')->value = $signature_format;
   +    return $this;
   

   It would be nice to open up a follow up to convert signature into a single TextItem.

re #2: I think that's what #1548204: Remove user signature and move it to contrib covers?

@mikey_p
Cool, just added that as a related issue

Test that the form validation calls out to entity validation.

Tested Maximum length of user signature:

1. Enabled Signature field for user profile.
2. Created a new user
3. Edited the user's profile and added signature with the length greater than signature maximum length(255) .

Patch looks good, test cases seem sufficient. I agree that we should split off the security issue regarding password changes to #2418119: REST user updates bypass tightened user account change validation. Only some minor nitpicks:

1. +++ b/core/modules/system/src/Tests/Entity/EntityValidationTest.php
   @@ -113,7 +113,8 @@ protected function checkValidation($entity_type) {
   -    $this->assertEqual($violations->count(), 1, 'Validation failed.');
   +    // This should fail on AllowedValues and Length constraints.
   +    $this->assertEqual($violations->count(), 2, 'Validation failed.');
        $this->assertEqual($violations[0]->getMessage(), t('This value is too long. It should have %limit characters or less.', array('%limit' => '12')));
   

   2 violations here, so we should also assert the second message here.

2. +++ b/core/modules/user/src/AccountForm.php
   @@ -359,7 +360,21 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
   +    /** @var \Drupal\user\UserInterface $account */
   +    $account = parent::buildEntity($form, $form_state);
   +
   +    $signature = $form_state->getValue('signature');
   +    $account->setSignature($signature['value']);
   +    $account->setSignatureFormat($signature['format']);
   

   what's up with the signature here, why do we have to populate it manually? Please add a comment.

3. +++ b/core/modules/user/src/AccountForm.php
   @@ -368,63 +383,24 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
   +    // Customly trigger validation of manually added fields and add in
   +    // violations.
   +    $field_names = array(
   

   why do we have to trigger the validation manually here? Why does parent::validate() fail us here? Please add a comment.

4. +++ b/core/modules/user/src/Entity/User.php
   @@ -553,4 +594,35 @@ protected function getRoleStorage() {
   +   * @return string[]
   

   Description is missing, like "The machine names of allowed filter formats."

5. +++ b/core/modules/user/src/Entity/User.php
   @@ -553,4 +594,35 @@ protected function getRoleStorage() {
   +   * @return string[]
   

   Same here.

6. +++ b/core/modules/user/src/Entity/User.php
   @@ -553,4 +594,35 @@ protected function getRoleStorage() {
   +   * @return string[]
   

   And here.

+++ b/core/modules/user/src/AccountForm.php
@@ -368,63 +385,25 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
+      'name',
+      'mail',
+      'signature',
+      'signature_format',
+      'timezone',
+      'langcode',
+      'preferred_langcode',
+      'preferred_admin_langcode'
+    );

Should we add a corresponding section in UserEditTest for each of these validations?

Great, almost ready. I missed a couple of nitpicks last time:

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/LanguageItem.php
   @@ -51,6 +57,15 @@ public static function propertyDefinitions(FieldStorageDefinitionInterface $fiel
      /**
   +   * Defines allowed language codes for the field's AllowedValues constraint.
   +   *
   +   * @return string[]
   +   */
   +  public static function getAllowedLanguageCodes() {
   

   @return description missing.

2. +++ b/core/modules/user/src/Entity/User.php
   @@ -553,4 +594,38 @@ protected function getRoleStorage() {
   +   *   The allowed values.
   

   Not really creative, but OK :-P

3. +++ b/core/modules/user/src/Tests/UserEditTest.php
   @@ -16,12 +16,43 @@
   +  public static $modules = array('filter');
   +
   +  protected function setUp() {
   

   doc blocks are missing.

Looks good, assuming the testbot comes back green.

Yay to see this RTBC! Also, confirmed the criticality of this with the branch maintainers.

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 17c2500 and pushed to 8.0.x. Thanks!

+++ b/core/modules/user/src/AccountForm.php
@@ -368,63 +385,25 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
+    // Customly trigger validation of manually added fields and add in
+    // violations. This is necessary as entity form displays only invoke entity
+    // validation for fields contained in the display.
+    $field_names = array(
+      'name',
+      'mail',
+      'signature',
+      'signature_format',
+      'timezone',
+      'langcode',
+      'preferred_langcode',
+      'preferred_admin_langcode'
+    );

There is an issue to issue this right? #2002180: Entity forms skip validation of fields that are edited without widgets?