Problem/Motivation

Comment has basefields uid, name and mail.

  • When the user is anonymous the name and mail fields are used to store user contact info and uid is 0.
  • When the user is authenticated the name and mail field are not used at all and uid is set to user ID.

Proposed resolution

Make sure mail and name fields are NULL when the comment uid is not 0.

Remaining tasks

  • Do we need to update the exsiting data? We'll need a update hook and upgrade path test.
  • Review.
  • RTBC.
  • Commit.
  • Rejoice.

User interface changes

None

API changes

None but this is kind of API clean up we'll no longer store the name and mail of a comment which belongs to authenticate user.

Data model changes

None

Original report

Related to #1986606: Convert the comments administration screen to a view.

When a comment is accessed from a view, there is no obvious way to display the author's name, URL and email that works consistently for anonymous and registered users. To add to the confusion, fields within the database that appear to contain meaningful data (name and mail) are actually undefined if the uid points to a non-anonymous user. When uid points to a real user, this information should come from the user entity.

For historical reference, the issue was originally reported as:

When a comment is edited, the user name is blanked out in the database for the comment entity. This does not seem to affect the display of comments on the entity display itself nor on the comment edit form. However, it causes Views to be unable to display the author name. @andypost provided this screenshot of the database after editing some comments.

Steps to reproduce
  1. Install Standard.
  2. Create a comment view using fields, and add the comment author field.
  3. Create an article node.
  4. Add a comment on the article.
  5. Visit the view and confirm that the username is displayed.
  6. Edit the comment and change something, e.g. the comment body.
  7. Visit the view. The username disappears.
  8. Perform a full cache clear. The bug still persists.

Proposed resolution

  • Force the undefined fields to NULL so that people can easily spot misuse of them through tests.
  • Make sure that the relevant plugins are available and working so that people can extract the Author Name, homepage (and email?) in a consistent way. This is possibly the highest priority bit since it blocks #1986606: Convert the comments administration screen to a view. Because of that we might want to split out the labelling as a sub-issue. The other pieces are needed for testing.
  • Test coverage for all of the above.

