the function t() defined on line 599 of includes/common.inc wants a simple test for an empty string argument, i.e. insert

if (empty($string)) return '';

on line 3.

Without this, there can be a lot of wasted sql calls. As an example, the empty argument gets passed to the t function in the taxonomy_dhtml module on line 32 whenever a term doesn't have a description.

This proposal comes from the thread on the devel list beginning here:
http://lists.drupal.org/pipermail/development/2006-July/017504.html

CommentFileSizeAuthor
#19 locale.patch_7.txt496 bytesadixon
#17 locale.patch_6.txt432 bytesadixon
#14 locale.safer107 bytesadixon

Comments

killes@www.drop.org’s picture

killes@www.drop.org commented
Status: Active » Closed (won't fix)

that is a bug in the calling module. We don't translate user input (which could potentially be empty), so t() around a term description should be removed.

adixon’s picture

adixon commented

well, that's what I thought, but in my example it's not user input, and it seems to me a valid use (ie. translation of terms and descriptions). If it's just that one module that is misbehaving, yes it could be fixed relatively easily, but it seems to me that as the code base grows it would be wise to add the simple test.

adixon’s picture

adixon commented
Project: Drupal core » Taxonomy dhtml
Version: x.y.z » master
Component: base system » Code
Status: Closed (won't fix) » Active

okay, converting to a bug for taxonomy_dhtml ...

killes@www.drop.org’s picture

killes@www.drop.org commented

Well, "user input" really means "everything not hardcoded into the module".

meba’s picture

meba commented

This way, we can solve all potential problems for all modules which suffers this and it's only one line patch doing sensible stuff, not just a hack. +1 for submitting to locale.module

moshe weitzman’s picture

moshe weitzman commented
Title: simple speed up » performance: exit t() if $string is empty
Project: Taxonomy dhtml » Drupal core
Version: master » x.y.z
Component: Code » locale.module

fixing this once and for all in locale module is a much better idea.

i don't really agree that term names/descriptions cannot be sent through t() but thats not related to the proposed fix.

killes@www.drop.org’s picture

killes@www.drop.org commented
Status: Active » Closed (won't fix)

we never fix contrib cruft in core.

rkerr’s picture

rkerr commented

What's so wrong about doing proper error checking of inputs?

adixon’s picture

adixon commented

Thanks to whoever improved the title of this post.

Gerhard: you can think of this as fixing contrib cruft, but I don't - I think it's really about the t() function, whether it should be restricted to fixed module-defined strings or whether it can be applied to dynamic strings (what you're calling "user-defined"). Certainly one argument for restricting to fixed module-defined strings is that then the string-management functions actually work (e.g., you can generate proper .po templates). You could also make an "efficiency" argument, but I don't think that's really the key issue.

One key place that this arises in my experience is taxonomy (not just taxonomy_dhtml, which I haven't actually used), and taxonomy translation is difficult because taxonomy in drupal is really a great piece of code that can be and is used for many different things (i.e., it's a handy mechanism for many problems rather than a well-defined feature that solves one problem). So taxonomy terms can be sort of content and sort of interface, it depends on how it's used. For example, when you provide a fixed set of terms that is used consistently across a website, it becomes part of your interface and providing translations makes sense. But free-tagging is unpredicatable and so it's definitely content.

So I think it'd be helpful to think more about how the t() function is being used with taxonomy and whether there's a better way. I've worked with i18n and taxonomy for a while but I still get confused ...

My only proposal for some kind of immediate resolution is to make a cover function translate() that just tests for the empty string and then applies t otherwise. Then the t() function could be used as intended, and the translate() function could be used for cases where you really do think you want to translate 'dynamic strings' in your code. In other words, translate() is a 'try and translate if you can' kind of function for cases where you'd like to use any available translation but don't necessarily know if you've got one.

Alternatively, if there's a better way to deal with 'taxonomy terms as interface', I'd love to hear about that, and also of other examples where t() is currently being used with dynamic strings.

killes@www.drop.org’s picture

killes@www.drop.org commented

I am just telling you what the current policy is. I am not saying it can't be changed.

Since Drupal core only really supports one language (changing the interface language whle the content stays the same is kinda lame), translations of terms etc aren't currently a core issue. If i do a site in German, all my terms are going to be in German too.

adixon’s picture

adixon commented
Status: Closed (won't fix) » Active

Any more thoughts on this issue? Leaving as is doesn't seem like a good idea. Three solutions are:

1. add the patch and consider it simple inputs checking.

2. add a new cover function taxonomy_translate() to the taxonomy module as just a cover function for t() with a test for the empty string. Assumes the problem is limited to translating taxonomy terms (the only place currently identified).

3. add the new cover function as part of i18n, and let i18n be responsible for translating terms (would require updating taxonomy modules).

From a short term view, 1. is the simplest to implement and most robust, but contradicts a core assumption. 2. is easier but may not solve the problem under all circumstances. I think 3. is the most difficult (maybe not from a code point of view, but it affects the most number of modules/maintainers...), though logically correct.

I'm tending to both 3. and 1., myself...

drewish’s picture

drewish commented

I'd say it seems very logical to exit if there's no input.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

This is my comment from the mailing list thread:

Where is the line if you support t()-ing user specified content? Long descriptions? Site missions? Node bodies?

The problem with extending t() this way is that we should have different interfaces and APIs directly for content translation in Drupal. If you modify your site mission (say fix up some grammar), you have this workflow to change other language versions (or pair them again with your new site mission, if they don't need fixing):

- modify site mission on settings interface
- go to the locale admin page
- click on search local task
- search for the old mission text (and keep this tab open in your browser)
- search for the new mission text in another tab, and copy over all translations for all languages one by one manually, modify them as needed
- save your new translation by submitting
- remove your old mission translations manually

... The above reasoning presented by the start of this thread is about taxonomy term descriptions. The same issue as with site missions, just that you have a lot more discriptions and lot more possibility to change them (ie. more people have permission to do so, not just the super admins as with site missions).

If the interface is not laying itself for the task, maybe we could think that the db storage is right for this kind of solution. Well, as far as I see, it is not. Variables like the mission statement are preloaded to Drupal for quick access. You don't know which translations are variables in the database, so you need to first fetch 'original' variables, then t() them all one-by-one (nice DB hammering), then store them in memory. Same for taxonomy descriptions.

It is also not desirable to translate these texts in PO files, and especially not mix interface strings with site content.

IMHO there are a lot of conceptual and performance problems with allowing t() usage with arbitrary user defined content. The fact that there is no solution in core for this task and t() seems to be an easy fit (at first sight without thinking about it deeply) does not mean we should do this. Unfortunately there was very little interest in our i18n2core effort, which is headed by Jose, but it does try to offer "better" (depends on how you evaluate them) solutions for these problems. Look into those, discuss, improve, suggest better ways.

IMHO we should focus on that instead of pushing a user interface and database nightmare on our users throat. Issue is here, and will obviously have merit once HEAD is open for development:

http://drupal.org/node/77866

Jose also has a session on i18n in Brussels. Go discuss! Unfortunately I will not be able to be there.

That said, calling t() with an empty argument is a malfunction in the calling module. t() is supposed to be used for literal string translation. It is sometimes called with dynamic arguments (eg. module defined node type names), but these are handled by the PO generator. Anyway, putting in a simple empty check in t() breaks down to a performance question IMHO (given that we follow the spirit that a function should check its argument for their proper format). Does it badly affect performance?

adixon’s picture

adixon commented
StatusFileSize
new locale.safer107 bytes

Okay, as per these comments and discussion on the devel list, here's a simple patch to locale, which is designed to rescue drupal's performance in case of improper (or unexpected) use of the t() function.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Active » Reviewed & tested by the community

I am OK with this patch, and Gerhard was OK too on the mailing list. Maybe this should also get added to Drupal 4.7.x, not only to Drupal 5.0.0/HEAD.

drumm’s picture

drumm
he/him
Location NY, US
commented
Status: Reviewed & tested by the community » Needs work

Needs to be in the correct diff format.
http://drupal.org/diffandpatch

adixon’s picture

adixon commented
Status: Needs work » Needs review
StatusFileSize
new locale.patch_6.txt432 bytes

Better?

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs review » Needs work 
// for performance, in case of improper use

should be:

// For performance, in case of improper use.

(capitalized, period at the end)

However, I don't know that that comment is sufficient, in any case. What is "improper" use? Trying to t() a dynamic value? If so, can we say that?

adixon’s picture

adixon commented
StatusFileSize
new locale.patch_7.txt496 bytes

Fixed capitalization, added explanatory comment about improper use.

chx’s picture

chx commented
Status: Needs work » Closed (won't fix)

I read the whole issue and I can't fathom how we arrived to the point where this is even considered to be accepted. t() is usually not called on variables, except a very few cases and those can not be empy. This fix adds cruft to core no matter how little. There can be no valid use case.

adixon’s picture

adixon commented
Status: Closed (won't fix) » Active

There seems to be a determined collective denial going on here amongst some core developers. The t() function is called with a variable argument even in the core modules. It may be that in all those instances, the developer is sure that the variable has been assigned a fixed module-defined string, but I find it hard to believe that anyone can guarantee that (look at line 95 of the current watchdog module as an example).

I'm going to give up now and just keep this patch as one of those things I have to do with all my sites. Pride seems to have exceeded good sense on this issue.

I have changed the status back to active in case anyone else wishes to take up the torch ....

gerhard killesreiter’s picture

gerhard killesreiter commented
Status: Active » Closed (won't fix)

foreach (_watchdog_get_message_types() as $type) {
$names[$type] = t('!type messages', array('!type' => t($type)));
}

The string contained in $type isn't a variable. It is a hardcoded string passed to watchdog as its first argument. This approach is neccessary to enable contrib modules to add their own classes of watchdog messages.

adixon’s picture

adixon commented

Sorry, can't resist ... as you say, the string '$type' is whatever the calling module puts into it's first argument, which, exactly as you say, is because you want the flexibility to allow modules to define their own types. Could there be a case where a module is actually dynamically generating these strings? Maybe ... although a that's separate debate. I'd say there's another debate about whether t() should actually be applied to those types (shouldn't they be "internal"?)

My only point here is that in our real world, the drupal code is complex enough that even good modules (or even core drupal code) may end up providing an empty string as its first argument to t(), so there is a valid case for inputs checking on this function.

Try:
grep ' t(\$' */*.module
in the cvs contributions repository ...

moshe weitzman’s picture

moshe weitzman commented

"There seems to be a determined collective denial going on here amongst some core developers. "

oh man, thats a perfect quote. well said. why one would resist so hard a couple lines of safety code i do not know. this project tends to attract purists which is good in many ways but tiresome in others.

killes@www.drop.org’s picture

killes@www.drop.org commented

The project probably attracts purists because it was started by one. :p

adixon: I agree that the handling of the watchdog messages isn't really optimal. We should probably use some defines:

define('MY_MODULE_USER_ERROR', t('my module user error'));

meba’s picture

meba commented

This is almost the same case as validating inputs for SQL. Why should we do that? It's a programmers fault if inputs are not sanitized.
</irony>

I don't get this. Really. It my first task in every project to test if variable inserted to SQL is not empty. It is not related with code purity i think...
jimmy@arkeia:~/cvs/contributions/modules$ grep -R ' t(\$' * | wc -l
363
jimmy@arkeia:~/cvs/contributions/modules$

363 occurrences of t($variable) in contrib, not counting these without a space before function. If we save 1 SQL SELECT for every page view, it is good. </myopinion>

Steven’s picture

Steven commented

oh man, thats a perfect quote. well said. why one would resist so hard
a couple lines of safety code i do not know. this project tends to
attract purists which is good in many ways but tiresome in others.

Because t() is one of the most called functions in core. From my t() patch, I remember also that the vast majority of t() calls have a fixed string and do not use placeholder arguments. Just look around on this very page.

Adding code to check whether $string is empty will slow down all of these calls. On the other hand, in the rare case that a module does (incorrectly, in my opinion) pass an empty string to t(), the extra SQL call is an acceptable side effect.