Apply formatters and widgets to Comment base fields

Fixing those test fails.

Fixing the remaining test fails.

The access stuff should be handled through field access, but we're waiting on #2029855: Missing access control for user base fields for that to become easier.

Patches would help.

1. +++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
   @@ -259,21 +259,35 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +      ->setSetting('max_length', 64)
   

   Should we really care about 60 vs 64 characters? Keeping the current UI is important, but 4 characters more or less really doesn't make much of a difference?

2. +++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
   @@ -259,21 +259,35 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    
   +    // @todo: use timestamp widget after https://drupal.org/node/2226493 lands.
        $fields['created'] = FieldDefinition::create('created')
   

   Yeah, I was wondering if we should try to get the new widgets/formatters in separately because node is quite a tough issue. But then we wouldn't have test coverage for them...

Need to reroll.

@e0ipso looks like it's been a while, so unassigned, just comment if you want it back!

I'm at NYC Camp sprint right now, so going to have a look and see what I can do with it.

One test fails, because the comment form not posted after submit. I got this error message after submit
"This value should be of the correct primitive type."
I found out that, this row cause the problem:

<?php
$form['author']['homepage']['widget'][0]['value']['#access'] = $is_admin || ($this->currentUser->isAnonymous() && $anonymous_contact != COMMENT_ANONYMOUS_MAYNOT_CONTACT);
?>

This field get FALSE value, but I don't have enough experience so I don't have idea what cause the problem.

This will fail. It's just the re-roll for now. Applied #8, then manually applied interdiff from #10, and fixed a merge conflict.

Fixed at least one test here.

This needs more than just fixing the tests. When I manually go to :
/comment/reply/node/1/comment

1. I get an AJAX error.
2. When I submit the comment, I get: "This value should be of the correct primitive type."

#2 was introduced by the interdiff in #10.

I'm doing conversion of comment subject field in #2292821: Use widget for comment subject field
But my one changes a migration path so probably better to fix subject field here and proceed with migration in the related

Here's a patch for comment subject field only

#2292821: Use widget for comment subject field landed

Wrongly filed patches to #2226493-95: Apply formatters and widgets to Node base fields

Initial patch, also converts name field

PS: pushed to github

should fix reply form

homepage validation broken somehow (empty homepage does not pass validation via "uri" widget)

another try

This won't pass yet, but this is cleaning up a lot of stuff. Also adding access control. uid/name certainly needs more work, we'll see about other things.

Fixing some tests.

Most remaining fails are related to uid/name behavior. Not sure yet how to make sense of that.

+++ b/core/modules/rdf/src/Tests/CommentAttributesTest.php
@@ -155,9 +155,9 @@ public function testCommentRdfaMarkup() {
     // Posts comment #2 as anonymous user.
     $anonymous_user = array();
-    $anonymous_user['name'] = $this->randomMachineName();
-    $anonymous_user['mail'] = 'tester@simpletest.org';
-    $anonymous_user['homepage'] = 'http://example.org/';
+    $anonymous_user['name[0][value]'] = $this->randomMachineName();
+    $anonymous_user['mail[0][value]'] = 'tester@simpletest.org';
+    $anonymous_user['homepage[0][value]'] = 'http://example.org/';
     $comment2 = $this->saveComment($this->node->id(), 0, $anonymous_user);

Do we need this? It gets passed to entity_create().

Will try it.

I was surprised to see more errors. I think it needs to be re-rolled due to: #2287071: Add cacheability metadata to access checks

Lets wait on #2098419: Missing default access for all comment fields.

Ok, let's see how good the access checks are that were added there :)

Completed beta phase template in issue summary

And re-rolled whilst at it

Install works, there's still 2 debug() calls, and form needs some love:

Fix few tests, email widget changed (size setting, looks a feature)

Rerolled. Conflict was on comment.info.yml, not a big deal.

+++ b/core/modules/comment/src/CommentForm.php
@@ -123,42 +107,16 @@ public function form(array $form, FormStateInterface $form_state) {
-    $form['author']['name'] = array(
-      '#type' => 'textfield',
-      '#title' => $this->t('Your name'),
-      '#default_value' => $author,
-      '#required' => ($this->currentUser->isAnonymous() && $anonymous_contact == COMMENT_ANONYMOUS_MUST_CONTACT),
-      '#maxlength' => 60,
-      '#size' => 30,
-    );

If we remove this, there is no author name field for anonymous users.

Does that mean we need a new widget?

Not a full review, but :

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/EmailDefaultWidget.php
@@ -29,6 +30,7 @@ class EmailDefaultWidget extends WidgetBase {
   public static function defaultSettings() {
     return array(
+      'size' => 60,
       'placeholder' => '',
     ) + parent::defaultSettings();
   }

We have to grow a habit of updating config schemas accordingly :-) (field.widget.settings.email_default here)

re-roll and fixes #63

patch would help

Found #2405675: Comment field settings don't save and #2405691: CommentAccessControlHandler checks for an invalid setting (anonymous_contact) the second of which blocks this

This includes #2405691: CommentAccessControlHandler checks for an invalid setting (anonymous_contact)

Forgot to merge

More work - this *just* might be green.

Now we have an issue with the admin-user seeing too many fields on the comment-add form (see screenshot).

And anytime we want to change values, we have to submit both uid and name - including when making the comment anonymous (see interdiff).

Also found that the CommentNameValidatorConstraint does a === against 0, but it's actually "0", so added a cast to the Comment::getOwnerId() method (see interdiff).

So we need to decide how to handle the duel fields and what the admin sees I think.

@Wim #74 explains that there only 2 problems with patch currntly:
1) for UID1 access does apply that's why we see name and uid fields both
2) patch needs some love to properly alter form widgets to keep previous look

