User name is correct, but the link points to the profile of the last reply author.
I can't understand why needs this line in template_preprocess_forum_topic_list()
$topic->uid = $topic->last_comment_uid ? $topic->last_comment_uid : $topic->uid;

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

droplet’s picture

Status: Active » Needs review
Issue tags: +Quick fix
FileSize
713 bytes

introduce in this patch:
http://drupal.org/files/issues/taxonomy_67.patch

wrong patch I think, quick fix

Chi’s picture

Status: Needs review » Reviewed & tested by the community

I have applied the patch. It works fine.

Chi’s picture

The patch will be commited to HEAD?

catch’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
Dries’s picture

Status: Reviewed & tested by the community » Needs review

I'd like another pair of eyes on this patch because I don't think it is correct.

Removing that line is likely to break the 'last reply' field, not?

Shouldn't it be:

      $variables['topics'][$id]->created = theme('forum_submitted', array('topic' => $topic));
      $topic->uid = $topic->last_comment_uid ? $topic->last_comment_uid : $topic->uid;
      $variables['topics'][$id]->last_reply = theme('forum_submitted', array('topic' => isset($topic->last_reply) ? $topic->last_reply : NULL));
sun’s picture

Priority: Major » Normal
Status: Needs review » Needs work

Yes. Also not major.

Nneka’s picture

Assigned: Unassigned » Nneka
Nneka’s picture

Assigned: Nneka » Unassigned
Status: Needs work » Closed (fixed)

opening ticket. mistake to close. implementing the fix noted here.

Nneka’s picture

Assigned: Unassigned » Nneka
Status: Closed (fixed) » Needs work
Nneka’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Needs review
FileSize
1.11 KB

Preface this by saying a newbie to contributing and git.

Made the code change suggested in post #5, tested and everything seems to be working without fail. This attached patch was made for 7.x.

Status: Needs review » Needs work

The last submitted patch, forum-createduser-1078646.patch, failed testing.

Vincent Giersch’s picture

Status: Needs work » Needs review
FileSize
858 bytes

Your patch was in a patch and there was an incorrect indentation.

droplet’s picture

what's the diff by move that line ?

droplet’s picture

Version: 7.x-dev » 8.x-dev
Assigned: Nneka » Unassigned

#5, #12 moving that lines would result a wrong topic->uid.
topic->uid should be same as TOPIC author, not the comment user.

#1 my patch seems more right.

@Dires,
last reply handled in other parts:

$variables['topics'][$id]->last_reply = theme('forum_submitted', array('topic' => isset($topic->last_reply) ? $topic->last_reply : NULL));
Nneka’s picture

Version: 8.x-dev » 7.x-dev
FileSize
1.11 KB

After looking a little deeper into the code, I would agree with #14.

I believe the only reason that line of code was added ($topic->uid = $topic->last_comment_uid ? $topic->last_comment_uid : $topic->uid;), was to override the Commented by (its being used when called the form_submitted theme to display the user submission info.).

I've tested with this line of code commented out and it works fine.

Including the fix for Drupal 7 with the code above removed.

Status: Needs review » Needs work

The last submitted patch, forum-createduser-1078646.patch, failed testing.

Nneka’s picture

Status: Needs work » Needs review
FileSize
863 bytes

Ignore the last patch. This one is correct.

droplet’s picture

Version: 7.x-dev » 8.x-dev

@Nneka,
Please leave this at 8.x as we fix bugs first in 8.x and backport them as fast as possible.

Git patch:
http://drupal.org/node/707484

Sounds like you are going to do something like #1 patch ?

Nneka’s picture

Yes. This patch is the same #1.

Nneka’s picture

Version: 8.x-dev » 7.x-dev

This ticket also exists as a duplicate. Closed http://drupal.org/node/1181240 as duplicate.

droplet’s picture

Hey,

Please don't change version again. Thanks!

droplet’s picture

Version: 7.x-dev » 8.x-dev
FileSize
721 bytes

still same as #1 but a -p1 style patch

both D7 & D8

larowlan’s picture

