Collapsed comments

I can reproduce this problem. I'm marking this 'critical'.

From what I can tell, collapsed comments are not very popular. Maybe we are better off removing this feature?

I just fixed this bug.

As reported in this duplicate, this bug is not completely fixed.

I've attached a patch with an alternate fix. I'm pretty sure this restores collapsed comments to their previous functionality. If not, please tell me what's wrong.

Works here. Sorry about the duplicate; I intentionally created it because I thought bug reports for the RC and CVS were supposed to be kept separate, and as I'm not currently using CVS I couldn't verify if this was actually fixed.

Just a followup: when viewing a node with comments in collapsed mode, after this patch, clicking the comment subject will bring you to the body of that comment.

However, avatars don't display on that page. If you switch to viewing the thread in expanded mode, all the avatars display as usual.

Since my custom theme_comment function is correctly loading the profile values and displaying the avatars in expanded modes, I'm guessing that somehow having the mode set to collapsed (folded) is somehow interfering with its ability to load these values; the image link is in the HTML with an empty string supplied.

I can't seem to reproduce the avatar problem. It sounds like a separate problem to me.

Two thinks I'd like you to investigate more:

• You might be able to simplify the change in node.module. The original check might be redundant due to the legacy.module, which was introduced later on.
• Should the changes to comment.module be part of the $may_cache clause or should it be non-cachable?

TDobes,

I suspect you may be right; however, I don't think (please correct me if I'm wrong" that any of the themes that come with 4.5 display avatars, so the problem may be with the way in which I've updated my theme to work with 4.5.

Can you point me to a fully 4.5-compliant theme that supports avatars?

Dries: Addressing your two points:

• I looked at legacy.module and thought a bit about the node.module code in question. In the attached patch, I have made it a bit more readable by using only one conditional operator rather than two, but the logic remains the same. I've also removed the "temporary fix" comment, as I don't think this is really temporary at all. If we want to switch on $op, as we do now, we have to be able to deal with the following types of URL's:
  • node/1 ($op = 'view')
  • node/1/2 ($op = 'view')
  • node/add ($op = 'add')
  • node/1/edit ($op = 'edit')

  I don't really see any easier way to choose the correct $op than what we currently use. For 4.6, we might consider eliminating the node_page function entirely and instead turning the individual node.module functions into callbacks.

• The comment.module code should be non-cachable. It is in an else clause to comment.module's if ($may_cache).

dmjossel: Both bluemarine and pushbutton themes in core support avatars (now called "user pictures"). In your own themes, you can use theme('user_picture', $node) or theme('user_picture', $comment), which will generate the appropriate img tag for you. Note that both pushbutton and bluemarine respect the new theme settings which can be modified by navigating to admin->themes->configure. The test site on which I tested this patch has avatars enabled (both in comments and nodes), and they seem to work fine for me.

Thanks for the follow up, Tom. Committed your patch.

I agree that we should consider to split node_page() using menu handler. Maybe we start compiling a list of code beautifications we want to make? Or add it as a task to the issue tracker.

Oops, marking this fixed.