theme_links() don't allow for the html in the title (comment links broken)

The links can be altered now. Would that allow you to work-around the limitations?

Gordon, you are refering to this line of code?

  //Some links are actually not links, but we wrap these in <span> for adding title and class attributes
        $output .= '<span'. drupal_attributes($link['attributes']) .'>'. check_plain($link['title']) .'</span>';

If so, yes perhaps adding a $html = FALSE and then doing if ($html) // don't do check plain

I would be cool with that.

not critical

I'm creating a list of icons that are links; this makes it impossible to use the theming functions. Adding a simple $html = FALSE param at the end of the function would be much smoother.

First of, this is critical. Second, the fix needs to be per link, not per theme links call, or so I think. If I think right then patching this is simple.

I made a small logic flaw in the patch but it's too late to find on first read :)

Above patch catches text-only entries, but not calls to l(). This one catches both. let it be known: chx wrote this version, too, including the comment! ;-)

I don't like the proposed solution.

You can overwrite the theme function if you want to change this behavior.

If that is not sufficient, I suggest we refactor the functionality so that the filtering happens on the module-side.

Dries, this is a matter of API changes in 5.0 breaking functionality that worked in 4.7.

Previously, we did allow modules to handle the filtering: theme_links() just took an array of HTML chunks. Now, it takes an array of structured arrays.

You can overwrite the theme function if you want to change this behavior.

The problem is that the theme function *used* to allow it but now does not. It seems broken to remove a perfectly reasonable case and ask every module developer who wants to use icons as links to copy-paste a core function, implementing their own custom version.

If that is not sufficient, I suggest we refactor the functionality so that the filtering happens on the module-side.

This patch makes it an option. Set #html => TRUE, and you're in charge of the filtering.

If you want icons, just use CSS to add them?

"You can overwrite the theme function if you want to change this behavior" since when can a module override a theme function? We do not have a too good distribution vehicle for template.php snippets.

"If that is not sufficient, I suggest we refactor the functionality so that the filtering happens on the module-side." Steven told me that the way Drupal tries to be more secure is that we provide such an API which helps the run-of-the-mill developer to not make security holes. The new t() function is a big step forwards to this direction as was the big check_plain patch which made l() more secure in 2005 March. I seriously doubt that we want to step back and ask every module developer to check_plain for his link.

Looks okay to me after reading the last patch.

The 'this item is in your shopping cart' example is awkward. Maybe you are trying to abuse the link system in ways you shouldn't? It looks more like textual information than a link. The function is call theme_links(), theme_text_with_links().

I don't think it's particularly grave abuse of the system. We've removed fairly straightforward functionality, and this patch restores it in a way that's very simple and clean.

The function *already* has tons of special casing code for 'links' that are only text, not links at all. So clearly, that's a supported use. This patch just makes it possible to have more complex HTML in the link -- a list of icon-buttons, for example.

There is a precedent for this in core and I believe it is broken at the moment, "login or register to post comments." I think this patch will need to fix that.

OK, then we fix comment links in this patch.

For me this illustrates that we did the wrong thing with the theme_links() patch. Hackability has been significantly reduced, and the complexity has been significantly increased. Maybe that is something that ought to be roll-backed in Drupal 6.0.

I'm OK to commit this patch for now. Haven't had time to test it yet.

Dries, hackability in *general* is much higher now because we store more meta-information about each link, and via hook_links_alter, modules can muck around with links before they're rendered into a chunk of HTML. We just 'capped' a lot of the hackability at the very last stage, when we forced everything through check_plain().

Overall, I think the links patch was a very valuable addition (though, in retrospect it would've been nice to set it up as a formsapi formatted array... but that's something for 6. ;))

Committed to CVS HEAD. Thanks.