Convert form validation of comments to entity validation

Just temporary patch. Still need working under validate() in Comment.php. I'm going to make researching on entity validation deeper. So proposals about patch are welcome!

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/Core/Entity/Comment.phpundefined
@@ -219,4 +219,26 @@ protected function init() {
+  public function validate() {
+    ¶

code indenting error

Code cleanup

@fago. Many thanks for information. I'll look this and re-wtite code according to your advices.

First patch that adds some length constraints and a test case.

I also tried to convert the comment email field to the email_field type. That showed a very strange bug where the isEmpty() method returns TRUE although an email value has been set. I tried to debug this and as soon as I copy over the exact code from ItemList::isEmpty() to Field::isEmpty() the test passes.

So I think that there is some underlying bug with setting values and isEmpty(), or I'm doing something very wrong here.

Ok, so let's ignore the email field for now, that should be investigated in a different issue.

Added a CommentName validation constraint to check that the anonymous author name does not collide with a registered user name on the site. And a lot of other length constraints.

Ha, I think I found the problem in the setValue() method of FieldItemBase. Why are we unsetting the values here? Anyway, let's see what else breaks with this change.

Ok, so that was a bad idea. Turns out we need to fix the isEmpty() method of EmailItem. Actually that fix deserves its own unit test, but I could not reproduce it with typed data alone:

$item = $this->typedData->create(array(
  'type' => 'email_item',
  'class' => 'Drupal\Core\Entity\Plugin\DataType\EmailItem',
));
$item->setValue('test@example.com');
debug($item->isEmpty()); // Returns FALSE as expected, why does it work here?

Anyway, this should fix the email validation for comments now.

Looks like there is a hidden dependency to email.module in EntityStorageControllerBase::invokeFieldItemPrepareCache(), so we simple check if the module key even exists in the typed data definition before reading from it.

Added a test case for invalid comment homepage now that #2039961: uri_field should use uri type as value is committed.

Does not apply anymore, rerolled.

So suddenly the CommentTranslationUITest fails, does anyone have a clue why?

When looking at the test case I found out that in ContentTranslationUITest::assertBasicTranslation() after line 74 a translation for French already exists. That's why the POST request on line 80 fails, because it tries to add a translation which already exists, so it gets an access denied on /fr/comment/1/translations/add/en/fr.

BTW: never start your test methods with "assert", that is a special name where Simpletest does not show the correct line number, which makes the test case harder to debug. Example: use checkBasicTranslation() instead of assertBasicTranslation().

So it looks like the comment homepage uri_field was set to the empty string, which triggered a constraint violation (because empty strings are not valid URIs). So I fixed the PrimitiveTypeConstraintValidator to ignore empty string values, is that the correct approach? Or should we unset() entity fields if the form API gives us an empty string for a field?

#20: comment-validation-2002158-20.patch queued for re-testing.

#20: comment-validation-2002158-20.patch queued for re-testing.

The change for EmailItem is not necessary anymore, removed that.

+ $comment = $this->context->getMetadata()->getTypedData()->getParent()->getParent();

Really? Thats what Drupal has become?

Also, we are simply validating string length here. Do we really need to keep creating new constraint classes each time. This seems extraordinarily verbose.

Yeah, retrieving the comment object from the validation context is a bit long, but I guess we can't do much about that at this point.

No, we don't have to create length constraints every time, we can simply reuse them. But we have to create a constraint for the comment author name, because we have to run a custom query that does not already exist in another constraint.

#24: comment-validation-2002158-24.patch queued for re-testing.

Rerolled and remove a @todo that does not make sense, because CommentNewValue is a computed field anyway.

klausi: CommentNewValue is going away in #1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user :)

Patch does not apply anymore, rerolled. No other changes.

#31: comment-validation-2002158-31.patch queued for re-testing.

Rerolled, no other changes.

Comments are fields now, so had to fix the test case.

+ $comment = $this->context->getMetadata()->getTypedData()->getParent()->getParent();

Really? Thats what Drupal has become?

#2110345: Simplify validation constraint implementations for fields improves that.

Updated issue summary.

There are a couple changes I'd like to make here. Starting with a reversion of the change to PrimitiveTypeConstraintValidator. Curious about what currently fails if we do that.

This moves the constraint from the context-less $value to a context-full $field_item per #2110345-4: Simplify validation constraint implementations for fields, and also converts the db query to a EFQ to match what is done in HEAD's CommentFormController::validate().

I think this is good to go, but since I made changes, I'm not eligible to RTBC. Anyone else care to?

1. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/Validation/Constraint/CommentNameConstraintValidator.php
   @@ -0,0 +1,37 @@
   +      $author_is_unauthenticated = ($field_item->getEntity()->uid->value === 0);
   

   should we use id() method here?

2. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentValidationTest.php
   @@ -0,0 +1,124 @@
   +    $this->assertEqual($violations[0]->getMessage(), t('The name you used belongs to a registered user.'));
   

   It is a bit awkward IMO that our violations make referene to the value instead of actually telling you 'foo' is already a user on this site.

3. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentValidationTest.php
   @@ -0,0 +1,124 @@
   +    $this->assertEqual($violations[0]->getMessage(), t('This value is not a valid email address.'));
   

   Same. Tell us the value that is invalid.

4. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentValidationTest.php
   @@ -0,0 +1,124 @@
   +    $this->assertEqual($violations[0]->getMessage(), t('This value should be of the correct primitive type.'));
   

   Same. What value? "Primitive type" is a very odd error message as well.

Child issue of #1696648: [META] Untie content entity validation from form validation, therefore critical.

Rerolled.

@moshe: there is a @todo in the code to fix accessing the comment author, so leaving that as is for now. And regarding the messages: those are generic messages most of the time, we have the property path to indicate which variable should be fixed. I agree that "This value should be of the correct primitive type." is a terrible message, but that should be fixed in a separate issue against PrimitiveTypeConstraint and PrimitiveTypeConstraintValidator.

Fixed most of that things.

I don't think we can establish a generic max length for URIs, since no such thing is defined. The field schema does not even use "varchar" but rather "text", so it looks like we are prepared for long values here.

And opened #2171539: Improve PrimitiveTypeConstraint validation messages.

klausi opened a new pull request for this issue.

Fixed fago's remarks. Now also using max_length for UriItem, where we need to specify a default. I went with 2048 which seems to be the limit of older browsers and search engines, see http://stackoverflow.com/questions/417142/what-is-the-maximum-length-of-...

Some commits were pushed to the pull request.

klausi pushed some commits to the pull request.

Rerolled after #2002168: Convert form validation of terms to entity validation went in.

Looks RTBC, except 2 small nitpicks (3,4)

1. +++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
   @@ -394,19 +395,27 @@ public static function baseFieldDefinitions($entity_type) {
   +    $fields['homepage'] = FieldDefinition::create('uri')
   ...
   +      ->setDescription(t("The comment author's home page address."))
   +      // URIs are not length limited by RFC 2616, but we can only store 255
   +      // characters in our comment DB schema.
   +      ->setSetting('max_length', 255);
   

   This needs follow-up issue to fix, maybe allover core
   But it should not stop this issue

2. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/Validation/Constraint/CommentNameConstraint.php
   @@ -0,0 +1,23 @@
   +  public $message = '%name belongs to a registered user.';
   

   Original message is 'The name you used belongs to a registered user.'
   But I think it's fine now, would be unified in #2002180: Entity forms skip validation of fields that are edited without widgets

3. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentValidationTest.php
   @@ -0,0 +1,124 @@
   +    // Make the name valid.
   +    $comment->set('name', $this->randomString());
   

   Just a small chance that random will be the same as root_user, but better extend comment here or make

   do {
   $name = randomstring;
   } while (root_user name != $name) 

4. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentValidationTest.php
   @@ -0,0 +1,124 @@
   +  }
   +}
   

   needs blank line between

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

Fixed issue 3) and 4) mentioned by andypost, leaving 1) and 2) as is because fixing other URI fields is out of scope for this issue and the validation error message has already been bike-shedded above.

