CommentForm selects using the user formatted name

This field can only be edited by admins, are you saying the default value is not username?

Can with on this tomorrow, unless someone beats me to it

+++ b/core/modules/user/tests/modules/user_hooks_test/user_hooks_test.module
@@ -5,15 +5,14 @@
     if (\Drupal::state()->get('user_hooks_test_user_format_name_alter_safe', FALSE)) {
-      $name = SafeMarkup::format('<em>@uid</em>', array('@uid' => $account->id()));
+      // Make $name a SafeStringInterface object.
+      $name = t('<em>@id</em>', ['@id' => $account->id()]);
     }
     else {
       $name = '<em>' . $account->id() . '</em>';

Any reason to make it translatable?
this string will appear in translations without need

Not sure, but maybe #2566245: Comment author name overridden by hook_user_format_name_alter() is related?

This is blocked on #2112679: getUsername() should return the username getDisplayName() for the formatted user name

Looked at the patch, no its not

Sigh #2227503: Apply formatters and widgets to Comment base fields would probably have resolved this. Wonder if its too late

working on fails

Closing #2566245: Comment author name overridden by hook_user_format_name_alter() as a duplicate of this issue, since the patch here is actually solving the problem in a more appropriate way. IMHO it would be appropriate to give @dunix credits here though.

+++ b/core/modules/user/tests/modules/user_hooks_test/user_hooks_test.module
@@ -5,15 +5,14 @@
-      $name = SafeMarkup::format('<em>@uid</em>', array('@uid' => $account->id()));
+      // Make $name a SafeStringInterface object.
+      $name = t('<em>@id</em>', ['@id' => $account->id()]);

still issue

@andypost if l.d.o picks up translatable strings from test modules then that is the issue - no?

Trying to proceed this since this would help #2559971: Make SafeMarkup::format() return a safe string object to remove reliance on a static, unpredictable safe list

I think we could create SafeString object there manually even though its internal because its only for a testing purpose. I added a @todo to point out that it could be changed to SafeMarkup::format() after it returns a SafeString object.

this is blocking a critical

working on the test fail, pinging laurii on irc for guidance

As discussed on irc

As per discussion on irc with @stefan_r

Should we be worried that the patch from #32 failed on the old testbot but passed on DrupalCI, or is that just random failure?

@amateescu I think that may have been a random failure indeed - thanks for spotting that.

We changed <em class="placeholder"> to <em>, so let's change the test just to be sure.

I ran #32 lot's of times times locally. Could not reproduce this so I don't think the specific <em> in #34 was the problem - let's ignore the failure?

MacBook-Pro-de-sr:core sr$ git apply comment-name-field-2571909.32.patch 
MacBook-Pro-de-sr:core sr$ php scripts/run-tests.sh --repeat 100 --url http://localhost/drupal/ --class "Drupal\user\Tests\Views\HandlerFilterUserNameTest::testExposedFilter"
Drupal\user\Tests\Views\HandlerFilterUserNameTest::testExpos  16 passes                                      
Drupal\user\Tests\Views\HandlerFilterUserNameTest::testExpos  16 passes                                      
Drupal\user\Tests\Views\HandlerFilterUserNameTest::testExpos  16 passes                                      
Drupal\user\Tests\Views\HandlerFilterUserNameTest::testExpos  16 passes                                      
Drupal\user\Tests\Views\HandlerFilterUserNameTest::testExpos  16 passes                                      
Drupal\user\Tests\Views\HandlerFilterUserNameTest::testExpos  16 passes                                      
Drupal\user\Tests\Views\HandlerFilterUserNameTest::testExpos  16 passes                                      
Drupal\user\Tests\Views\HandlerFilterUserNameTest::testExpos  16 passes                                      
Drupal\user\Tests\Views\HandlerFilterUserNameTest::testExpos  16 passes                                      
Drupal\user\Tests\Views\HandlerFilterUserNameTest::testExpos  16 passes                                      
Drupal\user\Tests\Views\HandlerFilterUserNameTest::testExpos  16 passes                                      
Drupal\user\Tests\Views\HandlerFilterUserNameTest::testExpos  16 passes                                      
Drupal\user\Tests\Views\HandlerFilterUserNameTest::testExpos  16 passes                                      
Drupal\user\Tests\Views\HandlerFilterUserNameTest::testExpos  16 passes                                      
Drupal\user\Tests\Views\HandlerFilterUserNameTest::testExpos  16 passes                                      
Drupal\user\Tests\Views\HandlerFilterUserNameTest::testExpos  16 passes                                      
Drupal\user\Tests\Views\HandlerFilterUserNameTest::testExpos  16 passes                                      
Drupal\user\Tests\Views\HandlerFilterUserNameTest::testExpos  16 passes                                      
Drupal\user\Tests\Views\HandlerFilterUserNameTest::testExpos  16 passes

We wondered why this changed more than actually needed. Yes, using #access would be nice, but why is that in scope of this particular issue?
@stefan.r and @dawehner could not figure that out.

considering this has been bumped to critical maybe let's revert any out of scope changes?

1. +++ b/core/modules/comment/src/CommentForm.php
   @@ -152,29 +152,23 @@ public function form(array $form, FormStateInterface $form_state) {
   +        'data-drupal-default-value' => $config->get('anonymous')
   

   comma needed at end of line.

2. +++ b/core/modules/comment/src/Tests/CommentAnonymousTest.php
   @@ -100,7 +100,7 @@ function testAnonymous() {
        // Make sure the user data appears correctly when editing the comment.
   

   The message can be in single quotes now as it no longer has an apostrophe.

Found a couple of little niggles.

reroll - includes the nits from #40

minimal patch just to see what fails

this seems like an unrelated fail? Value array ( ) is equal to value array ( 0 => array ( 'title' => 'example EN', 'sticky' => 'Off', ), 1 => array ( 'title' => 'example ES', 'sticky' => 'On', ), ).

+1 for going back to #33

It also brings us closer to #2227503: Apply formatters and widgets to Comment base fields which has separate widgets for each field (with access controlled by the existing field access)

yeah why not - reuploading #41

I think #2572553: Incomprehensible validation message when anonymous tries to submit comment with an existing username is related since we get some weird validation issues because of the Name field...

1. +++ b/core/modules/comment/src/CommentForm.php
   @@ -152,29 +152,23 @@ public function form(array $form, FormStateInterface $form_state) {
   +      '#description' => $this->t('Leave blank for %anonymous.', array('%anonymous' => $config->get('anonymous'))),
   

   Nit: we use the short array syntax elsewhere, so also should here.

2. +++ b/core/modules/comment/src/CommentForm.php
   @@ -263,16 +257,27 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
   +    // Empty author-ID should revert to anonymous.
   

   Nit: s/author-ID/author ID/

3. +++ b/core/modules/comment/src/CommentForm.php
   @@ -263,16 +257,27 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
   +      if ($author_id || !$form['author']['name']['#access']) {
   

   #access can also be an AccessResultInterface object.
   Nope, I was wrong, this code fully controls #access, and therefore it can never be an AccessResultInterface object, hence the code is correct.

4. +++ b/core/modules/comment/src/Tests/CommentPreviewTest.php
   @@ -200,6 +204,8 @@ function testCommentEditPreviewSave() {
   +    // Web-user cannot change the comment-author.
   

   Nit: s/Web-user/Web user/ + s/comment-author/comment author/

5. +++ b/core/modules/comment/src/Tests/CommentTestBase.php
   @@ -70,6 +70,9 @@ protected function setUp() {
   +      // Username's aren't shown in comment edit form autocomplete unless this
   

   Nit: s/Username's/Usernames/

I fixed all these nits.

+++ b/core/modules/comment/src/CommentForm.php
@@ -152,29 +152,23 @@ public function form(array $form, FormStateInterface $form_state) {
-      $form['author']['name']['#cache']['contexts'][] = 'user';

We no longer set this cache context, because it is no longer necessary.

\o/ \o/ \o/ \o/

This is what it now looks like, for all users, including an authenticated user:

#60 means we can now finally delete this piece of code, which TBH should've already been removed in #2543334: Auto-placeholdering for #lazy_builder with bubbling of contexts and tags, but now is also completely unnecessary, which is even better :)

+++ b/core/modules/comment/src/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php
@@ -190,16 +190,6 @@ public function viewElements(FieldItemListInterface $items) {
-          if ($this->currentUser->isAuthenticated()) {
-            $output['comment_form']['#cache']['contexts'][] = 'user';
-          }

This part in particular of #61 is now wrong and must be updated here, in this issue, so it remains in sync with CommentForm, i.e. with this particular part that is now gone:

+++ b/core/modules/comment/src/CommentForm.php
@@ -152,29 +152,23 @@ public function form(array $form, FormStateInterface $form_state) {
-    elseif ($this->currentUser->isAuthenticated()) {
-      $form['author']['name']['#type'] = 'item';
-      $form['author']['name']['#value'] = $form['author']['name']['#default_value'];
-      $form['author']['name']['#theme'] = 'username';
-      $form['author']['name']['#account'] = $this->currentUser;
-      $form['author']['name']['#cache']['contexts'][] = 'user';
-    }

And if we remove that first quoted bit, we might as well remove the entire thing, which is exactly what the #61 interdiff does.

Overall this great, just nits

1. +++ b/core/modules/comment/src/Tests/CommentTestBase.php
   @@ -70,6 +70,9 @@ protected function setUp() {
   +      // Usernames aren't shown in comment edit form autocomplete unless this
   +      // permission is granted.
   +      'access user profiles',
   

   looks a separate bug

2. +++ b/core/modules/user/tests/modules/user_hooks_test/user_hooks_test.module
   @@ -5,7 +5,7 @@
   +use Drupal\Core\Render\SafeString;
   

   looks new use is unused

Removing the unused 'use' statement.

re: #67.1, not a bug really, since that permission is intentional, similar to not being able to see a node title if one cannot access the node.

This isn't actually true :)

Did some manual testing of the Comment preview and things seem to look ok, except for a couple of issues tackled elsewhere:

* #2574597: Comment body doesn't show up in preview for Anonymous users
* #2572553: Incomprehensible validation message when anonymous tries to submit comment with an existing username

re: #67.1, it is a bug and there is an existing issue for it: #2471154: Anonymous user label can't be viewed and auth user labels are only accessible with 'access user profiles' permission

Ok, we had lots of reviews, manual testing, found various bugs but nothing caused by this. Code looks good to me as well, awesome cleanup and performance improvement.

Adding credit to @dunix per #21.

Adding issues from #70 and #71 as related issues. I haven't fully wrapped my head around what the nature of that relationship is though: i.e., whether they're completely independent issues or in some way affected by this.

@effulgentsia: Both are existing bugs in HEAD. We just found them while testing this and are working on fixing them separately. They are otherwise not related to this issue.

I haven't fully grokked all of the changes in this patch yet, and possibly I need to do some manual testing in order to, but this jumped out at me as potentially problematic:

+++ b/core/modules/comment/src/CommentForm.php
@@ -263,16 +257,27 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
+        $comment->setAuthorName($account->getDisplayName());

If getDisplayName() returns a SafeString, then we'd be populating the author name with HTML, but AFAICT, the comment author name is always rendered elsewhere as plain-text. Should we change this to:

$html_display_name = SafeMarkup::format('@name', ['@name' => $account->getDisplayName()]);
$text_display_name = PlainTextOutput::renderFromHtml($html_display_name);
$comment->setAuthorName($text_display_name);

Kind of verbose, but given the explicit flexibility of AccountInterface::getDisplayName() returning either plain-text or SafeString, any other suggestions on how to do this?

Hm, maybe #78 isn't a problem, since Comment::getAuthorName() ignores what's in the 'name' field anyway for authenticated users. But that then raises the question of why set it?

More code removed, tests are passing, looks good to me. Nice!

I meant to do this.

Thanks! #80 definitely resolves that concern. I still want some more time with this patch before I can feel comfortable committing it, but I don't object to someone else committing it before me if they're comfortable with it. I'll try to get to this within the next 24 hours if no one beats me to it.

Taking for a manual spin

Screenshots - all looks good.

Confirmed the anonymous name isn't retained if the comment is switched to an authenticated user and then back to anonymous.

Looks good to me - one final question - should we name the field uid instead of admin_select - as that is the base-field we're editing - which would also align with #2227503: Apply formatters and widgets to Comment base fields

Yay for extra manual testing!

Here's a little cleanup of test code.

I'm happy with #89, so as long as bot is happy with #91 and someone RTBC's that interdiff, I can commit.

Pushed to 8.0.x!

See #2552873-17: node/1 flamegraphs and #2552873-18: node/1 flamegraphs. This caused a big performance regression: the comment form still has a max-age=0 that is bubbled, therefore the entire node is no longer render cached…

The combination of #2463567: Push CSRF tokens for forms to placeholders + #lazy_builder plus #2578855: Form tokens are now rendered lazily, allow forms to opt in to be cacheable would solve this regression.

This issue leads to data loss #2614504: Values of 'name' & 'email' fields should be NULL when comment has author (uid > 0)