Various bugs in theme_links()

Ber is right, theme_links would give the casual themer a fit.

Well, while I'm not a themer, I imagine I have a little more coding skill than the casual themer might have. I had to monkey around with this function on a D5 site and saw some simple things to do to clean up the code and make it smaller. I imagine it needs more code comments, but because of the nature of Ber's original post, I'll hold off and wait for feedback on what I've done.

This initial patch may be a step in the right direction. Instead of concatenating text onto the output as we go, I'm storing the li's in an array and imploding them to form the output. This would've saved me some headache when I was trying to force read more links to always be the first in a list.

If we want to break it down further, perhaps there should be a theme_link that theme_links calls, but theme_link is pretty ambiguous imo.

So, I'm not sure what further steps should be taken, though I'm happy to work on suggestions, but I think that we should at least work this patch in even if nothing else gets added to it.

I think the approach by rszrama is good, but there are too many counts going on.

I have used i++ approach, but I think this should be immediately after the foreach to show what is happening, and in case someone adds code that uses i at the end of the loop (it was not quite at the end before, but the following code didn’t use i). I’m tempted to write

 $i = 0;
foreach ($links as $key => $link) { $i++;
  ...
} 

but will resist :-)

There would be a problem if; !isset($link['href'] && empty($link['title']) as the value of $contents from the previous link would be used. I’ve added a continue; , with an alternative solution in comment. The alternative ensures an empty item will be output with the correct class, as at present.

Also using counts would not output class='last', and possibly get class='active' on the wrong item in the above condition – though it would always output class='first', whereas mine might not: can combine approaches or use alternative, if this is an issue that needs addressing.

Finally I’ve eliminated $output as it just needs returning, and I think php doesn’t need the last return.

Should we then just have;

 function theme_links($links, $attributes = array('class' => 'links')) {

  if (count($links) > 0) {
    return '<ul' . drupal_attributes($attributes) . ">\n" . implode("\n", _theme_links($links, $attributes)) . '</ul>';
  }
} 

and factor out the loop into _theme_links(), with theme_link adding the li tag? Should _theme_links() include the implode()?
Not sure if these functions are named correctly.

FYI, I'm with Ber on this one.

Here's a patch that implements Ber's suggestion to use theme_item_list() to build the list.

There's still some logic in the code, but everything that was redundant with theme_item_list() has been removed.

You beat me to it :-)

There would be a problem if; !isset($link['href'] && empty($link['title'])
as the value of $contents from the previous link would be used. This doesn’t happen at present. We should set

 else {
  $data = '';
} 

at least; or not add an element to $list[].

How do we tell people to theme theme_item_list() as theme_links() is now a wrapper and shouldn’t be themed?

Also '.' has gone missing from @param $attributes documentation again. (OK, maybe it should be a separate patch.)

@zeta ζ: good catch on the uninitialized variable. This new patch fixes this by initializing $data to an empty string, and adds the '.' in the function documentation.

Both theme functions can still be overridden. There are many theme functions who call other theme functions, I don't think this change would need any special documentation.

This catch was in #3. I’d prefer to see an else, as its absence was what flagged the bug.

My intention with #3 was not to solve the complexity of the original – despite Bèr’s response – but in the hope that the final solution would be based on more solid foundations. Therefore, I’m disappointed that flobruit started from scratch – and repeated the initial error.

Nevertheless, #8 looks good (but see above): though I haven’t yet tested it.

I’m still wondering how we tell people to theme theme_item_list() instead: Is just a comment sufficient?

Update instructions go at http://drupal.org/update

Sorry, I forgot to add a link to http://drupal.org/node/256827 Clean up theme_item_list() and make it a simpler theme function.

Patch #8 need to be rerolled.

Also, in #8,

+ // Some links are actually not links, but we wrap these in <span> for adding title and class attributes

needs to end with a period.

Bumping to D8, but if someone has energy to work on this for D7 and comes up with something that doesn't break any APIs, which are now locked for D7, please set back to 7.x-dev.

Latest patch only moves the badness to theme_item_list(), while that function is even more complex than theme_links(). While that may be semantically correct, I don't like the idea of having to potentially override *two* functions to change a single thing.

And yes, I think we can heavily simplify this function without breaking anything. (I recommend to hit the "Hide deletions" button of Dreditor)

oopsie :)

Additionaly takes #569254: theme_links heading only allows class attribute into account. I already wondered why it only allows for a class and not all attributes when I saw that earlier.

Any feedback? I really think that this is a major improvement compared to the current theme function body.

subscribing.

This is great, but can we avoid this += array stuff?

