Clean up theme_links() and make it a simpler theme function
Bèr Kessels - November 20, 2006 - 15:53
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | theme system |
| Category: | task |
| Priority: | normal |
| Assigned: | rszrama |
| Status: | patch (code needs review) |
Description
theme_links() is, eehm, improved a lot. But it is not way, WAY too complex for a theme function.
I count:
8 Ifs elses and such clauses
1 foreach loop
4 Drupal API calls
And many more string concatenations and such.
This one does-it-all must be turned into a few much simpler theme calls, wich get fed by one or two complex API/logic functions. As it is now, the fucntion is unfit to serve as theme function.
#1
Ber is right, theme_links would give the casual themer a fit.
#2
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.
#3
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
<?php$i = 0;
foreach ($links as $key => $link) { $i++;
...
}
?>
There would be a problem if;
<?php!isset($link['href'] && empty($link['title'])
?>
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;
<?php
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.
#4
The code is improved technically with this patch. But the main issue is unsolved: complexity.
"algorythms" such as
<?phpif (($num_links = count($links)) > 0) {
?>
<?phpif(count($links)) {
?>
I know the first one performs better. But really: this is *all* about readability and understandability. We should take care of performance and so, outside of the theme layer [1].
In the case above I say: if count($items) < 1, the caller should simply not call the theme functions (i.e. not call theme('links',..) at all. Therefore, we can safely assume in the theme layer that if we are called, the caller wants a list. IF not, that caller should have "thought" twice. We (the theme layer) are dumb and simple. We don't ever do your (the caller) dirty work such as making sure if you want me at all.
Therefore, get rid of all that unneeded logic.
Furthermore, I don't like the way this function builds its own LI strings. We have a perfectly fit function for this: theme_items.
Re-using that, and dithcing the additional logic should make it possible to make this whole function a less-then-5-liner.
Which is what the theme layer wants: simplicity.
#5
FYI, I'm with Ber on this one.
#6
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.
#7
You beat me to it :-)
There would be a problem if;
<?php!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
<?phpelse {
$data = '';
}
?>
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.)
#8
@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.
#9
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?
#10
Update instructions go at http://drupal.org/update
#11
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.