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
- Install Standard.
- Create a comment view using fields, and add the comment author field.
- Create an article node.
- Add a comment on the article.
- Visit the view and confirm that the username is displayed.
- Edit the comment and change something, e.g. the comment body.
- Visit the view. The username disappears.
- 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
Identify plugins needed, as they appear in Views (the highest priority being whatever blocks #1986606: Convert the comments administration screen to a view.Force the undefined database values to NULL, along with adequate test coverage (this is in progress).Write the missing plugins.- 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?
Comment | File | Size | Author |
---|---|---|---|
#87 | values_of_name-2614504-87.patch | 6.95 KB | jibran |
Comments
Comment #2
xjmComment #3
xjmComment #4
andypostInitial stub
Comment #5
xjmWe should use !== the anonymous user constant here, or possibly some method somewhere, rather than > 0.
Comment #7
thpoul CreditAttribution: thpoul as a volunteer commentedUpdated patch according to @xjm recommendation
Comment #8
blackra CreditAttribution: blackra as a volunteer and at SK Plus commentedI have been looking into this in some depth.
The causes of the problem are:
Our options are:
Comment #9
andypost@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)
Comment #10
blackra CreditAttribution: blackra as a volunteer and at SK Plus commentedWe 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.
Comment #11
blackra CreditAttribution: blackra as a volunteer and at SK Plus commentedHere is a failing test case that illustrates the original problem.
Comment #12
andypostYep, 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
Comment #13
blackra CreditAttribution: blackra as a volunteer and at SK Plus commentedIn 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:
I think those need to be considered and tested separately in order to get the right answer.
Comment #14
blackra CreditAttribution: blackra as a volunteer and at SK Plus commentedNote that my failing test patches don't include the content of patches #2 & #7.
Comment #15
andypostThat'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
Comment #16
andypostthere's no need to edit comment second time, better to login non-admin user and post new comment and edit
Suppose second one is getAuthorEmail()
Comment #20
larowlanprodding, starting by combining the patch and the test
Comment #21
blackra CreditAttribution: blackra as a volunteer and at SK Plus commentedLarowlan, 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:
Comment #22
larowlansome stuff, still some fails
Comment #23
larowlanFWIW 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
Comment #25
blackra CreditAttribution: blackra as a volunteer and at SK Plus commentedAfter 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.
Comment #26
blackra CreditAttribution: blackra as a volunteer and at SK Plus commentedThe 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.
Comment #27
blackra CreditAttribution: blackra as a volunteer and at SK Plus commentedUploaded a new version. This one has the test for the mail field too.
Comment #30
blackra CreditAttribution: blackra as a volunteer and at SK Plus commentedWell, 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.
Comment #31
blackra CreditAttribution: blackra as a volunteer and at SK Plus commentedIt 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?
Comment #32
andypostI agree that better to save NULL into name and mail fields.
OTOH formatters need fix because they get no field items to render
++ would be great to add @see pointing where name and mail set for anonymous
is not needed
Comment #33
blackra CreditAttribution: blackra as a volunteer and at SK Plus commented@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...
Comment #34
andypost@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 authorizedjoin 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
Comment #35
blackra CreditAttribution: blackra as a volunteer and at SK Plus commentedI'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?
Comment #36
andypostSuppose no,
last_comment_name
is already denormalized and should be used (hm, probably that should be a user-display-name then)Comment #37
blackra CreditAttribution: blackra as a volunteer and at SK Plus commentedHere are the additional tests and the comment. The problem with hal has not been fixed yet so I expect 3 test failures.
Comment #39
lokapujyaDoes 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.
Comment #40
blackra CreditAttribution: blackra as a volunteer and at SK Plus commentedHere 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.
Comment #42
blackra CreditAttribution: blackra as a volunteer and at SK Plus commentedNow 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.
Comment #43
blackra CreditAttribution: blackra as a volunteer and at SK Plus commentedHere 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.
Comment #45
larowlanWe 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.
Comment #46
blackra CreditAttribution: blackra as a volunteer and at SK Plus commentedWe 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.
Comment #47
andypostyep, looks promising, the only pita would be filtering and sorting
Comment #48
catchComment #49
blackra CreditAttribution: blackra as a volunteer and at SK Plus commentedThis 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.
Comment #51
dawehnerSo this is what I don't understand. Why do we not implement a field formatter to achieve the same?
Nitpick: we use snake case for local variables
You should be able to return a render array directly ... if this is not possible, there is a bug.
Note: We put the one line description for properties on the line above, see https://www.drupal.org/node/1354
Why can't we use
$this->placeholder()
directly?So why can't we use
$this->query->addWhereExpression
instead?Can we avoid hardcoding this tablename there and use the table mapping instead:
Query is already an object, we don't need it to create a reference to it.
no new newline.
i kinda dislike this endless IMHO not really helpful test messages but well, some people seem to liek them.
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.
Comment #52
blackra CreditAttribution: blackra as a volunteer and at SK Plus commented@dawehner, thanks for taking the time to review this. These are some really helpful comments.
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.
Comment #53
lokapujyaJust 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.
Comment #54
andypost#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 commentComment #55
andypostComment #56
lokapujyaCan the formatter still do what it needs to do when the database value is empty? something like this patch.
Comment #57
blackra CreditAttribution: blackra as a volunteer and at SK Plus commentedI 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:
As I see it there are three ways of tackling this, with different pros and cons:
Note that patch #56 does not address filtering or sorting. The field filters and sorts as NULL.
Comment #58
lokapujyaIt 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?
So, is that the real reason we need a field plugin?
Comment #59
blackra CreditAttribution: blackra as a volunteer and at SK Plus commented@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:
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.
Comment #60
blackra CreditAttribution: blackra as a volunteer and at SK Plus commentedSummary 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:
My proposal is that we do the following:
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).
Comment #61
lokapujyaI'm still confused about this. I compared 8.0.x to 8.1.x and don't see any changes in the comment module.
Comment #62
andypostAs maintainer I'd better do progress in 8.1 and later so anyone can backport to prev version (is there any need for?)
Usage of "name" property makes sense only for anonymous comments and all my exp said - mostly all the time this column contains default value.
Also here's some issues:
1) #129822: allow admins to assign anonymous comments to registered users
2) #1121876: If a user changes their e-mail address, then we need to also update {comment}.mail
3) #2422443: Fix default value of author in \Drupal\comment\CommentStatistics::create()
4) #2572553: Incomprehensible validation message when anonymous tries to submit comment with an existing username
Comment #63
andypostComment #66
xjm@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!
Comment #68
mohit_aghera CreditAttribution: mohit_aghera as a volunteer and at Axelerant commentedUpdating patch for 8.4.x branch, it was not getting applied properly.
Moreover above snippet from #56 seems refactored in 8.4.x branch.
Comment #70
mohit_aghera CreditAttribution: mohit_aghera as a volunteer and at Axelerant commentedUpdating patch to prevent fatal error
Comment #72
yogeshmpawarUpdated patch against test failures & also previous patch failed to apply on 8.4.x branch. surely this patch will pass all the tests.
Comment #73
Munavijayalakshmi CreditAttribution: Munavijayalakshmi at Valuebound commenteduse short array syntax (new coding standard).
Line exceeding 80 characters.
use short array syntax (new coding standard).
Comment #74
Munavijayalakshmi CreditAttribution: Munavijayalakshmi at Valuebound commentedComment #75
yogeshmpawarLittle more cleanup for coding standard issues.
Comment #78
jibranIn 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.
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.
Comment #79
andypostThis 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...this is a main point of the issue!
To set NULL to name&mail fields when comment got author
this exactly the test it for
Comment #80
jibran#79 makes sense. Thanks for explaining that @andypost.
Comment #81
jibranUpdated 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.
Comment #82
jibranDo we need an update path to clear the exisitng data? FWIW It is not has not effect on displays.
Comment #83
jibranIt's a new start so no interdiff.
Comment #84
andypostLooks great!
Please elaborate this change
I wondered this change. I'd better used xpath here to be sure that name rendered but without link
Comment #85
jibranHere you go.
Comment #86
andypostAs discussed at IRC - view renders 2 comments so for me it's not clear which comment render we testing
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
Comment #87
jibran@andypost nice catch. Now we have UnitTest, KTB and BTB for this bug.
Comment #88
andypost@jibran Thanx a lot! Now it loks perfect
Comment #90
catchThis 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.
Comment #91
larowlanI am comfortable with that - it feels like an edge case.
Comment #94
catchCommitted/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!
Comment #96
benjy CreditAttribution: benjy at Unearthed commentedWas 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.