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

I was mistaken on l(), but theme_link() still has the issue.

I thought of adding another option which could be past to l() as the $html parameter so that it will not run the check_plain() over the title, but this will not work for links where there is only a title.

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

Yes links can be altered, but in the end they are going to be past through theme_links(). Then the title will always be past through check_plain().

This will also be an issue where you are trying to add images to links, in the my situation where I only want part of the text to be a link.

1 Idea would be to add a html option to the links array which will stop it from being past to check_plain().

Please let me know it this is an OK option and I will build a patch.

Gordon, you are refering to this line of code?

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 am not sure as this breaks stuff in the E-Commerce module, and also the spam module.

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 have tested this patch with E-Commerce, and it fixes the problem.

Thanks chx.

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?

css is not really an option, as the image is the link.

In the case of E-Commerce it the following line which was value in 4.7, now cannot be done.

Now modules like E-Commerce and the service links need to patch the theme to get functionality which is currently available in 4.7.

"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.