Flag is not currently compatible with the latest D7 (including 'alpha7'):

According to #878092: Regression from D7 alpha: themes are unable to render one group of node links separately from another, modules should not add their own 'ul' to the node links.

CommentFileSizeAuthor
node_view_links.diff578 bytesmooffie

Comments

mooffie’s picture

mooffie commented
Title: Modules shouln't add their own 'ul' » Modules shouldn't add their own 'ul'

Notes:

- The "(array)" cast in the patch handles NULL (when there are no links).

- Why havn't I fixed our hook_comment_view() as well? Because the aforementioned #878092 doesn't "fix" it. I'll keep an eye on this.

quicksketch’s picture

quicksketch
he/him
commented

Is there a reason why you used this approach:

$node->content['links']['#links'] += (array) flag_link('node', $node, $view_mode == 'teaser');

Instead of just this?

$node->content['links']['#links'][] = flag_link('node', $node, $view_mode == 'teaser');
mooffie’s picture

mooffie commented

Is there a reason why you used this approach:
[...]
Instead of just this?
[...]

#links contains an array of links that's passed directly to theme_links. It looks like:

#links == array(
  'node-readmore' => ...
  'blog-usernames-blog' => ...
  'book-add-child' => ...
  'book-printer' => ...
  'comment-new-comments' => ...
);

(These keys are used by theme_link as CSS classes.)

We want to extend this array. With the code you suggest, $node->content['links']['#links'][] = flag_link(...), we'd end up with:

#links == array(
  'node-readmore' => ...
  'blog-usernames-blog' => ...
  'book-add-child' => ...
  'book-printer' => ...
  'comment-new-comments' => ...
  0 => array(
    'flag-bookmarks' => ...
    'flag-spam' => ...
  )
);

IMPORTANT:

I read (at that linked issue) that they're going to revert this "fix" (a good thing, because the previous behavior has its advantage). So we'll eventually revert this patch as well (if we commit it).

You may say: so let's do nothing and wait for them to revert their "fix". That's one possibility, but I'm for committing this patch because:

- It costs nothing to revert it.

- We don't know when they'll revert the "fix". It turns out Flag wasn't compatible with Drupal HEAD (including alpha7) since 2010-08-30 already.

(I have one more reason but I'll explain it if these two aren't enough.)

quicksketch’s picture

quicksketch
he/him
commented
Status: Needs review » Reviewed & tested by the community

Hey mooffie, I trust your judgement and though I'm not real happy trying to chase HEAD, I think it would be fine if you committed the change. I realized the advantages to your approach after writing my last comment. Casting as an array is likely necessary in case flag_link() returns FALSE and doesn't add anything. So overall sounds great to me. Thanks for keeping up with things.

mooffie’s picture

mooffie commented
Status: Reviewed & tested by the community » Fixed

Committed.
http://drupal.org/cvs?commit=427624

Status: Fixed » Closed (fixed)

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