Awesome!

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/Validation/Constraint/CommentNameConstraintValidator.php
@@ -0,0 +1,37 @@
+        $users = \Drupal::entityManager()->getStorageController('user')->loadByProperties(array('name' => $author_name));

Is there any reason beside lazyness that we don't inject dependencies for new code?

Seems ConstraintManager does not allows to inject container

Filed follow-up #2197029: Allow to inject dependencies into validation constraints
And fixed following

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/Validation/Constraint/CommentNameConstraintValidator.php
@@ -0,0 +1,37 @@
+      // @todo Improve DX of this after https://drupal.org/node/2078387.
+      $author_is_unauthenticated = ($field_item->getEntity()->uid->value === 0);

This already fixed

+1

63: 2002158-63.patch queued for re-testing.

After #2181593: Convert entity bundle base fields to entity reference we need properly populate 'field_name' and this field should exists

Postponed on #2197723: Comment::getFieldId() should return actual field ID
That interdiff fixes the test, but includes hack from [2197723-1]

No need to postpone critical, so just revert the part of #2181593-26: Convert entity bundle base fields to entity reference and re-open #2149859: Convert the 'field_id' base field on comment entities to an entity reference field

Also comment should be created by passing 'field_name' and field with instance should exists.

Only stupid nitpicks:

1. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/Validation/Constraint/CommentNameConstraint.php
   @@ -0,0 +1,23 @@
   +  public $message = '%name belongs to a registered user.';
   +}
   
   +++ b/core/modules/comment/lib/Drupal/comment/Plugin/Validation/Constraint/CommentNameConstraintValidator.php
   @@ -0,0 +1,35 @@
   +  }
   +}
   

   Missing newline before closing curly brace.

2. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/Validation/Constraint/CommentNameConstraintValidator.php
   @@ -0,0 +1,35 @@
   +    if (isset($author_name) && ($author_name !== '')) {
   

   Why these parentheses?

3. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentValidationTest.php
   @@ -0,0 +1,138 @@
   +    $this->assertLengthViolation($comment, 'subject', 64);
   +    // Make the subject valid.
   +    $comment->set('subject', $this->randomString());
   +
   +    $comment->set('name', $this->randomString(61));
   +    $this->assertLengthViolation($comment, 'name', 60);
   

   Why no newline before the code comment, but one between two method calls?

4. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentValidationTest.php
   @@ -0,0 +1,138 @@
   +    $comment->set('name', 'valid unused name');
   +
   +    $comment->set('mail', 'invalid');
   

   Similar strange newline here.

Fixed 1-4 + doc-block

Related issue explains why comment field_id can't be used as entity reference

All the newlines got adressed

Committed/pushed to 8.x, thanks!