Why can't that be done here?

shell be done :)
The problem 1) just needs more time

discussed at irc

larowlan> andypost: actually everything tagged as 'administrativeFields' in the commentaccesscontrolhandler should not be editable if $entity->isNew()
larowlan> andypost: so when $items is NULL or $items->getEntity()->isNew() is false
andypost> larowlan, hm, probably, but comment:create() does not care about access
larowlan> andypost: right, I'm not talking about create, I'm talking about access to widgets
andypost> aha
larowlan> andypost: the widgets are shown based on CommentAccessControlHandler::checkFieldAccess
andypost> larowlan, I just about that after entity created with initial values from form, the constraints should be invalidated
larowlan> andypost: the issue is the '$administrative_fields' should only be editable if the entity has been saved
larowlan> andypost: but you're right, we'd need default values for all of those - for REST as well

So
1) default values for fields
2) fix checkFieldAccess()
3) tune-up form
4) check test coverage

no interdiff, merged manually

a lot of code gone in comment form

Fixed tests except CommentBlockTest no idea why getSubject returns nothing

Here's few more fixes, still failed tests:
1) subject is not generated from comment body (CommentBlockTest shows that)
2) CommentTranslationUITest because now there's 2 fields (uid and name) still and changing name does not change author

Subject still needs generation, added access for elements

Maybe file separate issue to discuss UI regression - now there's 2 fields Name and Author?

Maybe file separate issue to discuss UI regression - now there's 2 fields Name and Author?

I thought we resolved in #78 that we could fix that with access handlers?

But access handlers does not help here because $is_admin needs both fields to allow admin change comment to anonymous and set name field

EDIT commited #2398073: Admin should not be able to edit email of authenticated commenters

Patch rerolled.

Patch rerolled.

Re-rolled!

The last patch needs to reroll.

Here it is the rerolled patch.

reroll

1. +++ b/core/modules/comment/comment.info.yml
   @@ -5,5 +5,6 @@ package: Core
   +  - entity_reference
   

   Is that really still needed ? Most of the functionnality about ER fields now lives in core