Remaining tasks

  1. Identify plugins needed, as they appear in Views (the highest priority being whatever blocks #1986606: Convert the comments administration screen to a view.
  2. Force the undefined database values to NULL, along with adequate test coverage (this is in progress).
  3. Write the missing plugins.
  4. Add tests.

User interface changes

The Author Name now has slightly different rendering options, reflecting a change to the how the field is treated.

API changes

None expected.

Data model changes

Should we blank out existing data in the fields with undefined meaning?

CommentFileSizeAuthor
#87 interdiff-009ed0.txt5.01 KBjibran
#87 values_of_name-2614504-87.patch6.95 KBjibran
#85 values_of_name-2614504-85.patch5.7 KBjibran
#85 interdiff-c1a8d2.txt1.48 KBjibran
#81 values_of_name-2614504-81.patch4.91 KBjibran
#75 interdiff-2614504-74-75.txt2.73 KByogeshmpawar
#75 views_name_field_is-2614504-75.patch10.49 KByogeshmpawar
#74 interdiff72_74.txt5.46 KBMunavijayalakshmi
#74 views_name_field_is-2614504-74.patch10.5 KBMunavijayalakshmi
#72 views_name_field_is-2614504-72.patch10.57 KByogeshmpawar
#70 interdiff-2614504-68-70.txt508 bytesmohit_aghera
#70 2614504-70.patch10.57 KBmohit_aghera
#68 2614504-68.patch10.34 KBmohit_aghera
XqxaPSm.png14.36 KBxjm
#4 2614504-2.patch1.49 KBandypost
#7 2614504-7.patch1.49 KBthpoul
#7 interdiff-2614504-2-7.txt580 bytesthpoul
#11 2614504-11-scenario-1-failing-test.patch2.16 KBblackra
#13 2614504-12-scenario-2-failing-test.patch2.39 KBblackra
#22 interdiff.txt1.81 KBlarowlan
#22 2614504-comment.20.patch3.03 KBlarowlan
#26 2614504-26-comment-author-in-field-views.patch4.15 KBblackra
#26 2614504-26-inderdiff.txt6.39 KBblackra
#27 2614504-27-inderdiff.txt6.67 KBblackra
#27 2614504-27-comment-author-in-field-views.patch4.43 KBblackra
#37 2614504-37-comment-author-in-field-views.patch8.06 KBblackra
#37 2614504-37-interdiff.txt5.46 KBblackra
#40 2614504-40-interdiff.txt2.71 KBblackra
#40 2614504-40-comment-author-in-field-views.patch10.54 KBblackra
#43 2614504-43-field-plugin.patch13.23 KBblackra
#43 2614504-43-interdiff.txt2.68 KBblackra
#49 2614504-49-interdiff.txt11.02 KBblackra
#49 2614504-49-views-name-field-broken.patch22.38 KBblackra
#56 2614504-56.patch11.37 KBlokapujya
#56 interdiff-40-56.txt840 byteslokapujya
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm created an issue. See original summary.

xjm’s picture

Title: Editing a comment blanks out the user name and email for te » Editing a comment blanks out the user name and email in the database
Issue summary: View changes
xjm’s picture

Issue summary: View changes
andypost’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
1.49 KB

Initial stub

xjm’s picture

+++ b/core/modules/comment/src/CommentForm.php
@@ -281,15 +281,15 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
+    if ($author_id > 0) {

We should use !== the anonymous user constant here, or possibly some method somewhere, rather than > 0.

Status: Needs review » Needs work

The last submitted patch, 4: 2614504-2.patch, failed testing.

thpoul’s picture

Updated patch according to @xjm recommendation

blackra’s picture

I have been looking into this in some depth.

The causes of the problem are:

  • Comment user information is recorded in two ways:
    • For registered users, as an entity reference to the User.
    • For anonymous users, as some fields on the Comment.
  • Methods on the comment extract the information from the correct location.
  • Views ignores the method interface and queries the database directly.
  • It is clear from the code that the intent was that the fields on the Comment would be NULL if there was a reference to a registered user.
  • There are related bugs affecting other fields as well as the author name. An example is email, which shows the opposite behaviour.
  • The strange behaviour observed is partly because the $is_admin flag is set to false for new comments, even for users with the relevant admin permissions.

Our options are:

  1. The fix as currently specified. We could make the author name, email, etc mirror the User fields, but they wouldn't track any changes to the User object.
  2. Make the fields NULL for non-anonymous users, and possibly change their display names to indicate that they relate to anonymous users, e.g. 'Anonymous author name'. I don't think changing the actual field names is justifiable at this stage.
  3. Something else :-)
andypost’s picture

@blackra thatx a lot for investigation.
The basic idea "probably" was to store denormalized data only for anonymous user's comments but that breaks views integration (the blocked issue)
So I think there should be changes to views plugins to display author's name and email differently for anonymous and authorized user's comments (3rd)

blackra’s picture

We need to be careful that doesn't get done in a way that has a huge performance hit for cases where a performance hit is not necessary.

blackra’s picture

Here is a failing test case that illustrates the original problem.

andypost’s picture

Status: Needs work » Needs review

Yep, exactly the case, also would be great to extend with non-admin user

+++ b/core/modules/comment/src/Tests/CommentInterfaceTest.php
@@ -216,6 +216,35 @@ public function testCommentInterface() {
+ $this->assertTrue($new_comment->get('name') == $this->adminUser->getAccountName(), 'Comment author name in database not changed by edit.');
+ $this->assertTrue($new_comment->getOwnerId() == $this->adminUser->id(), 'Comment owner not changed by edit.');
Make sense to assert email too

blackra’s picture

FileSize
2.39 KB

In that case I didn't understand your proposed fix. I was about to upload a failing test case for scenario 2, because I thought that was more applicable...

There are two questions:

  • How should the Comment entity storage behave?
  • What should the Views plugin do?

I think those need to be considered and tested separately in order to get the right answer.

blackra’s picture

Note that my failing test patches don't include the content of patches #2 & #7.

andypost’s picture

wouldn't track any changes to the User object.

That's exactly downside of the keeping name and email for auth users
OTOH that breaks views and makes logic to display author of comment confusing

So I'd like to get more opinions and use-cases to fix that properly

andypost’s picture

there's no need to edit comment second time, better to login non-admin user and post new comment and edit

+++ b/core/modules/comment/src/Tests/CommentInterfaceTest.php
@@ -216,6 +216,37 @@ public function testCommentInterface() {
+    $this->assertEqual($comment->getAuthorName(), $this->adminUser->getAccountName(), 'Comment author name returned by API is current user.');
...
+    $this->assertTrue($new_comment->getAuthorName() == $this->adminUser->getAccountName(), 'Comment author name from API not changed by edit.');

Suppose second one is getAuthorEmail()

The last submitted patch, 7: 2614504-7.patch, failed testing.

The last submitted patch, 11: 2614504-11-scenario-1-failing-test.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 13: 2614504-12-scenario-2-failing-test.patch, failed testing.

larowlan’s picture

Assigned: Unassigned » larowlan
Issue tags: -Needs tests

prodding, starting by combining the patch and the test

blackra’s picture

Larowlan, since you are taking this on and the first task is the decision making about the how things are supposed to work, here are some observations from the investigation I was in the middle of:

  • Whichever way the decision is made, it is going to have knock-on effects on things like comment statistics. I don't know what comment statistics currently assumes. This needs investigating.
  • The entity reference to the author is labelled the uid. There is a method in the API that turns the entity reference into a uid. However, views doesn't go through this method so it provides an entity reference (still labelled uid).
  • Right now we don't want API changes, or data model changes if we can help it (although it isn't clear what the current data model is - in clarifying the data model we are probably going to break something, but we get a choice of what to break). However, we also don't want to close the door on doing the right thing in the future.
  • I didn't combine the tests with the patch because it wasn't clear whether the patch was solving the right problem, or solving the problem in the right way. I'm still not clear on this.
  • We will also need tests for the views plugin, assuming the solution involves writing one (at the moment I think there is a chance we might be able to avoid this if we fix another problem with views, but I need to talk to a VDC expert).
larowlan’s picture

Assigned: larowlan » Unassigned
Status: Needs work » Needs review
FileSize
1.81 KB
3.03 KB

some stuff, still some fails

larowlan’s picture

FWIW the comment author name is supposed to use AuthorNameFormatter for outputting the values.
I think there might be an issue here with 0 as opposed to '0'.

We use $author_id === 0

Status: Needs review » Needs work

The last submitted patch, 22: 2614504-comment.20.patch, failed testing.

blackra’s picture

Issue summary: View changes

After discussing this with larowlan on IRC I am updating the issue summary with our current understanding of the issue.

I am pretty sure that the intent of the data model is such that we should always set the fields to NULL when the user is anonymous.

We also need a new title, but I'm not updating that yet.

blackra’s picture

The Storage preSave() calls the Entity preSave() at the end of the ::parent() chain so Comment::preSave() was setting the author name after CommentStorage::preSave() cleared it.

Here is an updated patch with the clear moved to the end of Comment::preSave(), and the tests now run for both an admin and an authenticated user.

blackra’s picture

Uploaded a new version. This one has the test for the mail field too.

The last submitted patch, 26: 2614504-26-comment-author-in-field-views.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 27: 2614504-27-comment-author-in-field-views.patch, failed testing.

blackra’s picture

Well, the failures are interesting.

There is one in hal which doesn't expect a field to be NULL when it is doing data comparison. The test is definitely not right for the comment data model.

There is one in CommentUserNameTest which is probably caused directly by a problem with the data model.

There is one in CommentLockTest where execution does not complete. I have no idea why.

blackra’s picture

It just occurred to me to wonder whether setting the values to an empty string would be more friendly to existing code than setting them to NULL. Does anybody else have an opinion on this?

andypost’s picture

I agree that better to save NULL into name and mail fields.
OTOH formatters need fix because they get no field items to render

  1. +++ b/core/modules/comment/src/Entity/Comment.php
    @@ -138,15 +138,16 @@ public function preSave(EntityStorageInterface $storage) {
    +    // The entity fields for name and mail have no meaning if the user is not
    +    // Anonymous. Set them to NULL to make it clearer that they are not used.
    

    ++ would be great to add @see pointing where name and mail set for anonymous

  2. +++ b/core/modules/comment/src/Tests/CommentInterfaceTest.php
    @@ -216,6 +217,50 @@ public function testCommentInterface() {
    +      $this->drupalLogout();
    

    is not needed

blackra’s picture

@andypost Ok. I'm not up to speed on how Views formatters work. Do we also need corresponding filters and sorters? I'm using this issue as an excuse to read up on the Views 8 Plugin API...

  1. I agree that we should have a comment pointing where name and mail set for anonymous, but it should be 'See', not '@see'. It is not a doc comment (and shouldn't be).
  2. The $this->logout() isn't strictly needed, but it is good defensive practice in automated tests. If the login() on the next loop failed for some reason (e.g. a typo in the user setup) you could end up logged in as the wrong user. Since I've already been caught out once this week by tests passing due to being run as the wrong user, I think it should stay. From a purist point of view this should never be an issue because the loop should be accomplished by using a provider. However, the setup cost is such that I chose a loop and a logout() instead.
andypost’s picture

@blackra both points makes sense but anyway we need the same testing for anonymous comments edited by admin

About views filters - looks we need to fix them too because for anonymous we will use c.name like ... but for authorized join users u ... u.name like ...
The same for emails.

So this is a price for denormalization and need to make #1986606: Convert the comments administration screen to a view into core and make it usable for people

blackra’s picture

I'm currently working on: adding the anon/admin test; fixing the hal failing test, and I will fix up the comment in preSave() too.

Do we need to do anything to the comment statistics, or do we think they are okay?

andypost’s picture

Suppose no, last_comment_name is already denormalized and should be used (hm, probably that should be a user-display-name then)

blackra’s picture

Status: Needs work » Needs review
FileSize
8.06 KB
5.46 KB

Here are the additional tests and the comment. The problem with hal has not been fixed yet so I expect 3 test failures.

Status: Needs review » Needs work

The last submitted patch, 37: 2614504-37-comment-author-in-field-views.patch, failed testing.

lokapujya’s picture

Does the user entity reference need to get over to the AuthorNameFormatter plugin? So that the formatter can look up the name (now that it's not stored in the comment.

blackra’s picture

Here is a patch to attempt to fix the hal and locking test-case bugs. It also contains a rewording of the field descriptions for name and mail.

@lokapujya, it is probably not quite that simple. The AuthorNameFormatter is already calling Comment::getOwner() which turns the uid into a user entity object for non-anonymous users. Why is this not already doing what we need?

What I'd really like to do is use a field plugin, but I suspect you can't override a field that already exists.

Status: Needs review » Needs work

The last submitted patch, 40: 2614504-40-comment-author-in-field-views.patch, failed testing.

blackra’s picture

Now the only test failing is the one that should be failing at this stage.

When the name field is empty, it is excluded from the FieldItemList being passed to the AuthorNameFormatter, so the formatter has nothing to work with.

blackra’s picture

Status: Needs work » Needs review
FileSize
13.23 KB
2.68 KB

Here is a first attempt at creating a field plugin. This does make the Author field in view show the correct result. However, it loses its formatter and I can't work out how to re-attach it. It also doesn't sort nicely.

Expecting about 5 test failures.

Status: Needs review » Needs work

The last submitted patch, 43: 2614504-43-field-plugin.patch, failed testing.

larowlan’s picture

We discussed on irc that author name formatter shouldn't work for the name field, because its meant for ER fields only, and that is the 'uid' field, not the 'name' field.

'name' field is a string.

Perhaps might even be the cause of the whole issue here, I suggest we remove the name field from views integration, and instead use uid field.

blackra’s picture

We discussed on IRC that because there are multiple values involved (name, email, homepage...) this very complex to handle with a formatter and it would do horrible things to the API (including an unpleasant BC break)

A question was raised about SQL support for COALESCE. This is already used in multiple places in comments and comment statistics. Also used in the search module. It is in the SQL' 92 standard and a quick check shows support in MySQL, MariaDB, PostgreSQL, SQLite, Oracle, DB2, Microsoft SQL Server.

We have decided to continue with the field plugin approach.

andypost’s picture

catch’s picture

Title: Editing a comment blanks out the user name and email in the database » Views 'name' field is broken when there's a uid
blackra’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
11.02 KB
22.38 KB

This patch adds a sorter and a filter plus some configuration options for rendering the name as a link for authenticated users and/or anonymous users.

This needs tests. Note that CommentUserNameTest still fails because the name of an anonymous user is text by default and you have to change a setting to enable links to homepages.

I'm probably not going to get a chance to do any more work on this issue for a week or so.

On reflection, I'm not sure whether we need to do the same work to the email field. That field should never be exposed publicly.

Status: Needs review » Needs work

The last submitted patch, 49: 2614504-49-views-name-field-broken.patch, failed testing.

dawehner’s picture

  1. +++ b/core/modules/comment/src/Plugin/views/field/AuthorName.php
    @@ -0,0 +1,135 @@
    + *
    + * This is necessary because the Author's name is contained in the name field
    + * of the comment for anonymous users, but for non-anonymous users the Author's
    + * name is provided by the user object pointed to by the uid.
    + *
    + * @ingroup views_field_handlers
    + *
    + * @ViewsField("comment_author_name")
    + */
    +
    +class AuthorName extends FieldPluginBase {
    +
    +  protected $uid;
    

    So this is what I don't understand. Why do we not implement a field formatter to achieve the same?

  2. +++ b/core/modules/comment/src/Plugin/views/field/AuthorName.php
    @@ -0,0 +1,135 @@
    +    $accountDetails = array(
    +      'uid' => $this->getValue($values, 'uid'),
    +      'name' => $this->getValue($values),
    +    );
    

    Nitpick: we use snake case for local variables

  3. +++ b/core/modules/comment/src/Plugin/views/field/AuthorName.php
    @@ -0,0 +1,135 @@
    +      return $this->getRenderer()->render($username);
    

    You should be able to return a render array directly ... if this is not possible, there is a bug.

  4. +++ b/core/modules/comment/src/Plugin/views/filter/AuthorName.php
    @@ -0,0 +1,136 @@
    +   * @var string $uniqueIdentifier
    +   *   Holds a unique identifier used when pre-compiling the query
    +   */
    +  protected $uniqueIdentifier;
    

    Note: We put the one line description for properties on the line above, see https://www.drupal.org/node/1354

  5. +++ b/core/modules/comment/src/Plugin/views/filter/AuthorName.php
    @@ -0,0 +1,136 @@
    +   * {@inheritdoc}
    +   */
    +  public function uniqueIdentifier() {
    +    if (!isset($this->uniqueIdentifier)) {
    +      $this->uniqueIdentifier = uniqid('AuthorName');
    +    }
    +    return $this->uniqueIdentifier;
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    ...
    +  public function nextPlaceholder() {
    +    return $this->placeholder++;
    +  }
    

    Why can't we use $this->placeholder() directly?

  6. +++ b/core/modules/comment/src/Plugin/views/filter/AuthorName.php
    @@ -0,0 +1,136 @@
    +    // Unfortunately we cannot just put our COALESCE expression in an alias and
    +    // pass it to the operator processing. Aliases are not permitted in an SQL
    +    // WHERE clause. We cannot put the expression directly in the field because
    +    // \Drupal\Core\Database\Query\Condition::compile strips the field in
    +    // operator Conditions (but not formula Conditions).
    

    So why can't we use $this->query->addWhereExpression instead?

  7. +++ b/core/modules/comment/src/Plugin/views/sort/AuthorName.php
    @@ -0,0 +1,46 @@
    +      'table' => 'users_field_data',
    

    Can we avoid hardcoding this tablename there and use the table mapping instead:

     $ drush php
    $Psy Shell v0.5.2 (PHP 5.5.28 — cli) by Justin Hileman
    >>> $mapping = \Drupal::entityManager()->getStorage('node')->getTableMapping();
    => Drupal\Core\Entity\Sql\DefaultTableMapping {#10649}
    >>> $mapping->
    __construct                   getColumnNames                getDedicatedTableNames        getFieldNames                 getTableNames                 setFieldNames                
    allowsSharedTableStorage      getDedicatedDataTableName     getExtraColumns               getFieldTableName             requiresDedicatedTableStorage
    getAllColumns                 getDedicatedRevisionTableName getFieldColumnName            getReservedColumns            setExtraColumns              
    >>> $mapping->
    __construct                   getColumnNames                getDedicatedTableNames        getFieldNames                 getTableNames                 setFieldNames                
    allowsSharedTableStorage      getDedicatedDataTableName     getExtraColumns               getFieldTableName             requiresDedicatedTableStorage
    getAllColumns                 getDedicatedRevisionTableName getFieldColumnName            getReservedColumns            setExtraColumns              
    >>> $mapping->getFieldTableName('title');
    => "node_field_data"
    >>> $mapping->getFieldTableName('nid');
    => "node_field_data"
    >>> 
    
  8. +++ b/core/modules/comment/src/Plugin/views/sort/AuthorName.php
    @@ -0,0 +1,46 @@
    +    $query = &$this->query;
    

    Query is already an object, we don't need it to create a reference to it.

  9. +++ b/core/modules/comment/src/Plugin/views/sort/AuthorName.php
    @@ -0,0 +1,46 @@
    +}
    \ No newline at end of file
    

    no new newline.

  10. +++ b/core/modules/comment/src/Tests/CommentInterfaceTest.php
    @@ -216,6 +217,127 @@ public function testCommentInterface() {
    +      $this->assertNull($comment->get('name')->value, new FormattableMarkup('Comment author name stored in database is NULL for @user.', $user_placeholder));
    +      $this->assertNull($comment->get('mail')->value, new FormattableMarkup('Comment author email stored in database is NULL for @user.', $user_placeholder));
    +      $this->assertEqual($comment->getAuthorName(), $user->getAccountName(), new FormattableMarkup('Comment author name returned by API is @user.', $user_placeholder));
    +      $this->assertEqual($comment->getOwnerId(), $user->id(), new FormattableMarkup('Comment owner is @user.', $user_placeholder));
    

    i kinda dislike this endless IMHO not really helpful test messages but well, some people seem to liek them.

  11. +++ b/core/modules/comment/src/Tests/CommentInterfaceTest.php
    @@ -216,6 +217,127 @@ public function testCommentInterface() {
    +      $subject_text = $this->randomMachineName();
    +      $comment_text = $this->randomMachineName();
    +      $new_comment_text = $this->randomMachineName();
    +      $new_subject_text = $this->randomMachineName();
    +      $anon_name = $this->randomMachineName();
    

    Let's not use random test data, see #2571183: Deprecate random() usage in tests to avoid random test failures. This test is not a special test, its just a generic test.

blackra’s picture

@dawehner, thanks for taking the time to review this. These are some really helpful comments.

  • #2, #4, #9 - yes. Those need fixing.
  • #3, #11 - valid points. #11 was from following the style of the surrounding class.
  • #1 - I originally tried using a formatter, but it has a few problems, the most serious of which is that the field doesn't exist to be formatted if the underlying database value is NULL (which it is in this case).
  • #5 - This is an implementation of Drupal\Core\Database\Query\PlaceholderInterface used to achieve local numbering of parameters, and actually called from within Drupal\Core\Database\Query\Condition::compile(). Maybe I should have created a separate class for this. Alternatively if we could find a source for the schema object then these could be moved into the query-wide parameter numbering scheme.<\li>
  • #6 - I would love to use addWhereExpression(), but the addWhere() calls are embedded within Drupal\views\Plugin\views\filter\StringFilter and I didn't want to duplicate all of that functionality. The result is a bit more hacky than I would like, although very Drupal. It is functionally equivalent to using hook_alter_query() except that Views has an option to disable hook_alter_query(). The "right" way to do this in the long term is probably to give Drupal\Core\Database\Query\Condition the concept of an operator acting on a formula (or a better way of tracking safe vs unsafe inputs) and then refactor StringFilter accordingly. If anybody has a cleaner way of doing this without rewriting StringFilter and Condition (the operator handling is split between the two of them) I would love to hear it.
  • #7 - I assume this is a valid point, but I have no idea how to implement it.
  • #8 - The mistake here is using a type hint instead of a cast. The method requires $this->query to be of type Sql. Do you have any idea why it isn't? Almost every Views plugin assumes that it is.
  • #10 - Almost all of the tests included directly in the test case have been necessary in catching bugs. I think this is a reflection of the convoluted nature of the code being tested. However, some of the indirect tests, e.g. in the postComment() helper, do not really add anything on their 20th invocation.

A lot of these things are the best solutions I could come up with, which doesn't necessarily mean I'm happy with them. There are other things I'm not happy with too. For example, I don't like the code duplication in query() for the three different plugins.

lokapujya’s picture

Just putting any value other than NULL, allowed the formatter to do it's thing. If we want to use the formatter, do we need to figure out how to use the formatter even when the value is NULL? I didn't really figure out how the value gets passed along.

andypost’s picture

#53 yep, that's a right way
I just stuck initially how to set proper default value for name and mail fields for authorized.
So NULL would mean to get default value that calculated runtime from \Drupal\user\EntityOwnerInterface from comment

andypost’s picture

lokapujya’s picture

Status: Needs work » Needs review
FileSize
11.37 KB
840 bytes

Can the formatter still do what it needs to do when the database value is empty? something like this patch.

blackra’s picture

Status: Needs review » Needs work

I think we need to be clear about the problem we are trying to solve. Originally the reason for wanting to use a formatter instead of a field was so that we could do it quickly enough to get this into the 8.0 release. We are now too late to do that. It is also too late to solve this without doing the work to create a field plugin.

We have two problems to solve:

  • Fix the bug in 8.0.x (or decide not to).
  • Fix the bug in 8.1.x onwards.

As I see it there are three ways of tackling this, with different pros and cons:

  1. Modify the underlying entity.
    • Pros: it makes using things simple.
    • Cons: it would mean that the entity doesn't map directly onto database fields. It is not clear that there is anything wrong with the current entity model.
  2. Create a field plugin.
    • Pros: this is the closest to making the code represent what we mean. It is also consistent with the refactoring between 8.0.x and 8.1.x
    • Cons: it is a slightly bigger change to 8.0.x
  3. Make the formatter work with the change.
    • Pros: this is slightly less work in 8.0.x if you ignore the work that has already been done.
    • Cons: the formatter no longer exists in 8.1.x. As far as Views is concerned the field is still empty and has the same value for all authenticated users, so there is a lot of potential for this approach to cause bugs.

Note that patch #56 does not address filtering or sorting. The field filters and sorts as NULL.

lokapujya’s picture

It is also too late to solve this without doing the work to create a field plugin. Why is it too late? I'm not grasping the concept.
The formatter no longer exists in 8.1.x. Can you explain or provide a link to issue that removes it?

Note that patch #56 does not address filtering or sorting. The field filters and sorts as NULL.

So, is that the real reason we need a field plugin?

blackra’s picture

@lokapujya my comment was really about making sure we are going in the right direction before we waste any more of my time or yours developing the wrong thing. A lot of the discussions about this have happened on IRC and not been adequately captured in the comments.

The reason I said that it was too late to save on work in my comment was that a field plugin had been developed not once but twice:

  • There was already a field plugin developed by me as part of patch #49 (which only failed testing because the tests hadn't been updated.
  • There is a field plugin in the 8.1.x branch of core. It is clear that the comment module has undergone some refactoring (not just in this area) for 8.1.x. I have no idea why.

However, at the time I wrote comment #57 there might have been reasons why a formatter would end up being the right answer (although this was a change from a previous decision). My question was genuinely a question, because I knew I didn't have all the information.

Anyway, I've been doing some investigation and talking to people on IRC so I have a lot more information, but I'm going to split that out as a new comment.

blackra’s picture

Summary of investigations/observations and a discussion on IRC:

According to "What's next for core patches after 8.0.0?" "[all patches] will be committed first to the 8.1.x git branch".

This causes us a problem because the field formatter does not exist to be patched on the 8.1.x branch and the 8.1.x branch introduces a views field plugin that doesn't exist in the 8.0.x branch.

The outcome of the discussion on IRC was that we should develop the patch based on 8.1.x and then back-port it to 8.0.x. Fortunately the code back-port is relatively straightforward because the plugin doesn't exist on 8.0.x so we can just add it. This does leave us with needing a BC hook to copy the formatter configuration in 8.0.0 to the plugin. I don't know whether the code to do this exists in 8.1.x already.

I'm assuming that this is an allowed level of API change. If not, then the comments module in 8.1.x has big problems regardless of this patch.

A problem that we are going to run into is that a patch for 8.1.x will fail against 8.0.x and vice versa. This raises a question for core maintainers: would you rather have:

  • A patch against 8.1.x developed, RTBC'd and committed followed by a back-port to 8.0.x, or
  • A patch against 8.1.x developed, RTBC'd and then back-ported before trying to commit both? (with the risk that the 8.1.x patch no-longer applies by the time the back-port is ready)

My proposal is that we do the following:

  1. Set the issue version to 8.1.x-dev
  2. Roll a patch based on #49, but discarding its field plugin and creating a new fix for the 8.1.x field plugin (annoyingly neither my work nor lokapujya's is useful here). This should include addressing dawehner's comments from #51
  3. Make sure that there is a BC hook for the formatter configuration option.
  4. Verify that 8.1.x has tests for sorting and filtering. If not, add them.
  5. Back-port to 8.0.x and get it committed to both 8.1.x and 8.0.x What happens here depends on the answer to the question above about workflow.

Also note that the latest 7.x version of Views has a comments plugin with a structure that corresponds to 8.1.x. By eye it looks as if it suffers from the same bug (although nobody would notice unless they looked at a view showing comments by a user that had subsequently changed name).

lokapujya’s picture

This causes us a problem because the field formatter does not exist to be patched on the 8.1.x branch and the 8.1.x branch introduces a views field plugin that doesn't exist in the 8.0.x branch.

I'm still confused about this. I compared 8.0.x to 8.1.x and don't see any changes in the comment module.

andypost’s picture

Version: 8.0.x-dev » 8.1.x-dev
Issue tags: -rc target

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

Issue tags: +Triaged core major

@dawehner, @tim.plunkett, @alexpott, @cilefen, and I discussed this issue and agreed that it is still a major bug, in part because of its impact on #1986606: Convert the comments administration screen to a view. Thanks!

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mohit_aghera’s picture

Status: Needs work » Needs review
FileSize
10.34 KB

Updating patch for 8.4.x branch, it was not getting applied properly.

+++ b/core/modules/hal/src/Tests/EntityTest.php
@@ -198,8 +198,8 @@ public function testComment() {
-    // No value will exist for name as this is only for anonymous users.
-    unset($original_values['cid'], $original_values['hostname'], $original_values['name']);
+    // No value will exist for name or mail as these are only for anonymous users.
+    unset($original_values['cid'], $original_values['hostname'], $original_values['name'], $original_values['mail']);
 

Moreover above snippet from #56 seems refactored in 8.4.x branch.

Status: Needs review » Needs work

The last submitted patch, 68: 2614504-68.patch, failed testing.

mohit_aghera’s picture

Status: Needs work » Needs review
FileSize
10.57 KB
508 bytes

Updating patch to prevent fatal error

Status: Needs review » Needs work

The last submitted patch, 70: 2614504-70.patch, failed testing.

yogeshmpawar’s picture

Status: Needs work » Needs review
FileSize
10.57 KB

Updated patch against test failures & also previous patch failed to apply on 8.4.x branch. surely this patch will pass all the tests.

Munavijayalakshmi’s picture

Assigned: Unassigned » Munavijayalakshmi
Status: Needs review » Needs work
+++ b/core/modules/comment/src/Plugin/Field/FieldFormatter/AuthorNameFormatter.php
@@ -38,6 +38,17 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
+      $elements[0] = array(
+        '#theme' => 'username',
+        '#account' => $account,
+        '#cache' => array(
+          'tags' => $account->getCacheTags() + $comment->getCacheTags(),
+        ),
+      );

+++ b/core/modules/comment/src/Tests/CommentInterfaceTest.php
@@ -220,6 +221,127 @@ public function testCommentInterface() {
+    $users = array(
+      'admin user' => $this->adminUser,
+      'web user' => $this->webUser,
+    );
...
+    $contact_valid_options = array(
+      array(
+          'level' => COMMENT_ANONYMOUS_MAYNOT_CONTACT,
+          'provide_email' => FALSE,
+          'label' => 'COMMENT_ANONYMOUS_MAYNOT_CONTACT',
+        ),
+      array(
+          'level' => COMMENT_ANONYMOUS_MAY_CONTACT,
+          'provide_email' => FALSE,
+          'label' => 'COMMENT_ANONYMOUS_MAY_CONTACT - no email',
+        ),
+      array(
+          'level' => COMMENT_ANONYMOUS_MAY_CONTACT,
+          'provide_email' => TRUE,
+          'label' => 'COMMENT_ANONYMOUS_MAY_CONTACT - email',
+        ),
+      array(
+          'level' => COMMENT_ANONYMOUS_MUST_CONTACT,
+          'provide_email' => TRUE,
+          'label' => 'COMMENT_ANONYMOUS_MUST_CONTACT',
+        ),
+    );
...
+      user_role_grant_permissions(RoleInterface::ANONYMOUS_ID, array(
+          'access content',
+          'access comments',
+          'post comments',
+          'skip comment approval'
+        ));

use short array syntax (new coding standard).

+++ b/core/modules/comment/src/Tests/CommentInterfaceTest.php
@@ -220,6 +221,127 @@ public function testCommentInterface() {
+      // Test editing the comment to make sure the user information stays intact.
...
+      // We don't need to check the updated body/subject because postComment() does.

Line exceeding 80 characters.

+++ b/core/modules/comment/src/Tests/CommentInterfaceTest.php
@@ -220,6 +221,127 @@ public function testCommentInterface() {
+      $user_placeholder = array('@user' => $user_key);

use short array syntax (new coding standard).

Munavijayalakshmi’s picture

Assigned: Munavijayalakshmi » Unassigned
Status: Needs work » Needs review
FileSize
10.5 KB
5.46 KB
yogeshmpawar’s picture

Little more cleanup for coding standard issues.

The last submitted patch, 74: views_name_field_is-2614504-74.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 75: views_name_field_is-2614504-75.patch, failed testing.

jibran’s picture

In my opinion, this a won't fix. As stated before name field is a StringItem and it works fine when the Anonymous user filled it with the name. For authenticated users, we use uid field because they are the author of the comments and when uid is zero we user name field and that's what we are doing in #1986606-352: Convert the comments administration screen to a view. When the name is empty we show uid instead.

The issue described in IS is a non-issue because it is correctly implemented in core.

+++ b/core/modules/comment/src/Plugin/Field/FieldFormatter/AuthorNameFormatter.php
@@ -38,6 +38,17 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
+    if (count($items) == 0) {
+      $comment = $items->getEntity();
+      $account = $comment->getOwner();
+      $elements[0] = [
+        '#theme' => 'username',
+        '#account' => $account,
+        '#cache' => [
+          'tags' => $account->getCacheTags() + $comment->getCacheTags(),
+        ],
+      ];
+    }

This change is simply not correct for string formatter. The admin view got around to it see #1986606-352: Convert the comments administration screen to a view therefore this is a won't fix for me.

andypost’s picture

Title: Views 'name' field is broken when there's a uid » Values of 'name' & 'email' fields should be NULL when comment has author (uid > 0)
Issue summary: View changes

This issue still makes a tons of sense to fix, because it makes data consistent on comment entity

But not sure it blocks [#12165834] because views can use "uid" field to get proper value, but once someone will try to use name field he will find that comments that was created by anonymous and will be edited to set author ... still have anonymous user provided name&email...

+++ b/core/modules/comment/src/Entity/Comment.php
@@ -146,14 +146,17 @@ public function preSave(EntityStorageInterface $storage) {
-      // We test the value with '===' because we need to modify anonymous
-      // users as well.
-      if ($this->getOwnerId() === \Drupal::currentUser()->id() && \Drupal::currentUser()->isAuthenticated()) {
-        $this->setAuthorName(\Drupal::currentUser()->getUsername());
-      }
...
+      // The entity fields for name and mail have no meaning if the user is not
+      // Anonymous. Set them to NULL to make it clearer that they are not used.
+      // For anonymous users see \Drupal\comment\CommentForm::form() for mail,
+      // and \Drupal\comment\CommentForm::buildEntity() for name setting.
+      if (!$this->getOwner()->isAnonymous()) {
+        $this->set('name', NULL);
+        $this->set('mail', NULL);
+      }

this is a main point of the issue!

To set NULL to name&mail fields when comment got author

+++ b/core/modules/comment/tests/src/Unit/Entity/CommentLockTest.php
@@ -69,6 +69,14 @@ public function testLocks() {
+    $anon_user = $this->getMock('Drupal\Core\Session\AccountInterface');
+    $anon_user->expects($this->any())
+      ->method('isAnonymous')
+      ->will($this->returnValue(TRUE));
+    $comment->expects($this->any())
+      ->method('getOwner')
+      ->will($this->returnValue($anon_user));

this exactly the test it for

jibran’s picture

#79 makes sense. Thanks for explaining that @andypost.

jibran’s picture

Priority: Major » Normal
Issue summary: View changes
Issue tags: -VDC, -Needs tests, -Triaged core major
FileSize
4.91 KB

Updated the patch as per #79. This has tests so removing the needs tests tag. This is not blocking the #1986606: Convert the comments administration screen to a view anymore so removing the VDC tag, demoting it to normal and removing triage tag added in #66. I have updated the issue summary as well.

jibran’s picture

Status: Needs work » Needs review

Do we need an update path to clear the exisitng data? FWIW It is not has not effect on displays.

jibran’s picture

Issue summary: View changes

It's a new start so no interdiff.

andypost’s picture

Looks great!

Please elaborate this change

+++ b/core/modules/comment/tests/src/Kernel/Views/CommentUserNameTest.php
@@ -144,7 +150,7 @@ public function testUsername() {
-    $this->assertLink($this->adminUser->label());
+    $this->assertNoLink($this->adminUser->label());

I wondered this change. I'd better used xpath here to be sure that name rendered but without link

jibran’s picture

andypost’s picture

As discussed at IRC - view renders 2 comments so for me it's not clear which comment render we testing

+++ b/core/modules/comment/tests/src/Kernel/Views/CommentUserNameTest.php
@@ -72,12 +72,18 @@ protected function setUp($import_test_views = TRUE) {
       'uid' => $this->adminUser->id(),
       'name' => $this->adminUser->label(),
+      'mail' => 'test@example.com',
...
+    // The entity fields for name and mail have no meaning if the user is not
+    // Anonymous.
+    $this->assertNull($comment->name->value);
+    $this->assertNull($comment->mail->value);

This covers a case when comment created but I'd like to see the case:
1) create comment from anonymous user
2) update comment to authenticated
3) make sure name,mail fields cleared

jibran’s picture

@andypost nice catch. Now we have UnitTest, KTB and BTB for this bug.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

@jibran Thanx a lot! Now it loks perfect

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

catch’s picture

This makes sense in itself. I have one question, which I apologise in advance for.

At the moment, if a comment is edited to an account, then edited back to anonymous, the name and e-mail will persist. This permanently removes that data so that it can't be edited back.

If comments supported revisions there's zero issues though. I can't imagine this ever really happening. There's also a further consideration that if someone cancels their account, they might want the previous data, which could be several years old, to be scrubbed back to 'Anonymous' same for any comments made when they were authenticated.

larowlan’s picture

I am comfortable with that - it feels like an edge case.

  • catch committed ff6eea7 on 8.5.x
    Issue #2614504 by blackra, jibran, mohit_aghera, Yogesh Pawar, larowlan...

  • catch committed 1ff8f06 on 8.4.x
    Issue #2614504 by blackra, jibran, mohit_aghera, Yogesh Pawar, larowlan...
catch’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

Status: Fixed » Closed (fixed)

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

benjy’s picture

Was it decided not to write an upgrade path to clear out the existing data? I can see Jibran asked for it in #82but there wasn't a response?

FWIW, I ran into an issue where an authenticated user was trying to update a comment they owned but it was preventing them from doing so because CommentNameConstraintValidator was checking their current username with the name saved against the comment which had since changed.