theme_links should return list of links

this is extremely needed.

I had this concern the other day.

+1 on the patch :)

+1 for the concept. I'll try and test this later today.

Updated patch, fixed a few more issues.

Cleaning and well tested now, please review!

I leave it up to other themers to decide whether this is useful or not. :)
Do we need to escape id and class or are these assumed to be safe?

Hmm, I'm not sure if that is necessary. The only place this function is called is directly from core (with no attributes passed in) or from a theme file with PHP.

Not sure if escaping is needed at all...

looks like a feature to me. :p

Code looks good. I tested all four core themes and didn't notice any major drama apart from some subtle differences:

1. The menu gets indented slightly
2. The separator bar looks slightly different, because it is a border and not an actual "|"

I've attached a screenshot to show an example of the differences. They don't bother me personally, but figured I would post it for themers to review.

My only question is does it make sense to take this part:

return '

• '. implode('
• ', $output) .'

';

And put it in its own theme function? theme_links_list, for example? theme_links seems like an awful lot of code to copy and paste in order to override the link output in your theme. And yes, I realize this patch didn't touch this other code, but figured I'd bring it up since I'm looking at it. ;)

Bah.

This part:

Hm.. on closer review, more work is needed. For example, what used to look like:

reply | edit | delete

at the end of comments now looks like:

* reply
* edit
* delete

...in Chameleon anyway. Probably other themes as well.

A grep for theme('links' reveals the following functions which should be checked:

theme_node
theme_poll_results
aggregator_page_sources
aggregator_page_categories
theme_comment

Agreed, there will be some minor discrepencies by switching from text | to CSS borders, but that is about it.

I did a double check and all theme_links() are coming out correctly.

Webchick, did you try the latest patch I posted? I fixed a few more I missed. Also make sure to reload your CSS, as it is probably caching an out of date version :-)

I was just thinking of writing this myself.

- I think we should have a standard class added to all <ul> output by this function so we don't have to make such a large css selector which only catches the standard places.
- I'm guessing this won't work since theme_item_list isn't in too good of shape (it would put some extra HTML cruft around the <ul>), but it /might/ be a good idea to merge them since its all lists.
- I haven't tried, but I don't think this will apply since there was a round of changing '0em' to '0'.

Updated patch against HEAD.

Neil, I'm not sure what you mean by adding a standard class to the output, this seems more confusing IMO, but maybe I fail to see something.

m3avrck: I think he means something like class="link-list" so we can differentiate a list of links generated from theme_links from a list of links that might happen to be present in page content or whatever that someone posted.

I'm cool with this but am not commiting this yet -- let's figure out Neil's comment first.

something like this went in with link_alter() patch? if so, please close the issue.

No, this patch makes the default behavior for theme_links() to return a UL of links, instead of a string.

good to go.

.primary-links li, .secondary-links li, .links li, .taxonomy li

This sort of thing is sprinkled all over. Any additional modules need to add CSS to make thier links display like other links. Themes have to think about which modules they will define styles for if they would like to override the look of all links.

There needs to be a consistent class provided by theme_links() that can be used to define simple CSS rules.

Here's an entirely new patch that really consolidates things.

theme_links() is much improved to be of perfect (well I hope) use for themers... the same logic is being used to generate the menu system on mtv.co.uk .. which I wrote (yes that is a Drupal menu!)

Otherwise, themes should look exactly the same, but they will be more semantically correct.

Docs will be updated when patch goes in.

this looks cool. maybe though this function should build a links array and send to theme('item_list')? seems like there is some overlap now.

Hmm, while there is some overlap I'm not sure if they would go as well together. theme_links() is looking for an array of links setup in a correct fashion--different than theme_item_list()

I do agree there can be some cleanup, but I'm not sure if they can be merged. They seem to offer different uses of lists--one for actual navigation with special CSS classes for funky menus (like mtv.co.uk ... it's using this ;-)) and then other for just regular ol' lists.

i wasn't suggesting merging functions, just suggesting that theme_links() do some preprocessing and then call theme('item_links'). that may or may not be a good idea, as you say.

Updated for HEAD.

Moshe, yeah I don't think sending any data to theme_item_list() would help. During the loop for theme_links(), we process all of the l() and add in some attributes. We could do this and put it into an array $data and pass it to theme_item_list() but that seems like overkill.

This patch has been set to RTBC before and it's still the same--but I've condensed it even more based on Neil's feedback. I think it's ready to go finally (this will be a *huge* boon to themers).

First of all, this functionality really needs to get into core. To follow up on moshe's comments, I thought I'd weigh the benefits of using theme_item_list() in theme_links(). The plus side of using theme_item_list is that we're using less lines of code. I wrote up a modified verion of this patch which was 10-15 lines shorter by calling item_list().

After writing the patch, I found a few pit-falls with this approach.
* item_list isn't as detailed, for instance it doesn't include classes for each <li> and the 'first' and 'last' classifications
* even with improving item_list to include this information, the list is still wrapped in an unnecessary <div class="item-class"> (which breaks all the current themes, with no easy solution)
* and finally, as m3avrk pointed out to me, we're recursing through the array twice, instead of just once

So all in all, that seems like a lot of problems just to avoid 10 lines of code. I give the current approach a thumbs-up.

I leave it up to other themers to decide whether this is useful or not.

It's needed: this is the way to do this. This is semantically correct html (for those of us who care!).

Having looked over it, I would agree, yes. This is indeed the way it should be done by default. This is the kind of code that folks implementing advanced themes have to copy and paste in every time.

+1. Yummy.

Committed to HEAD.

Updated docs: http://drupal.org/node/64292#links