Regression from D7 alpha: themes are unable to render one group of node links separately from another

I'm also seeing this with a clean install. IMO this is a critical bug.

Changing version.

How is this a critical bug?

Maybe not critical but definitely major at this stage, for themes obviously.

@steinmb - Definitely major, possibly critical... for a themer, this could be a nightmare - if you set your ul.links to clear: both; so it will always push down below the node content, then each ul.links block will go under the other, resulting in something like this (with fasttoggle and mollom turned on):

• Comments (X)

• Published   Promoted

• Report to Mollom

My comment from a duplicate issue:

Something very wrong with this array that node.tpl gets:

Array
(
    [node] => Array
        (
            [#theme] => links__node
            [#links] => Array
                (
                    [node-readmore] => Array
                        (
                            [title] => Read more
                            [href] => node/1
                            [attributes] => Array
                                (
                                    [rel] => tag
                                    [title] => argh
                                )

                        )

                )

            [#attributes] => Array
                (
                    [class] => Array
                        (
                            [0] => links
                            [1] => inline
                        )

                )

        )

    [comment] => Array
        (
            [#theme] => links__comment_node
            [#links] => Array
                (
                    [comment-add] => Array
                        (
                            [title] => Add new comment
                            [href] => comment/reply/1
                            [attributes] => Array
                                (
                                    [title] => Add a new comment to this page.
                                )

                            [fragment] => comment-form
                            [html] => 1
                        )

                )

            [#attributes] => Array
                (
                    [class] => Array
                        (
                            [0] => links
                            [1] => inline
                        )

                )

        )

    [#printed] => 1
)

Really needs input from someone who understands drupal_render().

This in node_build_content() (and similar in comment) is simply not compatible with what theme_links() expects:

  $node->content['links']['node'] = array(
    '#theme' => 'links__node',
    '#links' => $links,
    '#attributes' => array('class' => array('links', 'inline')),
  );

So we either return this to being a flat array without the module keying, or we allow theme_links() to take an array that's keyed deeper down.

Either way, I don't think the links__node thing is viable. There's no way you can run theme_links() twice and still get only one UL out of it.

Just spoke to moshe about this on the way to a session (yay Drupalcon!), and his take on it is that the links__node and links__comment specific theming is okay to be taken out.

Thanks @joachim!

Getting rid of the #theme for links__comment is fine (that didn't make much sense), but not links__node. From what I understand, this is so we can target theme_links__node() for overrides and only affect those links. It seems like changes to theme_links() may have been overlooked? I'm not sure, but I don't think getting rid of that is the fix here. I hope not.

> but not links__node. From what I understand, this is so we can target theme_links__node() for overrides and only affect those links.

Theme_links takes an array of link data and returns a UL. Currently, this implementation is attempting to theme only SOME of the node links with different versions of theme_links. That's never going to work, because there's no provision to return just part of a UL.

Do you mean instead that theme_links__node() handles ALL the node links that the different modules provide? That would work fine.

Do you mean instead that theme_links__node() handles ALL the node links that the different modules provide? That would work fine.

Yup. exactly. This is how I understand it:

theme_links() would make changes to all implementations
theme_links__node() would only make changes to the links inside nodes
theme_links__comment() would only makes changes to links inside comments
theme_links__contextual() for contextual links, etc...

Here's a patch.

Could a themer try out that theme_links__node() works as well as theme_links()?

The preprocess in RDF needed changing too.

Over to the bot again!

+++ modules/statistics/statistics.module	26 Aug 2010 11:51:40 -0000
@@ -114,19 +114,12 @@ function statistics_permission() {
-    $node->content['links']['statistics'] = array(
-      '#theme' => 'links__statistics_node',
-      '#links' => $links,

+++ modules/translation/translation.module	26 Aug 2010 11:51:40 -0000
@@ -188,11 +188,7 @@ function translation_node_view($node, $v
-      $node->content['links']['translation'] = array(
-        '#theme' => 'links__translation_node',
-        '#links' => $links,

This is not really as simple as it may look. I was aware of this potential issue for quite some time already.

Point being: The current situation allows you to easily

  <div class="admin-über-links-showed-on-höver">
    render($links['translation']);
    render($links['statistics']);
  </div>
  render($content);
  hide($links['blog']); // Never show goddamn blog links.
  render($links);

Also, the issue raised earlier about clear-fixing alignments and whatnot can be solved quite easily.

Powered by Dreditor.

Also, the issue raised earlier about clear-fixing alignments and whatnot can be solved quite easily.

You can't be serious?

It's not really clear what you are saying. Sounds like you are saying this is not a bug. I could not disagree more. I may not know the proper way to fix it, but something is really wrong, if that's the case.

EDIT:

Also...

  <div class="admin-über-links-showed-on-höver">
    render($links['translation']);
    render($links['statistics']);
  </div>
  render($content);
  hide($links['blog']); // Never show goddamn blog links.
  render($links);

This is all well and good, but only if it works at <li> level. Otherwise it's useless IMO.

@sun - @nomonstersinme is my sister and it's her birthday, so be nice. LOL. :D

This is a bug, and fixing it has more benefit than losing the ability to easily hide from within a template. There are several other ways to accomplish that. I introduced this bug when converting node and page to render system.

I reviewed the patch and it is RTBC.

Tested patch at #16 on some devel generated content.
Worked fine, and fixed up the markup [only one ul created].
Does it need some other specific testing?
If not, this patch is good to go.

Lovely to see more themers with passion here. :)

I also think this is ready to fly.

@sun - "Drupal 7: The Themers Strike Back"

Sweet!

BTW, I tested overriding theme_links__node() and theme_links() and it's all good. Thanks guys :)

+      $node->content['links']['#links']['blog_usernames_blog'] = array(

+    if (isset($variables['content']['links']['#links']['comment-comments'])) {

+        $node->content['links']['#links']['statistics_counter']['title'] = format_plural($statistics['totalcount'], '1 read', '@count reads');

We have to make sure that all modules are using their module name as prefix for each of their links though.

- Blog, Book, Statistics seem to be fine.

- Comment basically, too, but it's using hyphens instead of underscores... (?)

- Translation module only uses a language code ($langcode) as key for its links, which is wrong. We may want to defer that fix to #780316: Missing node translation links when no language detection is configured though.

Powered by Dreditor.

Hmm, this is yet again something that could be fixed in D8 if we make module_invoke_all() add a key with the name of the producing module into each piece of its array: #890660: add an alternative way of doing module_invoke_all() which sets a 'module' key in the returned info.

@sun: should we fix these in subsequent issues, or in this one?

Committed to HEAD. This is a very silly problem. :) Nice sleuthing getting it sorted out.

I should have reviewed this patch better.

One note on the _ => - changes though: The array keys are used as CSS class names for each link. In this patch, I took the time to standardize on underscores, but considering the CSS classes, I guess we should actually standardize on hyphens instead.

Translation module links will be fixed via #780316: Missing node translation links when no language detection is configured

Sorry for my confusion in #30. http://api.drupal.org/api/function/drupal_html_class/7 converts underscores into hyphens, so #30 should be fine.

Looks like my tests for Privatemsg are quite a nice indicator for core API changes... :)

I think this is an API change as it breaks all modules that were already adding their links in the old way, so we should post it to the mailing list. Adding the tag, is this the right one?

Also, I think the same should be done for comment links as well, because they still work the old way. (I changed both and was confused that the tests were still broken but turned out that I fixed the node links and broke the comment links with the change :)) So we probably want to hold up the api change information until that is done.

In case someone is looking for what's necessary for a custom module, here is the link to the commit: http://drupal.org/cvs?commit=417342. It's actually quite simple, you simply add the links directly to $node->content['link']['#links'] instead of defining your own links sub-element.

I'll announce this. Berdir, I'd appreciate it if you could just write a little longer note here about the fix. Just say what has to be done in an existing module in slightly more detail.

- I'd suggest to wait with the announcement until comment.module has been fixed as well (if we decide to do so, and I really think we do or this would be a really weird DX issue).

- The change is actually simple. In the old way, you had to define your own render-able array below $entity->content['links'] like you can see in the linked privatemsg patch. In that array, you had a #theme definition and an array of links in #links. Now, $entity->content['links] itself already has the #theme and #links property, so any links can be added directly to that.

Instead of

$node->content['links']['the_module']['#links']['the_link']

It's just

$node->content['links']['#links']['the_link']

now.

Thanks so much, @berdir.

Looking at the patch in #30

-      unset($content['links']['comment']['#links']['comment-add']);
+      unset($content['links']['comment']['#links']['comment_add']);

Shouldn't that actually wind up being unset($content['links']['#links']['comment_add']) instead?

OK. The last day or so I've been partially tasked with updating some D7 modules and themes for this API change, and after some work on it I found it to be a very unpleasant experience. The problem is, those link groups were darn useful, in a number of ways, and they aren't easy to replace if you were previously relying on them.

The change that was committed here was not only an API change, but also a lost feature (which had been in D7 for a while), so hopefully we can find a way to keep it around but in a way that makes everyone happy? So instead of continuing my unpleasant upgrade experience, I tried to write a core patch for that instead.

The attached patch is still a little rough around the edges, but I thought it was a good time to get some feedback from Drupal rendering experts, just to see if I'm totally off track here. However, the patch is actually pretty simple, and has the distinct advantage that it works :)

What it does:

1. Reverts the API change in #16 that was committed in #29 (i.e., the node link syntax in D7 goes back to where it was two weeks ago).
2. Module authors can therefore go back to putting their links in logical groups, with all the goodness that provides.
3. If in your theme, you do this:
   

   print render($content['links']);
   

   all the groups disappear and you get everything inside a single <ul>, just like you want.

4. However, the more complex theming (hiding links by group, displaying different types of links in different places in your template) is once again made possible. So if you do this in node.tpl.php:
   

     print render($content['links']['comment']);
     // ...
     // (some other stuff in between)
     // ...
     print render($content['links']);
   

   You'll get two separate <ul> lists, the first one containing all the comment-related node links, and the second containing all remaining node links. Again, I think that's exactly what you want.

5. You can implement theme functions to target any group of links that you want. Actually, we were almost there already in the original D7 code from before two weeks ago, but the theme suggestions seemed wrong. So when I rolled back #16 as part of the attached patch, instead of restoring what was actually there (things like '#theme' => 'links__comment_node'), I instead restored it as '#theme' => 'links__node__comment'. This restores what I think was the intention all along, and preserves the ability (which has been in core the last couple weeks) to theme all node links at once. In particular:
   • Implement theme_links() to affect the theming of all links on the site.
   • Implement theme_links__node() to affect the theming of all links inside nodes.
   • Implement theme_links__node__comment() to affect the theming of all comment-related links inside nodes.
6. The only price for all this (for existing code) is some tiny changes to theme_links(), which as can be seen from the patch are very small.

Any thoughts?

+++ includes/theme.inc	15 Sep 2010 15:56:17 -0000
@@ -1473,6 +1496,24 @@ function theme_links($variables) {
+function theme_grouped_element($variables) {
...
+  return theme($element['#theme_callback'], $element);

I love this suggestion (already expressed the usefulness of the separated links earlier myself), but I hate #type grouped_element ;)

It looks like everything is actually in place already:

The wrapping UL/OL + heading is a #theme_wrappers - gets the result of theme_links() (bare list items), and wraps that HTML.

Might not even need an additional #pre_render. drupal_render() should skip the top-level module keys and concatenate everything into #children already.

Powered by Dreditor.

Yeah, losing those groupings is pretty sad. It is worth fixing that, and I do think a rollback is sttarting point.

I'm not so sure about this 'grouped_element' approach. I have a really rough idea that we rollback and then put a #theme on $node->content['links'] (for example) and then that theme function does the aggregation of all the link groups into one list. this would be slightly messy but at least it is localized to links. Effectively, the #theme=links would get ignored on all the link groupings. Your render() technique would still work for using individual link groups. This needs some experimentation as I am just hand waving now.

Thinking out loudly: Why not simply a template_preprocess_links() ? It's invoked before theme_links(), can check whether #links is an array (whose keys have #properties), and conditionally flatten it.

Let's not over-engineer this - limit to the use-case at hand, as I don't know of any other similar pattern in Drupal.

/me likes sun's suggestion in #40. That seems very logical, and would fit firmly in with the themer's toolkit of changing the way things render using a preprocess function.

Re #40, my understanding was that a preprocess function for theme_links() will not work here, because it does not get the element's children as input, just the same $variables that theme_links() itself gets? So it cannot do anything to render them. If theme_links() were changed to take a render element as input instead of an array of variables, then maybe it would - but I was trying to absolutely minimize the changes to theme_links() here, this late in the cycle. Maybe I am mistaken.

****

Re #38, I had thought about #theme_wrappers, and in many ways it seems more natural - but again, I wanted to minimize changes to theme_links() at this point in the cycle - so even if we added one, I think we'd still want/need to preserve the ability of theme_links() itself to return an already-wrapped list by default.

****

Re #39 (and the other part of #40): Yes, we could just limit this to links (or even just to node links) if we want; once I had figured it out it was simple to generalize though :) But it sounds like it might be simpler not to.

If I understand what Moshe is suggesting, I think it would be something like this:

$node->content['links'] = array(
  '#theme' => 'megalinks',
  '#theme_callback' => 'links__node',
  'comment' => array(
    '#theme' = 'links__node__comment',
   ...
  ),
  'blog' => array(
    '#theme' => 'links__node__blog',
  ),
);

and then the theme_megalinks() would take a render array, and therefore could combine the work of the pre-render and theme functions I had in my patch, before passing the output along to theme_links__node()?

I think that would work - it is not too different from what I had, and I may have even tried it at one point, can't remember... Note that I left the #theme_callback idea in the above because I think we do want to preserve the ability of people to override the whole hierarchy of theme_links(), theme_links__node(), theme_links__node__comment(), etc - and I'm not sure how else to do that.

Anyway, my head is starting to spin and I'm not supposed to be working on this the rest of today :)

This is roughly what I mean.

+++ includes/theme.inc	15 Sep 2010 18:19:12 -0000
@@ -1364,6 +1364,27 @@ function theme_link($variables) {
+      elseif (element_child($superkey)) { // @todo Perhaps not required.
+        $new[$superkey] = $link;

of course that $link should have been $links

Powered by Dreditor.

Also note that my idea should be converted into a #pre_render, as this nested-links only applies to renderable arrays, not arbitrary theme_links().

I don't have much experience with this issue and its intricacies, but I skimmed through the comments and patches. I might be wrong, but I suspect that however we define $node->content['links'] (whether flat or grouped in some way), we can somehow achieve the goal of allowing a single UL by default, while having it be possible for multiple ULs each rendered in a different part of node.tpl.php.

I think the question is: who should be responsible for the grouping? Should the comment module be responsible for putting all comment related links in its own group? Or, should the code that wants to render some links separately do that? For example, should #37.4 be:

  $comment_links = $content['links'];
  $comment_links['#links'] = array();
  foreach ($content['links']['#links'] as $key => $link) {
    if (substr($key, 0, 8) == 'comment_') {
      $comment_links['#links'][$key] = $link;
      unset($content['links']['#links'][$key]);
    }
  }
  print render($comment_links);
  // ...
  // (some other stuff in between)
  // ...
  print render($content['links']);

If, on the other hand, we think it makes sense for the module that adds the link to be the one to put it into a group, as was the case before this issue, then something along the lines of #39/#40 seems doable.

Sorry, #46 was an x-post. But thinking about it more, yes to #45, I think we should revert this issue and instead add a #type=>'links' that has a #pre_render to flatten.

To be honest, I had no idea you could render/hide each of the links individually until this issue came up, so I'm glad it did. I guess it's one of those things those who fully understand the render API find really easy and think it's self-explanatory. It's definitely cool, but could use some more docs.

I prefer not messing with theme_links() unless we have to. That function needs to be simplified, not further complicated. Assuming I properly understand how this would all work, I like #43-45. In addition to leaving theme_links() alone, it provides a nice example of how to manipulate the structure of a render element to change the way it displays.

I don't know if I understand how it would all work, so I am going to try to rephrase via a concrete example :)

If I understand the proposal of the last several comments, it would result in render arrays looking something like this?

$node->content['links'] = array(
  '#theme' => 'links__node',
  // This is the function similar to the one @sun wrote, that would go through
  // and flatten the links before rendering.
  '#pre_render' = array('some_function'),
  '#links' => array(
    'comment' => array(
      '#theme' => 'links__node__comment',
      '#links' => array(
        'comment-some-link' = array(
          'title' => ...,
          'href' => ...,
        ),
        'comment-some-other-link' = array(
          'title' => ...,
          'href' => ...,
        ),
      ),
    ),
    'blog' => array(
      '#theme' => 'links__node__blog',
      '#links' => array(
        'blog-some-link' = array(
          'title' => ...,
          'href' => ...,
        ),
        'blog-some-other-link' = array(
          'title' => ...,
          'href' => ...,
        ),
      ),
    ),
  ),
);

If that's not correct, someone please provide an example that is!

I think that would work, but doesn't it mix properties and children together in an odd way? (Meaning that the first-level #links is a property which itself contains a renderable array with its own theme functions, etc.) I thought doing that kind of thing was the third rail or something :) But it would work.

It also seems to mean there'd be a bit of an awkward syntax to render this, e.g. render($content['links']['#links']['comment'], no? Though of course that would work too - PHP doesn't care about the difference between properties and children :)

Personally, though, I think I'd prefer having a slightly more code behind the scenes in order to preserve a clean array structure.

I think you can omit the top #links and let that get added by the #pre_render function. Here is some hand waving of the render array and #pre_render function

$node->content['links'] = array(
  '#theme' => 'links__node',
  // This is the function similar to the one @sun wrote, that would go through
  // and flatten the links before rendering.
  '#pre_render' = array('drupal_pre_render_links'),
  'comment' => array(
    '#theme' => 'links__node__comment',
    '#links' => array(
      'comment-some-link' = array(
        'title' => ...,
        'href' => ...,
      ),
      'comment-some-other-link' = array(
        'title' => ...,
        'href' => ...,
      ),
    ),
  ),
  'blog' => array(
    '#theme' => 'links__node__blog',
    '#links' => array(
      'blog-some-link' = array(
        'title' => ...,
        'href' => ...,
      ),
      'blog-some-other-link' = array(
        'title' => ...,
        'href' => ...,
      ),
    ),
  ),
);

function drupal_pre_render_links($elements) {
  if ($children_are_render_arrays) {
    $elements['#links'] = $flattenned_links;
  }
  return $elements;
}

Thanks. After I wrote my above comment, I had a feeling someone was going to suggest that :)

So that's mostly what my earlier patch did (same array structure), but the difference is that instead of doing everything in the pre-render function, I used the pre-render function just to mark the children, and then let the theme functions do all the work. I am now trying to remember why I split it up that way; I swear I had a very specific reason, but I don't remember what it was anymore :) Either I was confused about the order in which things happen, or I was already trying to generalize it at that point, and afraid of the general idea of an array parent presuming too much knowledge about and messing with its children (since in general Drupal rendering happens from the bottom up).

However, if we are now talking about a node-links-specific solution, that concern might be gone. We know what we expect the children to be, and we know that this is attached to an element with #theme = 'links__node' which we know takes an array of variables and therefore never renders its children anyway (if someone stuck something unexpected in there). I think this may work, and is certainly simpler.

I'll try to reroll it later today using that method.

This is what I had in mind. Turns out that the #pre_render is technically correct, but also technically ignorant, as it doesn't apply any further special callbacks (e.g., #pre_render) that may have been set on a sub-module-links bundle below. Perhaps that isn't so important though...

+++ includes/theme.inc	16 Sep 2010 14:03:29 -0000
@@ -1365,6 +1365,28 @@ function theme_link($variables) {
+function template_pre_render_links_flatten($element) {

We don't prefix any other pre_render function with "template". Should be drupal_pre_render_links_flatten(). Personally, I can do without the "flatten" suffix.

+++ includes/theme.inc	16 Sep 2010 14:03:29 -0000
@@ -1365,6 +1365,28 @@ function theme_link($variables) {
+    // The regular structure of a link item does not use element properties. So
+    // if it does, then we're dealing with a sub-links-array and need to flatten.
+    if (isset($child['#links'])) {

I don't understand this code comment. What's the regular structure of a link item? If we're in this pre_render function, and we have children, so we're in this foreach loop, isn't the child having a #links property what we're expecting 99% of the time?

+++ includes/theme.inc	16 Sep 2010 14:03:29 -0000
@@ -1365,6 +1365,28 @@ function theme_link($variables) {
+      if (empty($child['#printed']) && (!isset($child['#access']) || $child['#access'])) {
+        $element['#links'] += $child['#links'];
+      }

We should set $child['#printed] after doing this, no? Doesn't really matter when theme_links() is used to theme, since it doesn't call drupal_render_children(), but still.

+++ includes/theme.inc	16 Sep 2010 14:03:29 -0000
@@ -1365,6 +1365,28 @@ function theme_link($variables) {
+    // Take over links defined on the top-level.
+    else {
+      $element['#links'][$key] = $child;
+    }

As above, I don't understand when we ever have this use-case, and if we do, why we want to do this.

+++ modules/rdf/rdf.module	16 Sep 2010 13:38:24 -0000
@@ -500,7 +500,7 @@ function rdf_preprocess_node(&$variables
-    if (isset($variables['content']['links']['#links']['comment-comments'])) {
+    if (isset($variables['content']['links']['comment']['#links']['comment-comments'])) {
...
-      $variables['content']['links']['#links']['comment-comments']['attributes'] += $comment_count_attributes;
+      $variables['content']['links']['comment']['#links']['comment-comments']['attributes'] += $comment_count_attributes;

Might as well s/-/_/ here, while we're messing with it anyway.

Turns out that the #pre_render is technically correct, but also technically ignorant, as it doesn't apply any further special callbacks (e.g., #pre_render) that may have been set on a sub-module-links bundle below.

I actually think we're doing what is correct. A #pre_render on a child element will get used if the child element is rendered separately, just like the child element's #theme function will get used in that case. When not being rendered separately, everything gets moved to the parent, so the child element isn't rendered, so neither its #theme, nor its #pre_render gets used. I think this makes sense.

and afraid of the general idea of an array parent presuming too much knowledge about and messing with its children (since in general Drupal rendering happens from the bottom up).

#pre_render runs top-down. #theme runs top-down. I think it's okay for a parent to know about its children, where that makes logical sense, like here.

Powered by Dreditor.

Also, would be great to add tests ensuring that groups of node links can be rendered separately, but when not, that a single UL is output.

1. +      if (empty($child['#printed']) && (!isset($child['#access']) || $child['#access'])) {
   

   Very nice catch on the #access! I would love to say this validates the uneasiness I had with this approach, except for the fact that I did not think of the access bypass issue myself :) Anyway, as long as we caught it now, I guess.

2. Turns out that the #pre_render is technically correct, but also technically ignorant, as it doesn't apply any further special callbacks (e.g., #pre_render) that may have been set on a sub-module-links bundle below. Perhaps that isn't so important though...

   Yes, given that the #theme is also being ignored, I think ignoring #pre_render is OK too.

   Actually, now that I think about it, that was probably the issue that led me to not do everything in the top-level #pre_render function originally. I did not like the idea that you could define '#theme' => 'links__node__blog' but then Drupal would sometimes ignore that and never let you do your blog-specific theming. My patch in #37 solved that, but at the cost of some complexity. I'm OK with it for Drupal 7 - we want to avoid complexity at all costs at this point. I don't think it's good for Drupal 8.

3. Given the general uncertainty, perhaps we should rename template_pre_render_links_flatten() to node_links_pre_render() and move it to node.module? Better not to generalize anything if we aren't sure how it will work in the long term.

   Or at least, we should add a fair amount of documentation to explain the behavior and limitations.

Re #56.3, I think we want this for comment links too (i.e., links appearing under each comment, that can also consist of groups), and I think it's generalizable to anywhere we have links, certainly anywhere we have $entity->content['links'].

I did not like the idea that you could define '#theme' => 'links__node__blog' but then Drupal would sometimes ignore that and never let you do your blog-specific theming... I don't think it's good for Drupal 8.

Is it really so bad? So the idea is that "if the theme outputs the blog group of node links separately, then the theme can implement a specific override to how that blog-specific UL gets rendered, and if the theme does not output the blog group separately, but instead has them part of the overall node links, then the more generic links__node gets used for the full set of node links, which happens to include some links to blog related stuff". I think we have much much worse WTFs in Drupal than this previous sentence, which actually seems quite sane to me. But sure, for D8, perhaps we can split theme_links() into a #theme function for just the LI tags and a #theme_wrappers function for the UL tag, allowing each group of LI tags to get their own theme suggestion while still being part of a single UL. I think sun and JohnAlbin have been suggesting this in other issues too.

I agree that we want this for comments. IMO, the current solution is great. Lets incorporate recent fedback and then rtbc.

Ok, so this needs work then.

Upon further review, it looks like sun already upgraded comment links. We missed it because comment itself is the only module that adds comment links in core. This patch is RTBC.

My comments in #54 still stand. I haven't re-rolled myself, because some of those comments are questions that I'm hoping sun (or someone) can answer. Also, we really should add tests, since #37 indicates the earlier patch was committed with a regression because of a lack of test coverage.

yeah, I agree with effulgentsia's and also David's review issues. No time to re-roll now, but hopefully later this weekend.

I should be able to grab this and finish it up a little later today (unless you already started, @sun).

Also, we really should add tests, since #37 indicates the earlier patch was committed with a regression because of a lack of test coverage.

As far as I know, the earlier patch was committed with a regression on purpose (intentionally removed the feature in order to fix a bug). But I agree it would be good to add a test anyway.

As far as I know, the earlier patch was committed with a regression on purpose (intentionally removed the feature in order to fix a bug).

See #49.1. I don't think enough people really knew they were removing a feature, and given the lack of a test or docs about it, that's not surprising.

But I agree it would be good to add a test anyway.

2 tests (see #55). :)

I think this should address everything, and with plenty of tests.

The one thing I didn't do was this:

+++ modules/rdf/rdf.module	16 Sep 2010 13:38:24 -0000
@@ -500,7 +500,7 @@ function rdf_preprocess_node(&$variables
-    if (isset($variables['content']['links']['#links']['comment-comments'])) {
+    if (isset($variables['content']['links']['comment']['#links']['comment-comments'])) {
...
-      $variables['content']['links']['#links']['comment-comments']['attributes'] += $comment_count_attributes;
+      $variables['content']['links']['comment']['#links']['comment-comments']['attributes'] += $comment_count_attributes;

Might as well s/-/_/ here, while we're messing with it anyway.

That affects other places too, so I thought it would be better to save for another issue.

Also, for this:

+++ includes/theme.inc	16 Sep 2010 14:03:29 -0000
@@ -1365,6 +1365,28 @@ function theme_link($variables) {
+    // Take over links defined on the top-level.
+    else {
+      $element['#links'][$key] = $child;
+    }

As above, I don't understand when we ever have this use-case, and if we do, why we want to do this.

It seemed to me like the thing to do here instead was to recurse deeper (and allow multi-level grouped links), so I tried it that way instead. It seems to work well.

It occurs to me that maybe we should check if all of this hurts the performance of rendering node/comment links. I don't think it will change it noticeably, but since there can be a lot of those on a page (especially comment links) it is worth thinking about.

Holy awesome documentation. David, this is great!

Yeah, that’s crystal clear.

RTBC +1.

Summary for webchick or Dries: #29 introduced an unintended regression described in #37. Themes which prior to #29 were able to print out some of the node links separately from the rest of the node links (e.g., blog related links in one part of node.tpl.php and the rest of the node links in another part of node.tpl.php) were no longer able to do this after #29. This isn't that much of an edge case; David discovered this, because we have themes on drupalgardens.com that broke. #65 fixes the regression, while also fixing what this issue originally intended to fix: ensuring that multiple UL tags aren't output when there's no reason to.

It occurs to me that maybe we should check if all of this hurts the performance of rendering node/comment links. I don't think it will change it noticeably, but since there can be a lot of those on a page (especially comment links) it is worth thinking about.

I agree it makes sense to benchmark a page with 30 node teasers and a node page with 50 comments, both for a typical anonymous user, and for someone who has access to multiple link groupings. If someone has time to do that, that would be awesome. But I don't think it makes sense to hold up a regression fix, waiting for those benchmarks.

[EDIT: double post. please ignore.]

.

I don't think benchmarks are needed here. At least they need not block the bug fix. This is run of the mill copying of a few small arrays from one variable to another.

I rerolled with a slight performance improvement. Should still be RTBC, but someone might want to take a quick look? (I attached the interdiff also, to make it easy).

I decided to do some quick benchmarks anyway - mostly because I haven't done any before and this was a good chance to experiment with it. I put 30 nodes on the front page and gave anonymous users enough access to see three typical node links per node ("read more" + two comment.module node links). I also made sure page caching was turned off :) The results are attached, and indicate no significant performance difference, so that's good. (Yeah, I guess it probably is what we expected, but when you get a lot of nodes on the page, a lot of that node.module code does run over and over again, etc., so I'm somewhat glad I checked.)

+++ includes/common.inc	20 Sep 2010 15:17:35 -0000
@@ -4963,6 +4963,101 @@ function drupal_pre_render_link($element
+ * This function can be added as a pre_render callback for a renderable array,
+ * usually one which will be themed by theme_links(). It iterates recursively
+ * through all unrendered children of the element, collects any #links
+ * properties it finds, merges them into the parent element's #links array, and
+ * prevents those children from being rendered separately.

This can be heavily shortened -- this #pre_render only works for theme_links, resp., #links and nothing else.

+++ modules/simpletest/tests/theme.test	20 Sep 2010 15:17:36 -0000
@@ -165,6 +165,132 @@ class ThemeItemListUnitTest extends Drup
+class ThemeLinksUnitTest extends DrupalWebTestCase {

Only tests that extend DrupalUnitTestCase should use "unit" in their name - all other just use "TestCase", i.e., ThemeLinksTestCase.

+++ modules/simpletest/tests/theme.test	20 Sep 2010 15:17:36 -0000
@@ -165,6 +165,132 @@ class ThemeItemListUnitTest extends Drup
+      'name' => 'Themed links',

Just "Links" is sufficient.

+++ modules/simpletest/tests/theme.test	20 Sep 2010 15:17:36 -0000
@@ -165,6 +165,132 @@ class ThemeItemListUnitTest extends Drup
+  function testDrupalPreRenderLinks() {

This test can use the testing profile, just declare

protected $profile = 'testing';

at the top of the test case.

+++ modules/simpletest/tests/theme.test	20 Sep 2010 15:17:36 -0000
@@ -165,6 +165,132 @@ class ThemeItemListUnitTest extends Drup
+    $dom = new DOMDocument();
+    $dom->loadHTML($child_html);

I suspect you can use $this->drupalSetContent() here.

+++ modules/statistics/statistics.module	20 Sep 2010 15:17:36 -0000
@@ -114,12 +114,19 @@ function statistics_permission() {
 function statistics_node_view($node, $view_mode) {
...
+    $links = array();
     if (user_access('view post access counter')) {
...
       if ($statistics) {
...
+        $links['statistics_counter']['title'] = format_plural($statistics['totalcount'], '1 read', '@count reads');
       }
     }
...
+    $node->content['links']['statistics'] = array(

mmm... this is something we should discuss -- should every module unconditionally add its links container, even if it doesn't add any links?

Somehow doesn't sound correct to me. (?)

Powered by Dreditor.

Just "Links" is sufficient.

Fixed.

Only tests that extend DrupalUnitTestCase should use "unit" in their name - all other just use "TestCase", i.e., ThemeLinksTestCase.
...
This test can use the testing profile, just declare
protected $profile = 'testing';
at the top of the test case.

Fixed both of these by just switching to DrupalUnitTestCase instead :) (There didn't seem to be any reason not to, and the tests still pass just fine. I was a little surprised it worked, though.)

Note by the way that the other existing tests in that file use "unit" in their name in a similar way, so they are not using the correct convention either.

*************

This can be heavily shortened -- this #pre_render only works for theme_links, resp., #links and nothing else.

I didn't change this. It actually will work just fine for anything else that happens to use #links, which isn't limited to theme_links() necessarily.

I suspect you can use $this->drupalSetContent() here.

Maybe, but that function seems designed for putting the content somewhere where the Simpletest browser can interact with it, but here we don't really need that, so I didn't change anything.

mmm... this is something we should discuss -- should every module unconditionally add its links container, even if it doesn't add any links?

Interesting question, but I believe all this patch does in that regard is revert the previous patch that was committed in this issue, so maybe that could be discussed elsewhere.

The benchmarks in #72 are discouraging: patch causes a 1% slowdown (median of 402ms vs. 398ms) for a many node teaser page. A 1% slowdown has stopped other patches from being committed. Thoughts on whether it's worth it here? I suspect more optimization is possible, like not doing the recursion.

We should get this one in.

A few milliseconds isn't a whole lot (especially when you have to visit tons of pages for it to even become statistically significant) - however, I guess they do add up, and committing lots of patches that did that would not be a good idea :)

So, here is a revised version that removes the recursion. But if we're worried about performance, we also definitely should not be applying this to comment links (unlike node links, this feature never even existed on comment links before, so adding it to them should be a separate issue anyway); therefore, this version of the patch does not add it for comment links.

I'm also attaching new benchmarks. Looking at the median values, they don't show much of an improvement though (the patch is 3ms slower than HEAD now, vs 4ms before). You can see my second test (the "without patch" one) had a huge standard deviation though, so these tests are a little suspect in general - something else disruptive must have been running on my computer. The question is whether it's worth doing tons more benchmarking? Overall, I think we know the effect of this patch on performance is not that big.

Code looks good. Might as well go with this.

I ran some new benchmarks (attached) without 900 other programs running on my computer at the same time, and these should now be believable :) With the patch, the front page is 2 msec slower than HEAD (barely statistically significant, and averaging out to less than 0.1 msec per node teaser), so I think we are OK here.

As a reminder for core committers, the issue summary is in #68, and the patch is not a "real" API change since it simply reverts an API change that was made a couple months ago.

It may not be a "real" API change, in terms of disqualifying it from getting committed, but it's real enough to break existing code, right? Including people who fixed their modules after the original commit? So we'll need to announce it after commit.

Right, for module authors who have been tracking HEAD very closely, they would have to do a (pretty simple) update of their node links for this, so it would definitely be a good idea to announce.

For module authors who have not been tracking HEAD very closely, this removes some work they would otherwise have had to do :)

For theme authors who wrote something that depended on this feature (which was in D7 up through about a year after code freeze), this removes the need for them to figure out how to deal with its removal, since it won't be removed anymore :)

Just noting that I would hate to end up being partly responsible for removing a really useful feature from D7, that I did not even know existed, while trying to fix a markup error. I hope this is committed soon. :)

Given that #949576: Missing hook_entity_view() and hook_entity_view_alter() just went in, it would really be a good idea to make comment links consistent, so as to allow modules to add links to entity_view in a standard and consistent way. According to the latest benchmarks, it looks like the latest patch doesn't really have an impact, and we can add back those changes?

OK, well, consistency is not a bad argument :) Here it is with the comment link changes back in. If someone has a problem with that and thinks it's too much of a new feature, we can still go with #76 instead.

Thanks!

Confirming David's benchmarks. Because his had high stddev, I re-ran on my machine with no other software running, and with a well primed APC cache, but got essentially the same results:

Front page showing 30 node teasers:
HEAD: 159.4ms
Patch: 161.0ms (+1%)

Node with 50 comments:
HEAD: 195.6ms
Patch: 198.0ms (+1.2%)

This is for an anonymous user with default permissions, so each node teaser has a "read more" link provided by node.module and a "Log in or register to post comments" link item provided by comment.module, and each comment just has the latter.

Dividing the first difference by 30 results in 53us.
Dividing the second difference by 51 (1 for the node + 50 comments) results in 47us.

So, approx. 50us for each invocation of the new drupal_pre_render_links() function. This seems high to me, despite element_children() being known to be somewhat slow. I haven't traced it down further to see where we can micro-optimize.

But, I'm still +1 for this patch. The ability to render different groups of links separately is a feature we should have never removed, and as per initial issue work, it's undesirable to have separate UL tags when the groups are not being rendered separately.

If adding 1% to pages that show many entities is unacceptable, please say what would be acceptable, and maybe we can achieve it with some micro-optimizations.

Adding 1% to commonly viewed pages justifies the "Performance" tag.

#83: drupal.links-flatten.83.patch queued for re-testing.

#83: drupal.links-flatten.83.patch queued for re-testing.

Erg. It does look like this needs to go in before RC. Adding to my "to review" queue for tomorrow.

It does look like this needs to go in before RC.

Yes. It's an API change from HEAD, so if it's gonna go in at all, then it needs to go in before RC. Therefore, "critical".

Summary in #68. Benchmarks in #85.

Shoot. I guess that actually should've gone in before beta3. :\ Now we're going to break hook_links() in every module again. :(

1% slowdown is not great. :\ But I don't really know how to respond to the request about what would be an acceptable slowdown. And I think that for the purposes of this issue, we're trying to fix a regression here (the performance slowdown was likely present before #29 was committed), and further optimizations would be a follow-up, and could be committed in 7.1, 7.2...

On balance, I think it's important to fix this regression ASAP, so committed to #83 HEAD. Thanks a lot, and sorry this stayed off my radar for so long.

OK, now if somebody could explain the API change *this time* and how it's related to the last breakage :-)

Please explain what people have to do to fix their modules, and I'll announce it.

Thanks,
-Randy

@rfay: The change affects the $node->content['links'] and $comment->content['links'] arrays, and therefore, all modules and themes that use those arrays.

For modules that add to these arrays within hook_node_view() and hook_comment_view(), see the blog.module and book.module hunks in #83 for examples of what your module should likely be doing. It should likely also place its links within a per-module sub-key of content['links']. Note, it doesn't have to be per-module. The sub-key of content['links'] represents some logical grouping of links. For example, blog-related links in $node->content['links']['blog'] and book related links in $node->content['links']['book']. However, if your module adds a link that you want to place in another module's group, that's fine, add it to the group where you think it belongs.

For modules and themes that implement alter hooks or preprocess functions that do something with existing links within these arrays, you need to adjust your code to use their new location. See the rdf.module hunk in #83 for an example. Note, the "new" location is the same as what it was before #29.

If your module defines an entity type, and you want that entity type to follow the same pattern for its links that nodes and comments use (consistency++), then look at the node.module and comment.module hunks in #83 for how to do that.

I updated the documentation for this at http://drupal.org/node/224333#node_links as well.

(Although amusingly, it didn't need much updating since the previous change had not been documented either, so now we're mostly back full circle; all I had to change was to make the #theme property in the example use 'links__node__blog' like core now does.)

Two short follow-ups to explanation in #93:

- I didn't really follow the discussion here but I think the API for modules adding links is now like it was before the initial patch in this issue. So if they didn't update their code for the first patch, they don't need to do anything now. Correct?

- It actually looks adding links the wrong way still works. I updated Privatemsg to the first patch and updated core now but the links are still displayed and my tests still pass. So it doesn't actually break anything but modules really should update their implementations so that themes can display links grouped and so on.