+++ includes/theme.inc	31 Aug 2010 19:22:17 -0000
@@ -1396,73 +1396,78 @@ function theme_link($variables) {
+        $link += array(
+          'html' => FALSE,
+          'attributes' => array(),
+        );

and this:

+++ includes/theme.inc	31 Aug 2010 19:22:17 -0000
@@ -1396,73 +1396,78 @@ function theme_link($variables) {
+      $heading += array(
+        'level' => 'h2',
+        'attributes' => array(),
+      );

I think it's more confusing to the average noob than the previous code.

Powered by Dreditor.

Actually, I guess the confusion would go away with a comment explaining what this does.

Alright! Added a comment to both locations in order to clarify that code.

Great, thank you. That's better :)

@Dries/webchick: I'd like to repeat that it's easier to see the resulting massive simplification of this patch by hitting the "Hide deletions" button of Dreditor. Without doing so, the changes look quite scary, but they are definitely not - it's only the resulting logic that matters.

Just because someone is upset doesn't mean we have to mark something as won't fix. Someone else is more than welcome to pick up work on a ticket.

Looks like a nice TX improvement, but too late for Drupal 7.

This issue will be moved back into the D8 queue, after it has been committed to Edge 7.x-1.x.

So here's the patch. Since there is nothing yet, the patch also has to create the baseline of module files. Let's not get distracted by them.

Tested this manually and works perfectly.

Looks good! We should add tests tho ;)

Alright. I just learned that Drupal core does not even have any tests for theme_links(), so I had to write some.

Since the testbot is not enabled for Edge yet, we need to run them manually.

Committed the baseline of module files separately to simplify this patch.

Massively improved the theme_item_list() tests, which revealed quite some bugs in the nested child list handling, as well as empty list handling.

I'm quite sure that those bugs also exist D7 core (though I didn't run the tests with theme_item_list() in core).

Fixed that comment. Just ran the tests with core's theme_item_list(), and yeah, it outputs a list container and heading even if there is no list, and nested child list attributes are utterly broken.

Re-rolled against HEAD. Improved the tests to account for a heading without links.

Sweet! This one is good to go too. :)

Thanks for reporting, reviewing, and testing! Committed to HEAD.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

Moving back to core.

Just to clarify why this suddenly needs work, and for later reference:

theme_links() in core...

1. outputs invalid CSS classes for link list items, since drupal_html_class() is not invoked.
2. does not output so called zebra classes (i.e. "odd" and "even").
3. outputs newlines between list items, which do not allow for certain, common use-cases of themers.
4. is insufficiently tested.

The last patch for core did not sufficiently account for most of these bullets, so it has to be redone based on the committed patch.

Bugs cannot target D8 at this point, because the development tree is not even open yet.

I'd be most grateful if someone would be able replace the code in core 1:1 with the code + tests in http://drupal.org/project/edge.

It's all ready and merely a matter of replacing it. Even tested on D7 production sites.

From Edge with love.

This patch still goes hand in hand with #256827: Various bugs in theme_item_list() - when either one is committed, the other one needs to be re-rolled.

#256827: Various bugs in theme_item_list() landed, so this one is ready, too.

I'm not able to confirm those test failures locally. The entire test case successfully passes for me. Anyone any clues?

needs a reroll.

Squeezed some debug statements in there.

So the testbot believes that the current path is the front page for some reason, and thus adds the "active" class to the last link:

Expected:

'<h3 id="heading">Links heading</h3><ul id="somelinks"><li class="a-link odd first"><a href="/checkout/?q=a/link">A &lt;link&gt;</a></li><li class="plain-text even"><span>Plain &quot;text&quot;</span></li><li class="front-page odd last"><a href="/checkout/">Front page</a></li></ul>'

Actual:

'<h3 id="heading">Links heading</h3><ul id="somelinks"><li class="a-link odd first"><a href="/checkout/?q=a/link">A &lt;link&gt;</a></li><li class="plain-text even"><span>Plain &quot;text&quot;</span></li><li class="front-page odd last active"><a href="/checkout/" class="active">Front page</a></li></ul>'

Fixed in attached patch by enforcing/overriding the current path to be the front page path.

Gosh, testbot stuck on #55. And I already wondered why this is not RTBC yet. :(

+++ b/core/includes/theme.incundefined
@@ -1561,73 +1561,80 @@ function theme_link($variables) {
+      $item = '';

Why set this up here if it *going* to be set down below? It confused me at first.

+++ b/core/includes/theme.incundefined
@@ -1561,73 +1561,80 @@ function theme_link($variables) {
+        $item .= '<span' . drupal_attributes($link['attributes']) . '>';

This can become = instead of .= if the first change is made.

Also, out of habit, I would have used !empty around the check for $item and $heading, but that's just me.

17 days to next Drupal core point release.

Incorporated #57.

Ok, this looks good to me for 8.x.

Personally, I think attempting to backport will be a waste of time. Much smaller theme layer backports have been attempted and failed, so removing the tag.

Oh I meant to come RTBC this after it came back green. My concerns in #57 were addressed, thanks sun.

Also, Jacine, way to be such a realist :P

Looks great to me - both the changes and the additional test coverage. Committed/pushed to 8.x.

Any change we could have this backported to D7 please?

We decided to not backport this, as it minimally changes the markup and behavior of theme_links(), and custom themes might not be prepared for that.

This reasoning is ridiculous, given the scope and changes of this patch. That's why I tried hard to still push it into D7 before its first stable release. Unfortunately, that was rejected.

You can simply install http://drupal.org/project/edge module to get the improved functionality.