I get the following WSOD-inducing error when I try previewing a comment as an anonymous user in Drupal 8 dev (as of Oct. 13, 2013):

Fatal error: Call to a member function id() on a non-object in core/modules/comment/comment.module on line 1371

It looks like comment.module is still using global $user;, which won't work properly (I don't think) for anonymous users with methods like ->id().

This is interfering with my ability to write tests for the Honeypot module; see: #2036501: Don't run Honeypot checks on form previews.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Yes, it does. It's $account, not global $user that is accessed here, and there are multiple things that could be assigned to it.

It's more likely that the user_load_by_name() fails and returns NULL.

larowlan’s picture

Assigned: Unassigned » larowlan

fixamakating

larowlan’s picture

Assigned: larowlan » Unassigned
Status: Active » Needs review
FileSize
1.88 KB
1.25 KB

Should be fail/pass.
Thanks for reporting!

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Great!

geerlingguy’s picture

Is it possible to also add a test of an anonymous user posting a preview comment, so this doesn't sneak in again?

Nevermind, I noticed you have it post a comment by the anonymous user with preview required. That suffices! RTBC +1.

tstoeckler’s picture

So the resulting code in comment_preview() with the patch is:

    // Attach the user and time information.
    if (!empty($comment->name->value)) {
      $account = user_load_by_name($comment->name->value);
    }
    elseif ($user->isAuthenticated() && empty($comment->is_anonymous)) {
      $account = $user;
    }
    else {
      $account = drupal_anonymous_user();
    }

    if ($account->isAuthenticated()) {
      $comment->uid->target_id = $account->id();
      $comment->name->value = check_plain($account->getUsername());
    }
    elseif (empty($comment->name->value)) {
      $comment->name->value = \Drupal::config('user.settings')->get('anonymous');
    }

It *feels* like this could be simplified somehow, but I couldn't actually think of a way how, so just posting here for reference.

Patch totally looks good.

catch’s picture

Status: Reviewed & tested by the community » Needs review

We never actually use anything from drupal_anonymous_user(), so it looks like we could change this to something like:

    // Attach the user and time information.
    if (!empty($comment->name->value)) {
      $account = user_load_by_name($comment->name->value);
    }
    elseif ($user->isAuthenticated() && empty($comment->is_anonymous)) {
      $account = $user;
    }

    if (!empty($account) && $account->isAuthenticated()) {
      $comment->uid->target_id = $account->id();
      $comment->name->value = check_plain($account->getUsername());
    }
    else {
      $comment->name->value = \Drupal::config('user.settings')->get('anonymous');
    }
geerlingguy’s picture

That makes sense.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Yep, looks great! Thanks.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Great work!

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Issue summary: View changes

Fixed formatting.