Complete conversion of comment form validation to entity validation

#2227503: Apply formatters and widgets to Comment base fields is doing a lot there already, but we have issues with the uid/author behavior, I don't know how to fix that.

Is this a critical if we have a critical meta? I'd think that'd make this a major?
Also postponed on issue in OP

Discussed with catch

Added proposed handler to issue summary - I see there are three, and one hunk that belongs in a pre-save handler (is not to do with validaton)

Taking this on, doesn't need that issue to progress from what I can see

handler 3 already exists

WIP, doesn't touch 'date' yet - @Berdir indicated there are fixes in #2227503: Apply formatters and widgets to Comment base fields for that, so posting what I have and moving over there

+++ b/core/modules/comment/src/Plugin/Validation/Constraint/CommentAuthorConstraintValidator.php
@@ -0,0 +1,34 @@
+      // @todo Properly inject dependency https://drupal.org/node/2197029
+      $accounts = \Drupal::entityManager()->getStorage('user')->loadByProperties(array('name' => $author_name));

If you want to inject dependencies, just implement \Drupal\Core\Plugin\ContainerFactoryPluginInterface::create

more work

fixes #11
#9 see #2197029: Allow to inject dependencies into validation constraints

still to do - date

Also think #2227503: Apply formatters and widgets to Comment base fields will come back green now - but some questions around UX

