Introduce placeholder magic into t()

Actually I already have a finished conversion of core sitting here. I just got busy with eaton's install patch :/.

For those who just tuned in:

This patch tries to address the fact that nearly every call to t() uses check_plain() or theme('placeholder') on its arguments (or at least that's what people should be doing). Putting theme('placeholder') calls inside a t()-argument array means a lot of nesting, a lot of long lines and code that is hard to read.

So, why not let the conversion be done by t() itself, and make st() identical? We already implemented a similar technique in install.inc's st() function (rather on a whim), which is hardcoded to pass its arguments through theme_placeholder(). This has the benefit of reducing code bloat, but forces everyone to use the emphasized placeholder style. You might have noticed that the installer tends to have lots of italics.

So, here's an even better solution: we let the first character of the substitution key define the type of value:

• %value: The item is passed through theme('placeholder') (and thus also check_plain()d)
• @value: The item is passed through check_plain() (useful when you don't want to draw attention with excessive emphasizing).
• !value: The item is inserted as is (useful for non-HTML contexts and for inserting whole chunks of HTML into HTML).

Note that contrary to Goba's solution, I chose to change all t() calls around by changing the meaning of "%value". Why? Because too many contributed modules (and some parts of core) still don't use placeholders properly. Using this convention, we can fix a bunch of possible XSS exploit vectors at once. Of course, those modules that already work will need small changes, but those authors should already be familiar with the needed steps. They can remove any theme('placeholder') or check_plain() calls of your own, or use the !-prefix.

I also picked ! as the prefix for as-is insertion because it denotes "warning" and "danger": you're not doing any escaping here.

Examples:

// Here we choose not to emphasize the type (@), but only the title and revision (%)
watchdog('content',
  t('@type: reverted %title revision %revision.',
    array(
      '@type' => t($node->type),
      '%title' => $node->title,
      '%revision' => $revision
    )
  )
);


// Here we are assembling two pieces of existing HTML. No escaping, but safe in this case.
t('!date by !username',
  array(
    '%date' => l(format_date($revision->timestamp, 'small'), "node/$node->nid"),
    '%username' => theme('username', $revision)
  )
);

The @ prefix is very useful for st() / install.php in fact where we don't want to emphasize the name of the install profile (e.g. Drupal or Civicspace).

The only downside of this scheme is that it affects tons of translatable strings.

That last example should of course be:

// Here we are assembling two pieces of existing HTML. No escaping, but safe in this case.
t('!date by !username',
  array(
    '!date' => l(format_date($revision->timestamp, 'small'), "node/$node->nid"),
    '!username' => theme('username', $revision)
  )
);

seems like a good idea to me. this will cause loads of modules to review/change their escaping, but i guess that could be considered a good thing.

Are there any precedents from other applications, programming languages, or environments, that we should be considering when making our choice of placeholder prefixes? Although I think that Steven's logic behind the choices is sound, I'd still like to see some evidence that research has done into existing use of prefixes in the wider world, before we go ahead and commit a patch that will potentially break with already-established conventions.

One precedent that comes to my mind: the '@' symbol is the 'shut-up operator' (a.k.a. the 'error control operator') in PHP. I think that our proposed use of the '@' symbol for placeholders fits reasonably well with the way that PHP uses it (i.e. overriding the default 'safe behaviour'). Can't think of a precedent for the '!' character - except that some developers might confuse it with the PHP/C/Java/etc '!' (negation, or 'bang') operator. But the argument that '!' indicates 'warning' sounds like good enough justification to me.

+1, except:

- No longer applies.
- There is line-wrapping (search for 'gator/opml').

Rerolled.

Updated with HEAD (and fixed wrapping...).

Proposed update instructions:

The t() function was changed to be able to transparently escape and format its arguments to be safe for output. The idea is that where you previously called check_plain() or theme('placeholder') on an argument before passing it to t(), this is now done for you, based on the first character of the substitution key:

// Before:
print t('%type: %title was posted', array('%type' => check_plain($node->type),  '%title' => theme('placeholder', $node->title)));
print t('Submitted on %date', array('%date' => theme('username', $node)));

// After:
print t('@type: %title was posted', array('@type' => $node->type,  '%title' => $node->title));
print t('Submitted on !date', array('!date' => theme('username', $node)));

Note that in the first print statement, we removed the calls to check_plain() and theme('placeholder') as they are now applied automatically to arguments whose key starts with respectively '@' and '%'. In the second case, there was no escaping before, so we use the '!' prefix to insert the argument as is.

Using the above code as a template for upgrading your module only works if your module used check_plain() and theme('placeholder') when appropriate. It is important to use them in output to avoid XSS security problems. If you are not sure when to use them, it is better to try '%' or '@' first, and only switch to '!' when the first is causing problems. It is advised to read the book page about dealing with text in Drupal in a secure way as it contains coding guidelines and examples of good and bad code.

And obviously, you could just be lazy and replace all '%placeholders' with '!placeholders' to get the same behaviour as before. But that doesn't help you make your code more readable.

1. Before this can be committed, I'd like to understand the performance implications of this change. After all, t() is called a lot.

2. Can someone summarize the advantages of this patch? (Or the disadvantages of the current approach.) At the end of the day, st() looks like a small function and the code duplication might not that bad as the additional performance cost?

(All in all I'm OK with this patch. I just want us to benchmark it properly, and weigh the advantages and disadvantages. Rushing his sounds like a bad idea.)

I agree that the benefits are big, and I can see this patch go in if there are no significant disadvantages. Unfortunately, I won't be able to do performance testing myself this weekend. Maybe early next week.

Advantages:

• t() and st() calls become much more readable and shorter by internalizing check_plain() and theme('placeholder') calls on the arguments.
• It is harder to forget check_plain() or theme('placeholder') when using t() and st() (xss!)
• t() and st() now do exactly the same to their arguments, which means they can be used interchangably like this:
  

  $t = $is_install ? 'st' : 't';
  ...
  print $t('My string %foobar', ....);
  

  This is needed in code that runs both at installation time as well as run-time (think install-time/run-time requirements checking patch). The alternative would be for every string to make an if else or ? : construct. That's not handy.

Re performance: I don't have a good environment to benchmark at the moment.

I don't have a good environment to benchmark at the moment.

We don't need a super-duper setup for this. The following might give us some gut feeling:

 -print time-
 for (int $i = 0; $i < 100000; $i++) {
   $foo = t('some stuff here');
   $foo = t('some stuff here');
   $foo = t('some stuff here', arg1);
   $foo = t('some stuff here', arg2, arg 3);
 }
 -print time-

 $arg1 = 'xxxxx xx <b>xxx</b> xxxxxxxx';
 for ($i = 0; $i < 100000; $i++) {
   $foo = t('Filter by message type');      
   $foo = t('Filter by message type %arg', array('%arg' => $arg1));   
   $foo = t('Filter by message type @arg', array('%arg' => $arg1));
   $foo = t('Filter by message type !arg', array('%arg' => $arg1));
 }

Prepatch: 5923.72 ms
Postpatch: 22853.34 ms

Pre--------Post
1085.92 -> 1123.57 t(string)
1644.35 -> 7137.03 t(string %arg)
1611.61 -> 3170.20 t(string @arg)            
1658.33 -> 2481.58 t(string !arg)
1806.89 -> 8952.25 t(string %arg @arg !arg)

Ignore the above. Will redo with proper prepatch checks.

Now done with proper prepatch checks:

Prepatch: 11363.03 ms
Postpatch: 22853.34 ms

Pre --- Post
1133 -> 1123 t(string)
6443 -> 7137 t(string %arg) theme('placeholder')
2498 -> 3170 t(string @arg) check_plain()           
1638 -> 2481 t(string !arg)
8470 -> 8952 t(string %arg @arg !arg)

Heine: Summing the values you pasted in your last followup, gives me the following totals:

before: 20182ms
after: 22863ms

This is quite a different picture. Also, looking at the benchmark from #15, where the xss checks were absent (and the total time was only 5900ms), shows that most of the work is spent in actually performing the xss checks, not in the surrounding code. The performance impact of the changes to t() are minimal enough IMO.

Steven: How embarrasing; that's indeed an incorrect before value.

I've some averages & spreads from 10x 10.000 x iteration

Prepatch              Postpatch				
  110.84 +/-   4.23     110.29 +/-   8.26	(~1)   string
  656.99 +/- 114.15     740.11 +/- 188.25	(1.13)	string placeholder
  248.99 +/-  14.30     322.24 +/-  37.87	(1.29)	string check_plain
  161.02 +/-   8.69     258.04 +/-  31.85	(1.60)	string var (no check)
  734.50 +/-  26.54     991.84 +/-  35.15	(1.35)	string pcn 
1,320.80 +/-  51.92   2,175.18 +/-  70.60	(1.65)	string pcn pcn
1,913.15 +/-  21.84   3,380.97 +/-  56.38	(1.77)	string pcn pcn pcn

time in ms
pcn = placeholder, check_plain, no check

Mostly small differences as seen by

  161.02 +/-   8.69     258.04 +/-  31.85	(1.60)	string var (no check)

+1 for this patch

Wanted to benchmark this patch but it no longer applies. :/

I'll wait for Dries to do the promised benchmarks or until I hear otherwise.

Updating with latest HEAD.

Did you upload the proper patch? I was going to benchmark it, but the last version doesn't apply at all. Got 20+ failures or so ...

Not sure what happened there. I think my CVS tree might have been a bit messed up.

Trying again... I verified that it applies cleanly to a fresh CVS checkout.

Properly wrapping doxygen comments...

Dries' benchmarks showed a slowdown of less than 1% on the front page. Committed to HEAD.

This patch seems to have hosed things. with a fresh HEAD, I see weird errors on hitting install.php:
Warning: array_walk() [function.array-walk]: Unable to call <em>Database name() - function does not exist in /home/bonsai/public_html/test/includes/common.inc on line 601

form element titles show the check_plain'd output of their HTML rather than the properly rendered text, and so on. I'm not even able to load the registration form to create the first user.

When I reverse this patch against HEAD, everything starts working again.

also, the bug italicized this issue!

Better..?

Yes, better, thanks webchick :)

Having dug around, part of the problem appears to be that the case for handling "!" prefixes was completely removed from the patch when the last version of it was rolled. Is this, er, intentional?

No, I'm betting it wasn't.

+1.. confirmed that HEAD is totally broken without this patch (install.php is just a blank screen for me), and applying it fixes stuff again.

Committed to HEAD, thanks.