Fix the author names in the forum list

We need both, one is for the last comment, the other for the author. However the test pass intrigues me.

Yep we never actually fetch the uid field from the {node} table here so it is indeed a pointless join.

Committed/pushed to 8.x, moving to 7.x for backport.

+++ b/modules/forum/forum.module
@@ -823,11 +823,10 @@ function forum_forum_load($tid = NULL) {
-    $query->join('users', 'u2', 'ncs.last_comment_uid = u2.uid');
-    $query->addExpression('CASE ncs.last_comment_uid WHEN 0 THEN ncs.last_comment_name ELSE u2.name END', 'last_comment_name');
+    $query->join('users', 'u', 'ncs.last_comment_uid = u.uid');

please don't change alias name because contrib probably already alters this

thanx, thats much better

I don't think it's true that this join is pointless. It's an inner join, which means it guarantees there's actually a matching row in the users table.

By guaranteeing that, it forces the query to be consistent with the one in forum_get_topics(), which also does this join. And we do want them to be consistent (since one query gets you information about the most recently commented-on post and the other shows a list of posts, so it would be pretty weird if the forum overview says "Last post: 5 minutes ago by X" but then you go to the forum and there is no such recently-updated post).

In practice, this can only occur if the node author is a user who no longer exists on the site... so it should be pretty rare. However, we do have situations where a user could have been deleted without the node module's hooks being fired to respond to it, so it definitely can happen.

Moving back for discussion, since the general point is that this query should be consistent with forum_get_topics() but the patch makes it not consistent.

Also, to be honest, I'm not that excited about committing the patch to Drupal 7 either way. It doesn't seem to be an actual bug (or a noticeable performance hit, at least not that I know of?), and as @andypost points out this is an alterable query (in fact, it's a node access query) so we're probably better off not changing those in Drupal 7 unless we have a good reason to.

This is broken in D7, and the patch has broken it even more for D8.

Let me explain for D7, because that is less broken — here's what we have:

    // Query "Last Post" information for this forum.
    $query = db_select('node', 'n');
    $query->join('users', 'u1', 'n.uid = u1.uid');
    $query->join('forum', 'f', 'n.vid = f.vid AND f.tid = :tid', array(':tid' => $forum->tid));
    $query->join('node_comment_statistics', 'ncs', 'n.nid = ncs.nid');
    $query->join('users', 'u2', 'ncs.last_comment_uid = u2.uid');
    $query->addExpression('CASE ncs.last_comment_uid WHEN 0 THEN ncs.last_comment_name ELSE u2.name END', 'last_comment_name');

    $topic = $query
      ->fields('ncs', array('last_comment_timestamp', 'last_comment_uid'))
      ->condition('n.status', 1)
      ->orderBy('last_comment_timestamp', 'DESC')
      ->range(0, 1)
      ->addTag('node_access')
      ->execute()
      ->fetchObject();

    // Merge in the "Last Post" information.
    $last_post = new stdClass();
    if (!empty($topic->last_comment_timestamp)) {
      $last_post->created = $topic->last_comment_timestamp;
      $last_post->name = $topic->last_comment_name;
      $last_post->uid = $topic->last_comment_uid;
    }
    $forum->last_post = $last_post;

ncs.last_comment_uid is never 0 (at least I have no such record in my database), so this always returns u2.name, which is the name of the creator rather than the author.

Here's how to reproduce the bug: as admin create a new topic and set Authored by to 'Bob'. Bob's name will show up as the author of the topic, but forum/% will show the admin's name instead.

The code above is obvious nonsense, but the u1/u2 idea is a good one — it has probably worked at some point. The correct code is:

    // Query "Last Post" information for this forum.
    $query = db_select('node', 'n');
    $query->join('users', 'u1', 'n.uid = u1.uid');
    $query->join('forum', 'f', 'n.vid = f.vid AND f.tid = :tid', array(':tid' => $forum->tid));
    $query->join('node_comment_statistics', 'ncs', 'n.nid = ncs.nid');
    $query->join('users', 'u2', 'ncs.last_comment_uid = u2.uid');
    $query->addExpression('CASE ncs.comment_count WHEN 0 THEN u1.name ELSE u2.name END', 'name');   // FIX
    $query->addExpression('CASE ncs.comment_count WHEN 0 THEN u1.uid ELSE u2.uid END', 'uid');      // FIX

    $topic = $query
      ->fields('ncs', array('last_comment_timestamp'))   // FIX
      ->condition('n.status', 1)
      ->orderBy('last_comment_timestamp', 'DESC')
      ->range(0, 1)
      ->addTag('node_access')
      ->execute()
      ->fetchObject();

    // Merge in the "Last Post" information.
    $last_post = new stdClass();
    if (!empty($topic->last_comment_timestamp)) {
      $last_post->created = $topic->last_comment_timestamp;
      $last_post->name = $topic->name;   // FIX
      $last_post->uid = $topic->uid;     // FIX
    }
    $forum->last_post = $last_post;

We must check the comment_count! If there are no comments, then u1.uid / n.uid is the one to use, and only if there is at least one comment we get the comment author in u2.uid.

Besides the name we also need the uid. It is used to turn the user name on forum/% into a link to the user's profile page.

Here's the patch for D7 as explained above. Let's see what the testbot thinks of it.

(I know we'll need D8 first, and we'll need tests, obviously.)

Good, now D8...

#11: forum_author.1377468.11.D8.patch queued for re-testing.

#11: forum_author.1377468.11.D8.patch queued for re-testing.