from sun's issue here: #849862: Bartik code problems

+++ themes/bartik/templates/node.tpl.php 6 Jul 2010 03:49:00 -0000
@@ -0,0 +1,126 @@
+    // Remove the "Add new comment" link on the teaser page or if the comment
+    // form is being displayed on the same page.
+    if ($teaser || !empty($content['comments']['comment_form'])) {
+      unset($content['links']['comment']['#links']['comment-add']);
+    }

Such adjustments should really be done in a preprocess function, not arbitrarily stuffed within a template. All core theme templates should be lean and mean.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bleen’s picture

Status: Active » Needs review
FileSize
2 KB
Jeff Burnz’s picture

#1: bartik-process-node.patch queued for re-testing.

aspilicious’s picture

is bartik_process_node a PREprocess function?

Jeff Burnz’s picture

no, its a process function, it runs after the preprocess phase.

aspilicious’s picture

"Such adjustments should really be done in a preprocess function,"

So this needs work as it has to be done in a preprocess function..
Correct?

Jeff Burnz’s picture

not necessarily.

Jeff Burnz’s picture

Title: Simplify Bartik node.tpl » unset Add New Comment link is not unset
Category: task » bug
Priority: Minor » Normal

The code in this is wrong:

$content['links']['comment']['#links']['comment-add']

Should be...

$content['links']['#links']['comment-add']

I'm not sure why we need to set a new variable here, I have rerolled this to fix the issues.

Jeff Burnz’s picture

Weird, my patch didnt get attached, lets try that again...

bleen’s picture

d7
d7

bleen’s picture

Status: Needs review » Reviewed & tested by the community

status

sun’s picture

Status: Reviewed & tested by the community » Closed (duplicate)

Thanks for taking the time to report this issue.

However, marking as duplicate of #754760: "Add new comment" appears directly above comment form / "post comments" does not work without "access comments" permission. You can follow up on that issue to track its status instead. If any information from this issue is missing in the other issue, please make sure you provide it over there.

jensimmons’s picture

LoL, sun — YOU are the one who reported this issue, and didn't realize it's a dup. :P

sun’s picture

Yes. And Bartik should not contain any logic like this at all. (as copypasted in the OP of this issue from the original issue)

Therefore, Comment module needs to be fixed instead, and the existing code in Bartik needs to be removed.

Thus, duplicate of aforementioned issue.

sun’s picture

EvanDonovan’s picture

Looks like a nice simplification. Will try to apply to a 7.x install soon, unless someone beats me to it :)

sun’s picture

+++ themes/bartik/templates/node.tpl.php	11 Jan 2011 14:56:51 -0000
@@ -106,17 +106,11 @@
     // Only display the wrapper div if there are links.
-    $links = render($content['links']);
-    if ($links):
+    if ($content['links']):

Actually, I think we need to keep these old/current lines, since the renderable array contains additional #properties...

Powered by Dreditor.

geerlingguy’s picture

Can we update the title and re-summarize the issue? It seems the patch doesn't do what the title says, and the progression of this issue is a little confusing to me.

Jeff Burnz’s picture

Status: Needs review » Needs work

What sun is saying is that its fixed in the other patch, but they forgot to remove the cruft from Bartik as part of that patch, and that we need to keep the conditional the way it was because $links can contain additional properties, so needs to be like this (sorry I can't roll a patch right at the moment and am breezing through all bartiks issues):

$links = render($content['links']);
if ($links):
?>
<div class="link-wrapper">
  <?php print $links; ?>
</div>
<?php endif; ?>

Add a condition for teaser if we still want to do that (hide it on teasers), personally I don't like arbitrarily hiding this on teasers, not in our default theme at least.