Status: Needs review » Needs work
+++ b/modules/forum/forum.module
@@ -1123,7 +1123,6 @@ function template_preprocess_forum_topic_list(&$variables) {
-      $topic->uid = $topic->last_comment_uid ? $topic->last_comment_uid : $topic->uid;

This line is needed when constructing the last reply theme output.
The problem is it needs to be set after the created variable is set.
Ie it doesn't need to be removed, it just needs to go after

$variables['topics'][$id]->created = theme('forum_submitted', array('topic' => $topic));

and before

$variables['topics'][$id]->last_reply = theme('forum_submitted', array('topic' => isset($topic->last_reply) ? $topic->last_reply : NULL));

see http://api.drupal.org/api/drupal/modules--forum--forum-submitted.tpl.php... and http://api.drupal.org/api/drupal/modules--forum--forum.module/function/t... in the last reply variable ($variables['topics'][$id]->last_reply)) we need the 'by' link to point to the last_comment_uid.

Powered by Dreditor.

larowlan’s picture

Status: Needs work » Needs review
FileSize
979 bytes

Reroll with my comments

droplet’s picture

FileSize
166.48 KB

@larowlan,

It passed $topic->last_reply into theme function

and theme function access $variables['topic']->uid

the result it will access:

$topic->last_reply->uid

see my attached image & try to test my patch #22 :)

EDIT:
my debug function is after this line

$topic->uid = $topic->last_comment_uid ? $topic->last_comment_uid : $topic->uid;
dsm($topic);

topic created with UID 1, so the attached image UID change to 2 which is wrong.

larowlan’s picture

Please see http://api.drupal.org/api/drupal/modules--forum--forum-topic-list.tpl.ph...
We need to set the $topic->uid to that of the last commenter to make the 'last reply' column work.

So there are two calls to theme('forum_submitted'
The first one to setup the 'created by' column, the second to setup the 'last reply' column.
Without your patch, the 'created by' column does not work.
With your patch, the 'last reply' column does not work.
I think my patch is the common ground where they both work.

Please advise.

droplet’s picture

can you capture a screenshot to show how it doesn't work & point out the line that used $topic->uid on
http://api.drupal.org/api/drupal/modules--forum--forum-topic-list.tpl.ph...

what's your drupal version ?

droplet’s picture

FileSize
282.17 KB

my turn. I don't see any problem

larowlan’s picture

$variables['author'] in forum_submitted.tpl.php is set here using $topic->uid:
http://api.drupal.org/api/drupal/modules--forum--forum.module/function/t... and then output here
http://api.drupal.org/api/drupal/modules--forum--forum-submitted.tpl.php...

droplet’s picture

@larowlan,

are you tested with fresh installation ?? and on same issue ? I know you are the new forum maintainer, maybe working on a lot of forum issues and make comment on wrong issue ??

What here is fixing template_preprocess_forum_topic_list on forum topic list

but you are taking about forum_submitted on forum thread (node) page.

Anything I missed ??

larowlan’s picture

@droplet
template_preprocess_forum_topic_list calls theme('forum_submitted' which in turn calls template_preprocess_forum_topic_list which in turn uses $topic->uid to set the author.
template_preprocess_forum_topic_lic calls theme('forum_submitted' twice, once for the creator and again for the last reply - in the first instance the author is the original $topic->uid, in the second instance it's the commenter hence before the second call needs $topic->uid set to that of the commenter.

droplet’s picture

@larowlan,

Listen, give me a chance :)
- First reinstall your Drupal-8-dev
- add new user
- create a forum topic & reply is with different user

then edit this function and add debug function

function template_preprocess_forum_submitted(&$variables) {
  dsm($variables['topic']);
  $variables['author'] = isset($variables['topic']->uid) ? theme('username', array('account' => $variables['topic'])) : '';
  $variables['time'] = isset($variables['topic']->created) ? format_interval(REQUEST_TIME - $variables['topic']->created) : '';
}

Compare what's the difference.

Why your patch (seems like) solve the problem?

Because whatever you changed, it never pass into theme function (last_reply). It's like my patch (removed that line)

$variables['topics'][$id]->last_reply = theme('forum_submitted', array('topic' => isset($topic->last_reply) ? $topic->last_reply : NULL));

Notice: Patch #24 will get it work but if you try to theme your fourm list on forum-topic-list.tpl, you never know who is the topic creator.

try add following on forum-topic-list.tpl after you patched with #24
dsm($topics);
You will see UID is always equal to last reply user. (this UID should be the topic creator)

Hope you understand it, I have no idea to explain more in details.

Hope i'm not wrong so. :)

any other guys have chance to review it ??

larowlan’s picture

You're right, there is no difference between your patch and mind, the second call to theme('forum_submitted' passes $topic->last_reply (not $topic as I thought).

So yours is the better patch.

larowlan’s picture

#22: forum_1078646.patch queued for re-testing.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

The patch in #22 is good to go, I had changed it at #24 based on Dries comments at #5 but as rightly pointed out by @droplet, there is no difference between the two. The last reply columns works without the offending line.

luke.oakes’s picture

Sorry for my ignorance and if I've misunderstood... so this patch has been applied to drupal 8... but isn't backported to drupal 7 yet? What's the timeline for this happening?

larowlan’s picture

Hi Luke,
No the patch in #22 has not been applied anywhere yet.
You can apply it to Drupal 7.8 if you wish.
The patch should apply cleanly against both Drupal 7 and 8.
The patch should hopefully be committed before the next Drupal 7 point release (Drupal 7.9) which is due to come out at the end of the month.

luke.oakes’s picture

Thanks for the clarification Larowlan :)
I'll delete the line from my 7.8, and look forward to seeing the implementation in 7.9

sun’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/modules/forum/forum.module
@@ -1127,8 +1127,10 @@ function template_preprocess_forum_topic_list(&$variables) {
-      $topic->uid = $topic->last_comment_uid ? $topic->last_comment_uid : $topic->uid;
       $variables['topics'][$id]->created = theme('forum_submitted', array('topic' => $topic));
+      // Now we set the uid to correspond to the last comment so that username
+      // is used in the themed output.
+      $topic->uid = $topic->last_comment_uid ? $topic->last_comment_uid : $topic->uid;

For the world after us dinosaurs, can we clarify this comment on what "now" refers to and actually means?

Overall, I've the impression that we're relying on quite some bit of magic in that (overridable) theme function. If it gets overridden, the magic might be lost. Or not. Anyway, in the end, it looks like lots of non-obvious assumptions are being made here (unless I'm missing something obvious).

14 days to next Drupal core point release.

droplet’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
721 bytes

@sun, you're reviewed a wrong patch.

#22 is the right one.

(im just reupload it once.)

larowlan’s picture

Yeah, the patch at #40 is the right one (same as #22).

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x and 8.x. Thanks.

Status: Fixed » Closed (fixed)
Issue tags: -Quick fix, -Needs backport to D7

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