l() allows you to specify attributes that are added to the link tag, e.g. a title (l('Hello', 'node/1', array('options' => 'attributes' => array('title' => 'Welcome to the site', 'class' => 'hello-link'))).
The specified attributes are passed on to drupal_attributes() that takes care of check_plain'ing the values. But for some reason, l() calls strip_tags() on $options['attributes']['title']. This makes it impossible to specify a title containing something that looks like an HTML tag.
For instance, assume you have written an article about the <title> HTML tag and you want to link to it using l(). You want the generated HTML to look like this:
<a href="/node/1" title="Everything about the <title> tag">The <title> tag</a>
This is currently not possible. If you try calling l() with these parameters:
l('The <title> tag', 'node/1', array('attributes' => array('title' => 'Everything about the <title> tag')));
l('The <title> tag', 'node/1', array('attributes' => array('title' => 'Everything about the <title> tag')));
you will end of with this:
<a href="/node/1" title="Everything about the tag">The <title> tag</a>
<a href="/node/1" title="Everything about the &lt;title> tag">The <title> tag</a>
and that results in these two tooltips:
Everything about the tag
Everything about the <title> tag
The expected tooltip was this:
Everything about the <title> tag
The strip_tags() call was introduced in #186963: Links show ugly descriptions. The problem then was that the menu system is saving HTML into $options['attributes']['title'], and that array is saved into {menu_custom}.options and then passed this on to l(). I think the issue should have been fixed in the menu system instead of making a workaround in l().
This patch adds a new column, {menu_custom}.link_description, that contains the HTML description. The patch does not include a D6->D7 upgrade function and probably needs some additional testing.
What do you think of this approach? Is this too late for D7?
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | l_strip_tags-689672-15.patch | 18.6 KB | jessebeach |
| #14 | l-strip-tags-5.patch | 17.65 KB | c960657 |
| #12 | l-strip-tags-4.patch | 12.45 KB | c960657 |
| #9 | l-strip-tags-3.patch | 9.6 KB | c960657 |
| #4 | tooltip_test.png | 18.86 KB | pasqualle |
Comments
Comment #1
c960657 commentedComment #3
c960657 commentedUpdated the tests.
Comment #4
pasquallethe (html) menu description + (stripped) tooltip test looks good..
Comment #5
retester2010 commented#3: l-strip-tags-2.patch queued for re-testing.
Comment #7
damien tournoud commentedI support fixing this issue in D7.
Comment #8
lars toomre commentedThis came up today in Drupal patch bingo.
Does this issue still need to be resolved or should it be closed?
Perhaps the way to move this forward is to write tests that check the conditions defined at the top of this issue?
Comment #9
c960657 commentedIt is still relevant. This is a reroll. The patch is still missing an update hook. Marking as "needs review" in order to get the test bot's opinion.
Comment #10
pillarsdotnet commentedBack to cnw for the update hook and tests.
Comment #11
pillarsdotnet commented#9: l-strip-tags-3.patch queued for re-testing.
Comment #12
c960657 commentedRevived the old patch. Slightly different approach (I don't remember all the details). Let's see what the test bot has to say about it.
Comment #14
c960657 commentedComment #15
jessebeach commentedI rerolled the patch. Got the following error when applying the patch in #14
We tested this patch through a fresh standard install without incident.
The behavior of the patch matches the description. Still looking at the code.
Comment #16
mgifford#15: l_strip_tags-689672-15.patch queued for re-testing.
Comment #18
dawehnerMHO for now we should just strip it, in case its not
MarkupInterfaceComment #27
quietone commentedl() was deprecated in Drupal 8.0.0, https://www.drupal.org/node/2346779. It is used in Drupal 7 so changing the version.