+++ b/core/modules/comment/src/Entity/Comment.php
@@ -541,7 +541,7 @@ public function getOwner() {
-    return $this->get('uid')->target_id;
+    return (int) $this->get('uid')->target_id;

this ensures the === 0 in the validator works, because "0" (from form submissions) !== 0

+++ b/core/modules/comment/src/Plugin/Validation/Constraint/CommentAuthorConstraintValidator.php
@@ -21,19 +21,10 @@ class CommentAuthorConstraintValidator extends ConstraintValidator {
+      $anonymous_contact = $items->getEntity()->getCommentedEntity()->get($items->getEntity()->getFieldName())->getFieldDefinition()->getSettings()['anonymous'];

protected get6X() helper would be great!

1. +++ b/core/modules/comment/src/Entity/Comment.php
   @@ -540,7 +541,7 @@ public function getOwner() {
      public function getOwnerId() {
   -    return $this->get('uid')->target_id;
   +    return (int) $this->get('uid')->target_id;
   

   needs comment/todo why limiting type

2. +++ b/core/modules/comment/src/Plugin/Validation/Constraint/CommentAuthorConstraintValidator.php
   @@ -0,0 +1,33 @@
   +class CommentAuthorConstraintValidator extends ConstraintValidator {
   ...
   +  public function validate($items, Constraint $constraint) {
   ...
   +    if ($items->getEntity()->getOwnerId() === 0) {
   

   why not check that entity implements getOwnerId() method?

3. +++ b/core/modules/comment/src/Plugin/Validation/Constraint/CommentAuthorConstraintValidator.php
   @@ -0,0 +1,33 @@
   +    /** @var \Drupal\Core\Field\FieldItemList $items */
   +    $author_name = $items->first()->value;
   ...
   +      if (empty($author_name) && $anonymous_contact == COMMENT_ANONYMOUS_MUST_CONTACT) {
   

   author name used latter and isEmpty() looks more reasonable

Thanks @andypost will do

Tagging for today's office hours

Taking a look

This (hopefully) addresses the points in #17 Didn't get time to look at the fail, sorry.

Rolled back the isEmpty() change as it caused more fails.

This should fix the failing test. Just date left now.

Legend! thanks

+++ b/core/modules/comment/src/CommentForm.php
@@ -266,9 +266,11 @@ public function validate(array $form, FormStateInterface $form_state) {
+        $entity->name->value = $form_state->getValue('name');

This should happen in ::buildEntity? Which should in turn trigger the validation in ::validate(). I wonder why that isn't happening

Fixes #28
CommentForm wasn't using buildEntity in the same fashion as other entity forms, is now.

Now onto date

didn't mean that

Does date and the rest

Over are patch look good. Just some minor issues. I think it is ready.

1. +++ b/core/modules/comment/src/CommentForm.php
   @@ -247,30 +247,29 @@ protected function actions(array $form, FormStateInterface $form_state) {
   +    $entity = $this->buildEntity($form, $form_state);
   

   Let's call it comment. Comment entity and commented entity is already very confusing imo.

2. +++ b/core/modules/comment/src/Entity/Comment.php
   @@ -540,7 +541,9 @@ public function getOwner() {
   +    return (int) $this->get('uid')->target_id;
   

   I think it is fixed by #2232477: Fatal when adding new fields with NOT NULL constraints in a base table that contains existing entities.

3. +++ b/core/modules/comment/src/Plugin/Validation/Constraint/CommentAuthorConstraintValidator.php
   @@ -0,0 +1,71 @@
   +    if (($entity = $items->getEntity()) &&
   

   This is a comment right? Can we rename it?

4. +++ b/core/modules/comment/src/Plugin/Validation/Constraint/CommentAuthorConstraintValidator.php
   @@ -0,0 +1,71 @@
   +        $anonymous_contact = $this->getAnonymousContactSetting($entity);
   

   I confused it with core contact entity can we rename this?

5. +++ b/core/modules/comment/src/Plugin/Validation/Constraint/CommentAuthorConstraintValidator.php
   @@ -0,0 +1,71 @@
   +      elseif (!$entity->isNew() || (!\Drupal::currentUser()->isAnonymous()) && $author_name) {
   

   You can use isAuthenticated instead.

6. +++ b/core/modules/comment/src/Tests/CommentValidationTest.php
   @@ -36,6 +37,10 @@ protected function setUp() {
   +    $user = entity_create('user', array('name' => 'test'));
   

   User::create

Fixed #32 except for #2 - see the comment in the code for the reasoning.

Thank you for the fixes.

1. +++ b/core/modules/comment/src/CommentForm.php
   @@ -247,30 +247,29 @@ protected function actions(array $form, FormStateInterface $form_state) {
   -      if ($date instanceOf DrupalDateTime && $date->hasErrors()) {
   -        $form_state->setErrorByName('date', $this->t('You have to specify a valid date.'));
   -      }
   ...
   +      // @todo Remove when https://www.drupal.org/node/2395831 or
   +      //   https://www.drupal.org/node/2227503 is in.
   +      $result = $comment->created->validate();
   +      if ($result->count()) {
   +        $form_state->setErrorByName('date', $result->offsetGet(0)->getMessage());
          }
   ...
   +      if ($comment->name->value) {
   

   This looks untested.

2. +++ b/core/modules/comment/src/CommentForm.php
   @@ -247,30 +247,29 @@ protected function actions(array $form, FormStateInterface $form_state) {
   -      if ($form_state->getValue('name') && !$form_state->getValue('is_anonymous') && !$account) {
   -        $form_state->setErrorByName('name', $this->t('You have to specify a valid author.'));
   

   Where has this validation gone?

Interdiff for 37

+++ b/core/modules/comment/src/CommentForm.php
@@ -201,12 +201,6 @@ public function form(array $form, FormStateInterface $form_state) {
-    $form['is_anonymous'] = array(
...
-      '#value' => ($comment->id() ? !$comment->getOwnerId() : $this->currentUser->isAnonymous()),

@@ -243,43 +237,10 @@ protected function actions(array $form, FormStateInterface $form_state) {
-    elseif ($form_state->getValue('is_anonymous')) {

is this really gone?

+++ b/core/modules/comment/src/Plugin/Validation/Constraint/CommentNameConstraintValidator.php
@@ -19,18 +20,60 @@ class CommentNameConstraintValidator extends ConstraintValidator {
+    if (isset($author_name) && $author_name !== '' && $comment->getOwnerId() === 0) {
...
+    elseif (isset($author_name) && $author_name !== '' && $comment->getOwnerId()) {
...
+    if ($comment->getOwnerId() === 0 && empty($author_name)) {

Type checking could fail. Suppose better to use isAnonymous() on author entity.
Also 0 has special constant.

is this really gone?

grep says yes

so if current user switched somehow between form build and validate then form processing is wrong

+++ b/core/modules/comment/src/Plugin/Validation/Constraint/CommentNameConstraintValidator.php
@@ -19,18 +20,60 @@ class CommentNameConstraintValidator extends ConstraintValidator {
+    if ($comment->getOwnerId() === 0 && empty($author_name)) {
+      if ($this->getAnonymousContactDetailsSetting($comment) == COMMENT_ANONYMOUS_MUST_CONTACT) {

Could be combined into one if.

So those two minor changes and I think this is RTBC again. I'll talk someone through it during tomorrow's critical office hours.

Manually tested removal of the old is_anonymous - works great

or I'll just do it now :)
fixed #39 and #42

We can't use getOwner()->isAnonymous() because sometimes the user entity is not set.

I think this is rtbc again

So @fago +1ed it so nothing functional just extreme nits

1. +++ b/core/modules/comment/src/Plugin/Validation/Constraint/CommentNameConstraint.php
   @@ -19,6 +19,8 @@
   +  public $messageNameTaken = 'The name you used (%name) belongs to a registered user.';
   +  public $messageRequired = 'You have to specify a valid author.';
   +  public $messageMatch = 'The specified author name does not match the comment author.';
   

   @var blocks missing.

2. +++ b/core/modules/comment/src/Plugin/Validation/Constraint/CommentNameConstraintValidator.php
   @@ -19,18 +20,59 @@ class CommentNameConstraintValidator extends ConstraintValidator {
   +    $comment = $items->getEntity();
   +    /** @var CommentInterface $comment */
   

   These can switch places.

3. +++ b/core/modules/comment/src/Plugin/Validation/Constraint/CommentNameConstraintValidator.php
   @@ -19,18 +20,59 @@ class CommentNameConstraintValidator extends ConstraintValidator {
   +    /** @var CommentNameConstraint $constraint */
   

   I don't see this var anywhere? Maybe stale comment. Or move it near the use of var :)

4. +++ b/core/modules/comment/src/Plugin/Validation/Constraint/CommentNameConstraintValidator.php
   @@ -19,18 +20,59 @@ class CommentNameConstraintValidator extends ConstraintValidator {
   +      $this->getAnonymousContactDetailsSetting($comment) == COMMENT_ANONYMOUS_MUST_CONTACT) {
   

   Can we use === here?

5. +++ b/core/modules/comment/src/Plugin/Validation/Constraint/CommentNameConstraintValidator.php
   @@ -19,18 +20,59 @@ class CommentNameConstraintValidator extends ConstraintValidator {
   +   * Gets the anonymous contact details setting for the entity.
   

   Gets the anonymous contact details setting form the comment.

6. +++ b/core/modules/comment/src/Plugin/Validation/Constraint/CommentNameConstraintValidator.php
   @@ -19,18 +20,59 @@ class CommentNameConstraintValidator extends ConstraintValidator {
   +   * @param \Drupal\Core\Entity\EntityInterface $entity
   ...
   +  protected function getAnonymousContactDetailsSetting(CommentInterface $entity) {
   

   @param type mismatch. Can we please rename it to comment?

7. +++ b/core/modules/comment/src/Plugin/Validation/Constraint/CommentNameConstraintValidator.php
   @@ -19,18 +20,59 @@ class CommentNameConstraintValidator extends ConstraintValidator {
   +      ->getSettings()['anonymous'];
   

   I think we can use getSetting('anonymous') here

Fixes ##4

Thanks for the fixes. As per #46 it is RTBC.

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

+++ b/core/modules/comment/src/CommentForm.php
@@ -243,43 +237,10 @@ protected function actions(array $form, FormStateInterface $form_state) {
   /**
-   * Overrides Drupal\Core\Entity\EntityForm::validate().
-   */
-  public function validate(array $form, FormStateInterface $form_state) {

Lovely.

This also opens the door to interesting new comment UIs and mechanisms in (headless) D8, without having to reimplement all validation :) Great work!

Nice! This one fixed all remaining exceptions in #2356967: PostgreSQL: Fix tests in comment test group and just leaves 2 fails open in new CommentRestExportTest tests. Great!