Let's convert the form validation logic of comments to validation constraints + test that. We can keep the existing form validation in place until we leverage the entity validation for forms and remove it then.

Files: 
CommentFileSizeAuthor
#71 2002158-71.patch11.01 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 64,440 pass(es). View
#71 interdiff.txt5.21 KBandypost

Comments

fago’s picture

Issue tags: +Entity Field API
vlad.dancer’s picture

Assigned: Unassigned » vlad.dancer
Status: Active » Needs review
Issue tags: +CodeSprintUA
FileSize
2.76 KB
FAILED: [[SimpleTest]]: [MySQL] 55,834 pass(es), 11 fail(s), and 0 exception(s). View

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!

podarok’s picture

Status: Needs review » Needs work
+++ 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

vlad.dancer’s picture

Status: Needs work » Needs review
FileSize
2.76 KB
FAILED: [[SimpleTest]]: [MySQL] 57,818 pass(es), 11 fail(s), and 0 exception(s). View

Code cleanup

Status: Needs review » Needs work

The last submitted patch, form-validation-comments-to-entity-validation-2002158-4.patch, failed testing.

fago’s picture

We should not invoke validate from save(). Please see for the patch in #2002152: Implement entity validation based on symfony validator for a working example and implement it analogously.

Note, that #2002152: Implement entity validation based on symfony validator contains the foundational constraints you'll need, so best just base any work on this on the patch over there.

vlad.dancer’s picture

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

klausi’s picture

Status: Needs work » Needs review
FileSize
5.69 KB
FAILED: [[SimpleTest]]: [MySQL] 54,963 pass(es), 133 fail(s), and 293 exception(s). View
4.79 KB
FAILED: [[SimpleTest]]: [MySQL] 56,979 pass(es), 2 fail(s), and 249 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, comment-validation-2002158-8-hack.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
9.17 KB
10.3 KB
PASSED: [[SimpleTest]]: [MySQL] 56,622 pass(es). View

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.

klausi’s picture

FileSize
2.62 KB
10.83 KB
FAILED: [[SimpleTest]]: [MySQL] 56,326 pass(es), 53 fail(s), and 8,524 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, comment-validation-2002158-11.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
1.34 KB
10.92 KB
FAILED: [[SimpleTest]]: [MySQL] 57,070 pass(es), 0 fail(s), and 248 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, comment-validation-2002158-13.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
12.03 KB
PASSED: [[SimpleTest]]: [MySQL] 57,255 pass(es). View
1.1 KB

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.

klausi’s picture

FileSize
1.22 KB
12.25 KB
PASSED: [[SimpleTest]]: [MySQL] 56,858 pass(es). View

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

klausi’s picture

FileSize
1.46 KB
12.25 KB
FAILED: [[SimpleTest]]: [MySQL] 57,663 pass(es), 25 fail(s), and 0 exception(s). View

Does not apply anymore, rerolled.

Status: Needs review » Needs work

The last submitted patch, comment-validation-2002158-17.patch, failed testing.

klausi’s picture

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().

klausi’s picture

Status: Needs work » Needs review
FileSize
702 bytes
12.22 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment-validation-2002158-20.patch. Unable to apply patch. See the log in the details link for more information. View

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?

klausi’s picture

moshe weitzman’s picture

Status: Needs review » Needs work
Issue tags: +Entity Field API, +CodeSprintUA

The last submitted patch, comment-validation-2002158-20.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
11.59 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment-validation-2002158-24.patch. Unable to apply patch. See the log in the details link for more information. View

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

moshe weitzman’s picture

+ $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.

klausi’s picture

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.

Berdir’s picture

Status: Needs review » Needs work
Issue tags: +Entity Field API, +CodeSprintUA

The last submitted patch, comment-validation-2002158-24.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
658 bytes
11.48 KB
PASSED: [[SimpleTest]]: [MySQL] 58,544 pass(es). View

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

Wim Leers’s picture

klausi’s picture

FileSize
10.38 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment-validation-2002158-31.patch. Unable to apply patch. See the log in the details link for more information. View

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

jibran’s picture

Status: Needs review » Needs work
Issue tags: +Entity Field API, +CodeSprintUA

The last submitted patch, comment-validation-2002158-31.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
10.34 KB
FAILED: [[SimpleTest]]: [MySQL] 58,527 pass(es), 1 fail(s), and 0 exception(s). View

Rerolled, no other changes.

Status: Needs review » Needs work

The last submitted patch, comment-validation-2002158-34.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
1.35 KB
10.47 KB
PASSED: [[SimpleTest]]: [MySQL] 59,018 pass(es). View

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

effulgentsia’s picture

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

Really? Thats what Drupal has become?

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

effulgentsia’s picture

Issue summary: View changes

Updated issue summary.

effulgentsia’s picture

Assigned: vlad.dancer » effulgentsia
Issue summary: View changes
FileSize
9.78 KB
FAILED: [[SimpleTest]]: [MySQL] 59,150 pass(es), 1 fail(s), and 0 exception(s). View
402 bytes

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.

The last submitted patch, 38: comment-validation-2002158-38.patch, failed testing.

effulgentsia’s picture

FileSize
9.95 KB
PASSED: [[SimpleTest]]: [MySQL] 59,090 pass(es). View
3.1 KB

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().

effulgentsia’s picture

Assigned: effulgentsia » Unassigned

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

moshe weitzman’s picture

Status: Needs review » Needs work
  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.

