Comment previews #prefix problem

This is causing problems that were reported in the Bartik issue #748314: Sidebar drops on comment preview. It's a problem that needs to be fixed in core.

Ugly, right? Wanna fix it?

This still happens in alpha6, and indeed probably the latest checkout from CVS HEAD.
I'm marking this critical, as it is a core issue that affects all themes. Feel free to whack me over the head if this is not correct thinking.

I've attached a patch which I believe to be the intended behavior (that is, to overwrite the original prefix, which is a fairly useless anchor).

Changing the status to needs review.

Rerolled without the path prefix... Ugh, this is a bad format to require patches in.

Have you checked this issue against 7.0-dev? I don't have this with problem with the dropping sidebar there. Seems to have been fixed...

I haven't directly checked it, but the code involving the comment preview (and, indeed, all of comment.module) is the same, so the bug is still there. It could be theme-dependent whether it's noticed or not, but it always breaks the structure of the page.

My dumb... I checked with only the first sidebar active :-(

The bug still exists, of course. The patch in #5 solves the problem in both Garland and Bartik.

Yup, #5 fixes the problem for me.

EDIT: I was completely wrong when I wrote this ... removed my erroneous comment

restoring status

I went back and fourth on this for a few minutes and I believe it is technically more correct to use array_merge in this case. Also a quick glance at core tells me that we generally use array_merge. So... see patch

There is no technical reason to use array_merge (or not), apart from it being more consistent with the core code - and that's certainly a good enough reason to use it.
It would be different if there were numeric indexes involved, but there are not. I only used + because I'm not familiar with the conventions used in core, and + was used originally.

The patch in #12 is not correct - array_merge does not alter the array that is passed, it returns a new array.

oops ... forgot to fix that after I was testing some stuff

a common mistake (for me, anyway).

A line of code documentation would justify the proper method to override these properties and also explain what is being done here and why for future readers. array_merge() is 100% overhead here.

better. thx.

It might be cleaner to add a preview flag to comment_view()? At least that would keep all the code together?

Something like this?

same as #21 without whitespace probs

The original post additionally stated that we do not need that div.preview container anymore, because http://api.drupal.org/api/function/template_preprocess_comment/7 already sets a .comment-preview class for http://api.drupal.org/api/drupal/themes--bartik--templates--comment.tpl....

Hence, the only remaining property would be the #weight. Personally, I'd say that all preview logic belongs into comment_preview(), and should not be mixed into the generic comment_view() function. Would be interesting to know what moshe thinks, as this also cries for a design pattern/standard regarding renderable $build arrays and where to build/prepare what.

I'm not too keen on $comment->in_preview but thats consistent with nodes so lets go with it IMO.

sun is correct about the redundant .preview container.

This patch removes it, but leaves the #weight property in comment_view() (as per Moshe's comment in #23)

The $comment->in_preview property itself is consistent, yes. However, neither node_preview() nor theme_node_preview() (which looks odd on its own) nor node_view() is setting a special negative #weight.

To me, it looks not clean to have comment_view() account for a special #weight that may be useful when previewing. comment_view() should only care for building the requested view mode - setting a #weight would be a presumption or prediction on the surrounding elements on the page. Likewise, I believe that the already existing DIV containers for indentation actually belong into comment_view_multiple(), since they have nothing to do with the view mode or a rendering of a single comment at all, which of course is a different issue.

EDIT: - Ignore this patch ... wrong one :(

This is the patch I meant to upload...

+++ modules/comment/comment.module	26 Jul 2010 15:27:16 -0000
@@ -913,26 +913,28 @@ function comment_view($comment, $node, $
+  if (!isset($comment->in_preview) || !$comment->in_preview) {

=== empty($comment->in_preview)

+++ modules/comment/comment.module	26 Jul 2010 15:27:16 -0000
@@ -913,26 +913,28 @@ function comment_view($comment, $node, $
+  ¶

Trailing white-space.

Powered by Dreditor.

hmph .. learned something new. I figured empty would throw an error.

Cool beans.

Overall, Comment module + markup + logic needs much more love, but this patch cuts it for now.

Committed to CVS HEAD. Thanks.