Rationalize calls to check_plain() in l() [#698992]

l() always calls check_plain() on the output of url() even though there's no XSS risk. This is due to #399488: Invalid markup generated by l(). (which mrbaileys pointed me to in irc) - check_plain() doesn't only fix XSS.

However, we only run into that issue if $options['query'] is passed - so should be able to skip the check_plain() check in other cases.

Patch is simple, here's micro benchmarks:

HEAD:
5.81 seconds for 100,000 iterations

Patch:
5.51 seconds for 100,000 iterations

That works out about a 3ms gain for every 1000 calls to l(). I've seen over 1500 calls to l() on some pages, so that's not too bad.

Comment	File	Size	Author
#6	698992-encoding-madness.patch	810 bytes	Damien Tournoud
#6
	l.patch	1.04 KB	catch

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

catch

he/him

English

CreditAttribution: catch commented 29 January 2010 at 14:26

Status:

Active

» Needs review

Comment #2

mfb

they or he

CreditAttribution: mfb commented 31 January 2010 at 01:43

Status:

Needs review

» Closed (works as designed)

l() says it accepts external URLs, not just internal drupal paths, thus could contain & which needs escaping.

There's really no way to get around HTML-escaping pretty much anything that is being output as HTML -- unless you have some means of guaranteeing that escaping is irrelevant, such as an integer

HTML-escaping is not just about XSS risk, it's about proper HTML format.

Comment #3

catch

he/him

English

CreditAttribution: catch commented 31 January 2010 at 14:33

Status:

Closed (works as designed)

» Needs work

We could still add an $options key to l(), or strpos('&') or something, re-opening until this is exhausted, which it isn't just yet. Also crosslinking #687712: Optimize rdf.module for displaying multiple comments for background.

Comment #4

scor CreditAttribution: scor commented 31 January 2010 at 16:11

subscribing

Comment #5

mfb

they or he

CreditAttribution: mfb commented 31 January 2010 at 16:36

This kind of semi-HTML-escaping isn't developer friendly.
Imagine you are getting URLs from user input or external source like search engine API. If passed thru to l() as-is, the URL string would not be properly escaped. If the developer runs the string thru check_plain() before l() then it would be double-escaped by l().

Comment #6

Damien Tournoud CreditAttribution: Damien Tournoud commented 31 January 2010 at 16:52

Status:

Needs work

» Needs review

File	Size
698992-encoding-madness.patch	810 bytes

Nope. This call *is* required. We output something in HTML context, we need to encode it. drupal_attributes(), used on the same line, does exactly the same thing.

I'm tired of explaining that, so let's commit this small comment patch.