Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
if no author is passed, comment module will try to use currently logged in user as the author. But, if we want to make comment Anonymous then we have to modify it in the db instead of on the UI.
FYI: in D6, on comment editing, if no author passed, then the comment is assigned to Anonymous.
Comment | File | Size | Author |
---|---|---|---|
#18 | comments.patch | 4.82 KB | bleen |
#16 | comments.patch | 5.75 KB | bleen |
#14 | comments-test.patch | 2.04 KB | bleen |
#13 | comments-with-test.patch | 5.75 KB | bleen |
#13 | comments.patch | 3.87 KB | bleen |
Comments
Comment #1
webchickHm. That sounds like a regression. We should add tests for this.
Comment #2
Damien Tournoud CreditAttribution: Damien Tournoud commentedConfirmed.
Comment #3
bleen CreditAttribution: bleen commentedThis patch fixes the original issue (no tests yet) however I don't know that this is necessarily the correct place for this code... not sure.
Also, while working on this I found that there are a couple of other things going on. Comments author's cannot be changed to Anonymous, nor can they be changed from Anonymous to a registered user. In fact, if a comment is authored by anon and an admin goes to edit the comment, the "Authored by" field is not an auto-complete field.
Comment #4
bleen CreditAttribution: bleen commentedpatch
Comment #5
bleen CreditAttribution: bleen commentedLast patch still had dsm()s
Comment #6
bleen CreditAttribution: bleen commentedok ... this patch is closer but definitely needs work (seeing what testbot has to say though, so needs review). It allows a comment author to be changed to anonymous but if you try and change from anonymous to a registered user, it still lists the post as "not verified"
my head hurts ... sleep
Comment #7
sun.core CreditAttribution: sun.core commentedPlease double-check against #777372: Regression: anonymous comments cannot be edited on Drupal 7
Comment #8
bleen CreditAttribution: bleen commented#777372: Regression: anonymous comments cannot be edited on Drupal 7 was fixed and committed in April 2010 ... this wasnt even opened till Oct 2010 .. so everything that has been tested/done on this has been with a version of HEAD that includes the fix from the other issue
Comment #9
bleen CreditAttribution: bleen commented#6: comment.patch queued for re-testing.
Comment #10
sunThis should be tackled after #632382: "Your name" should not be prepopulated with Anonymous, which solves the first half of the puzzle already.
Comment #11
yngens CreditAttribution: yngens commentedThis issue had been solved for 6.x if anyone interested on http://drupal.org/node/129822#comment-3169748
Comment #12
catchLooks like this still needs work.
Comment #13
bleen CreditAttribution: bleen commentedEvery time I try and enable the Simpletest module with the lates version of HEAD I get a PDO error (http://drupalbin.com/17147) so I'm attaching two patches: one with tests and one without. I'm fairly certain the tests will fail because I have how no way to try running them yet but you never know...
Comment #14
bleen CreditAttribution: bleen commented... and here is a patch with just the tests :)
Comment #16
bleen CreditAttribution: bleen commentedOk ... this patch (which includes the new tests) passes all tests locally
Comment #17
sunPlease use the same #description that is used for
$form['author']['name']
in node_form().The additional
!empty($comment->cid)
conditional is superfluous, since this code path is only reached with $is_admin, which already contains that condition.Minor: We can simply change the if into an elseif instead of indenting the entire code block.
Not visible in the diff context lines is the leading
if (!empty($form_state['values']['cid']))
condition.This means that the check for registered usernames is only performed for new comments from anonymous users. While that sounds about right, we need to double-check the impact of this change on the functional behavior when the edit own comments permission is granted to anonymous users.
We need to make sure that we do not re-introduce #845774: Regression: Anonymous users can post comments in the name of registered users
This should already happen earlier, in comment_form_validate(), so form validation and submit handlers of other modules can rely on the values.
I do not understand why these are two separate code blocks/conditions (and the comments don't explain either). It looks like only the second one is actually needed, whereas the condition as well as assignment of 'name' of the first should be merged in.
Lastly, never use $user, always use $account.
Powered by Dreditor.
Comment #18
bleen CreditAttribution: bleen commentedThis should address Sun's review in #17
Comment #20
bleen CreditAttribution: bleen commentedhuh?! ... /me goes to see how this could possibly effect the sessions tests
Comment #21
bleen CreditAttribution: bleen commented#18: comments.patch queued for re-testing.
Comment #22
yngens CreditAttribution: yngens commentedI believe one of these two nodes should be marked as duplicate to concentrate all the efforts to solve the issue at one place:
#936844: Cannot override comment author to or from Anonymous Posted by skyredwang on October 9, 2010 at 8:04am and has 21 comments to the moment
#129822: allow admins to assign anonymous comments to registered users Posted by ax on March 21, 2007 at 8:22am and has 38 comments to the moment
Comment #23
sunThanks, @bleen18! This looks sufficiently documented, tested, and ready to fly for me now.
Comment #24
wojtha CreditAttribution: wojtha commentedNow the patch needs to be rerolled against D8 in the first place...
Comment #25
sun#18: comments.patch queued for re-testing.
Comment #26
wojtha CreditAttribution: wojtha commentedReturning to RTBC state by sun (#23) since patch is ok for D8 too.
Comment #27
catchTagging for backport.
Comment #28
Dries CreditAttribution: Dries commentedThis looks good, and the tests are perfect and very readable. Committed to 7.x and 8.x. Yay! Thanks all.
Comment #29
skyredwangThe latest commit has a new bug: A comment cannot be assigned to an anonymous author with custom name on comment.create(), but this works on comment.update().
To reproduce, create a comment and fill a random string as name (not a registered username), the error will come out. On the other hand, create a comment, and leave the name blank, save, then edit the comment again, and fill a random string as name, then it works!
Comment #30
sunIf someone can reproduce what @skyredwang reported, then this issue must be bumped to 'critical' and block a 7.1 release (add the 'Release blocker' issue tag). I don't really grok what @skyredwang reported.
Comment #31
bleen CreditAttribution: bleen commentedI can reproduce, but i dont know if Ill have time this week to figure out why... Ill take a look next week if no one gets to it first
Comment #32
webchickWe're going to need a fix (and expended test coverage) for this in the next 24 hours or so, else I'm going to have to roll this patch back for 7.1.
Comment #33
bleen CreditAttribution: bleen commentedOk .. I took a few minutes tonight to look into this and there isn't actually a bug here. The situation that skyredwang describes in #29 is actually a conscious decision that we made. Webchcick and I discussed in IRC and she agrees ....
Take the following (common) situation: a user comments as an anonymous user (lazy or forgot pw...) and then wants to re-associate their comment later with the appropriate user so it appears correctly in tracker (etc...). That is precisely the behavior we are allowing in #29's example. We have no way of knowing if a user with "edit comments" perms (admin 99% of the time) is changing the name to a valid username intentionally (to correctly attribute it) or unintentionally (not knowing there is already a user named "bleen").
This is the same behavior we have in d6 (I'm 99% sure) and IMO its correct.
Skyredwang - if you want to make the case to change this behavior I'd say you should open a new D8 issue and reference this issue for background...
Comment #34
skyredwangI have a use case: with the current logical flow: when migrating a site (D5/D6/Joomla/whatever) with comments to a D7 site, the migration has to create all these comments with no names, then update the same comments and provide the missing names. Does this use case happen often? I don't know.
Comment #35
sun@skyredwang: A migration would programmatically create and update comments. The code in question here only affects form submissions.