2. +++ b/core/modules/comment/src/CommentForm.php
   @@ -120,7 +123,32 @@ public function form(array $form, FormStateInterface $form_state) {
   +    $form['uid']['widget'][0]['target_id']['#required'] = ($this->currentUser->isAnonymous() && $anonymous_contact == COMMENT_ANONYMOUS_MUST_CONTACT);
   +    if ($this->currentUser->isAuthenticated() && !$is_admin) {
   +      $form['uid']['widget'][0]['target_id']['#type'] = 'item';
   +      $form['uid']['widget'][0]['target_id']['#value'] = $this->currentUser->id();
   +      $form['uid']['widget'][0]['target_id']['#theme'] = 'username';
   +      $form['uid']['widget'][0]['target_id']['#account'] = $this->currentUser;
   +    }
   ...
   +    $form['mail']['widget'][0]['value']['#required'] = ($this->currentUser->isAnonymous() && $anonymous_contact == COMMENT_ANONYMOUS_MUST_CONTACT);
   

   The problem with code like that is that it hardcodes on the inner structure of the widget. And the field definitions make that widget configurable/changeable in Field UI (->setDisplayConfigurable('form', TRUE);)

3. +++ b/core/modules/comment/src/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php
   @@ -182,6 +182,7 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
            if ($this->currentUser->hasPermission('post comments')) {
   +
              $output['comment_form'] = [
   

   Needless hunk, one file less to change ;-)

1. +++ b/core/config/schema/core.entity.schema.yml
   @@ -167,6 +167,9 @@ field.widget.settings.email_default:
    
   

   This is the only thing that concerns me - I think it's probably OK before RC, but it'll break default configuration afterwards. So needs a change record, and might be good to split out to a separate major issue to do quickly.

2. +++ b/core/modules/comment/comment.info.yml
   @@ -5,5 +5,6 @@ package: Core
   +  - entity_reference
   

   This needs an upgrade path. Also we'd be removing it again if we do the entity reference ashes scattering. Again that seems fine before RC but trickier afterwards (would really like to minimize update hooks added during RC since more people will run them and less excuses if they break).

Apart from that everything here looks eligible for a minor release, but not a patch release, so my personal opinion is if it gets in before RC then that'd be fantastic, but if it doesn't we'll need to move to 8.1.x.

Tagging RC deadline to reflect that.

Also :

+++ b/core/modules/comment/src/Entity/Comment.php
@@ -262,27 +262,59 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+      ->setSetting('handler', 'default')

Shameless plug : that line shouldn't be needed after #2578249: Some e_r fields get the wrong Selection handler

Spun the schema stuff off into #2578741: Add setting for size to email widget

Changing to 8.1 per #106

Unassigned as not recently worked on

Hello guys,

reroll is ready

Re-rolling patch for 8.3.x

+++ b/core/modules/comment/src/Tests/CommentTestBase.php
@@ -106,7 +105,7 @@ protected function setUp() {
+  public function postComment($entity, $comment, $subject = '', $contact = array(), $field_name = 'comment') {

So $contact should always be an array(). Because later on, on line #134:

<?php
if ($contact !== NULL && is_array($contact)) {
?>

Complete re-roll of patch #122

The subject field is using a widget now, but it's not using a formatter, so we should either add that here or spin-off another issue.

Reroll the patch in #130 to work on latest 8.6.x

Can we get an issue summary update of the remaining tasks here - I see refs to addressing code reviews in the current remaining tasks - have they been completed?
I also see tags for update path and tests - should we add those to the remaining tasks?

I updated the IS to match the latest patch and to reference new issue #3090187: Mechanism to disable preprocessing of base fields in comment entity type so they can be configured via the field UI which answers #134.

Just a reroll with minor coding standard fixes.

Two pointers:
1)
core/modules/comment/src/Entity/Comment.php
on baseFieldDefinitions() for $fields['uid'] with description "The user ID of the comment author" doesn't have anymore:

->setTranslatable(TRUE)
->setSetting('target_type', 'user')

wonder if they should be there. They where removed in #2949964: Add an EntityOwnerTrait to standardize the base field needed by EntityOwnerInterface.

2)
In core/modules/comment/tests/src/Functional/CommentPreviewTest.php function testCommentEditPreviewSave() makes following change

-    $edit['date[date]'] = $date->format('Y-m-d');
-    $edit['date[time]'] = $date->format('H:i:s');
+    $edit['created[0][value][date]'] = $date->format('Y-m-d');
+    $edit['created[0][value][time]'] = $date->format('H:i:s');

but keeps :

$displayed['date[date]'] = current($this->xpath("//input[@id='edit-date-date']"))->getValue();
$displayed['date[time]'] = current($this->xpath("//input[@id='edit-date-time']"))->getValue();

wonder if that's correct?

// MODULENAME.module

/**
 * Implements hook_entity_base_field_info_alter().
 */
function MODULENAME_entity_base_field_info_alter(&$fields, EntityTypeInterface $entity_type) {
  if ($entity_type->id() == 'comment') {
    /** @see \Drupal\comment\Entity\Comment::baseFieldDefinitions() */
    $fields['uid']->setDisplayConfigurable('view', TRUE);
    $fields['created']->setDisplayConfigurable('view', TRUE);
  }
}

Original: http://xandeadx.ru/blog/drupal/981

Re-roll patch crated for 9.3.x.

Fixed custom command fail issue.

There are 200 odd test fails here https://dispatcher.drupalci.org/job/drupal_patches/94678/

We should be making the comment title part of the display mode configuration as well so it can be hidden (this used to be configurable via a 'show subject' option but that's no longer in Drupal 9).

@catch The comment title is being worked on as part of #2353867: [META] Expose Title and other base fields in Manage Display, especially #3090187: Mechanism to disable preprocessing of base fields in comment entity type so they can be configured via the field UI. The complexities are mostly not specific to the entity type.

rerolling the patch for the Drupal 10.1.x version and made it compatible with the version and made some improvements to the patch, along with the patch attaching the reroll_diff, Please review it and suggest if need to update something.

Fixing the CCF

Tried to fix CCF.
Please review.

Seems to be having failures going back to #148

Also was previously tagged for upgrade path and upgrade test which still need to happen

Did not test issue.