effulgentsia’s picture

klausi’s picture

Status: Needs work » Needs review
FileSize
10.07 KB
PASSED: [[SimpleTest]]: [MySQL] 59,826 pass(es). View
2.13 KB

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.

fago’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
    @@ -396,19 +397,25 @@ public static function baseFieldDefinitions($entity_type) {
    +      ->setPropertyConstraints('value', array('Length' => array('max' => 255)));
    ...
    +      ->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 DB schema.
    

    Reasonable, but then this constraint should be default in the field type?

  2. +++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
    @@ -396,19 +397,25 @@ public static function baseFieldDefinitions($entity_type) {
    +      ->setPropertyConstraints('value', array('Length' => array('max' => 255)));
    

    that's part of the field type already - or better should be. It has a weird max_length setting that does not seem to be applied. It should have a constraint by default though, as UuidItem.

  3. +++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
    @@ -427,7 +434,8 @@ public static function baseFieldDefinitions($entity_type) {
    +      ->setPropertyConstraints('value', array('Length' => array('max' => 255)));
    

    Again, should be the default.

  4. +++ 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.
    
    +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentValidationTest.php
    @@ -0,0 +1,123 @@
    +    $node = entity_create('node', array(
    

    These used should as an entity manager, created during setup.

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

    Let's add a todo linking the issue to fix that message?

  6. The name you used belongs to a registered user.

    Agreed with moshe - this really should tell me what is already registered.

klausi’s picture

Status: Needs work » Needs review
FileSize
11.88 KB
PASSED: [[SimpleTest]]: [MySQL] 59,951 pass(es). View
8.35 KB

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.

klausi’s picture

fago’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/StringItem.php
    @@ -62,4 +62,23 @@ public static function schema(FieldDefinitionInterface $field_definition) {
    +    // Not all plugins that inherit from this class have a configurable maximum
    +    // length, so we only apply the length constraint conditionally. Example:
    +    // UuidItem has a fixed length of 128.
    +    if ($max_length = $this->getFieldSetting('max_length')) {
    +      $constraint_manager = \Drupal::typedDataManager()->getValidationConstraintManager();
    +      $constraints[] = $constraint_manager->create('ComplexData', array(
    +        'value' => array('Length' => array('max' => $max_length))
    +      ));
    

    That seems weird to me as it violates the inheritance principle. I've not idea why there is an unused max_length setting on the string field - why not just use the length constraint if wanted?

  2. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/Validation/Constraint/CommentNameConstraint.php
    @@ -21,5 +21,5 @@
    +  public $message = 'The name %name you used belongs to a registered user.';
    

    "%name belongs to a registered user."

    ?

  3. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentValidationTest.php
    @@ -93,11 +93,12 @@ public function testValidation() {
    +    // @todo This message should be improved in https://drupal.org/node/2171539
    

    Needs an update ;-)

fago’s picture

>I've not idea why there is an unused max_length setting on the string field

oh - it's used to generate the schema.

fago’s picture

+ // Not all plugins that inherit from this class have a configurable maximum
+ // length, so we only apply the length constraint conditionally. Example:
+ // UuidItem has a fixed length of 128.

oh, we can simply fix uuid-item instead to provide a new default for max_length and we are good.

Also discussed with yched on IRC on settings vs. constraint. Having the setting even if not strictly necessary is fine and is more consistent with what some configurable fields *have to* do. Also, I think the DX is better that way as well :-)

Github sync’s picture

Status: Needs work » Needs review
FileSize
12.65 KB
FAILED: [[SimpleTest]]: [MySQL] 63,118 pass(es), 1 fail(s), and 0 exception(s). View

klausi opened a new pull request for this issue.

klausi’s picture

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

Status: Needs review » Needs work

The last submitted patch, 51: 2002158.patch, failed testing.

Github sync’s picture

Status: Needs work » Needs review
FileSize
14.77 KB
PASSED: [[SimpleTest]]: [MySQL] 63,233 pass(es). View

Some commits were pushed to the pull request.

Github sync’s picture

FileSize
10 KB
PASSED: [[SimpleTest]]: [MySQL] 64,385 pass(es). View

klausi pushed some commits to the pull request.

klausi’s picture

andypost’s picture

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’s picture

FileSize
10 KB
PASSED: [[SimpleTest]]: [MySQL] 64,433 pass(es). View

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

klausi’s picture

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.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Awesome!

dawehner’s picture

+++ 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?

andypost’s picture

Seems ConstraintManager does not allows to inject container

andypost’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.39 KB
9.94 KB
FAILED: [[SimpleTest]]: [MySQL] 64,384 pass(es), 9 fail(s), and 0 exception(s). View

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

+1

alexpott’s picture

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 63: 2002158-63.patch, failed testing.

andypost’s picture

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

andypost’s picture

Issue summary: View changes
Status: Needs work » Postponed
FileSize
2.69 KB

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

andypost’s picture

Status: Postponed » Needs review
FileSize
2.59 KB
10.98 KB
PASSED: [[SimpleTest]]: [MySQL] 64,438 pass(es). View

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.

Wim Leers’s picture

Status: Needs review » Needs work

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.

andypost’s picture

Status: Needs work » Needs review
FileSize
5.21 KB
11.01 KB
PASSED: [[SimpleTest]]: [MySQL] 64,440 pass(es). View

Fixed 1-4 + doc-block

andypost’s picture

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

All the newlines got adressed

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

fago’s picture