Allow localization of built-in menu items

I haven't done any translation, but what about adding a third parameter to t() as a flag that would tell it to just return the original string without any translation? That would allow extractor.php to still find the string and include it as an item to be translated. It seems like it would be useful in this context as well as others.

Two other simple options would be to make an entirely separate function or to overload the second parameter to t() (e.g. if it's NO_TRANSLATE don't translate the string).

I cut some corners. No significant change. Very nicely done.

@bdragon: Could you please submit "extractor.php" improvements to the relevant project: Translation template extractor. Also note that the extractor is now split into a common include file shared with a web interface and a cli file (potx-cli.php), which is the new command line tool. After a cursory look, your changes seem to be easily ported to the current extractor code. http://drupal.org/project/potx

@chx: I checked your latest patch. As you noted on the mailing list, the descriptions still need to be t()-ed at runtime. I also noticed that title arguments are not serialized when stored, although unserialized when used, so there is a small bug in there.

TODO:
- serialize title arguments on storage
- t() description on runtime (noted by chx on the dev list)
- reroll patch against current HEAD (noted by chx on the dev list)

Wait! Isn't it a "PHP injection via the database" vulnerability that this patch blindly executes every callback that comes out of the database? Even not checking for whether that callback exists... Hm... A single weak module on the site to achieve some DB poisoning, and (close to) arbitrary code can be run with defining a desired callback and a parameter list.

Well, if you have a weak module then that can directly execute b0rken code without running through the database...

I mean a db security hole is immediately extended to a PHP level security hole with this module. It is comparable to having the PHP input format kept intact (which is another nice example of possible database security problems taken to a new level). By the way, as long as the PHP input format is there, we are not introducing new possibilities for holes, so maybe this approach can be left alone.

Any takers to finalize the implementation?

subscribing

Since nobody was working on this, I took some time to get the patch to core commit level (hopefully). Updates:

- updated to latest 6.x-dev (alls hunks applied with some offset, many hunks failed, needed to fix them manually)
- watchdog became dblog and syslog, apply the changes to these new modules too (php.module required no changes)
- remove t() calls from descriptions and add runtime t() call to menu.inc

Installed and tested this and it works fine, the values are properly added to the menu table, string for localization appear in the locales_source table. Everything seems to be fine. I would welcome reviews, so we can get this fixed soon.

Note that the required system.install table definition changes were already committed earlier by Dries, in anticipation that this menu localization patch will get in soon. Also note that there is no update path for menus yet, but as chx informed me that is postponed to later, as soon as all menu changes accumulate. So system.install is not changed here.

Bdragon, please file an issue against potx module (http://drupal.org/project/potx) with your changes to the extractor. I would be very glad, if you could update your changes to the actual potx-cli.php and potx.inc codebase.

Applied the patch, tested and it works fine

Just a minor typo in block.module:

+      'title_arguments' => array('!key' => $theme->info['name']),

Should be:

+      'title arguments' => array('!key' => $theme->info['name']),

OK, fixed that small glitch. Seems to be ready(?)

I would not t() the description every time. That's a waste IMO, it's not used anywhere else but the admin* pages, is it?

chx: we only t() the description if it is not empty, be it an admin page or not! Anyway, the node/add/$type menu items also have the descriptions of the node types set as descriptions of the menu items. I don't know of other non-admin parts where a description is used. But how checking for $_GET['q'] and !empty($item->description) would be quicker then simply checking for !empty($item->description) (if the former is what you actually suggest)?

I was thinking on t'ing in theme_admin_block_content . Or maybe write a preprocess for that theme function, and t() there. It's not used anywhere else, why bother?

Well, it could be used to provide tooltips on menu items (depending on your theme), as well as used on the node/add pages as I have examined. Why close that possibility?

Let it be.

Both this issue and the patch have little or no details about the motivation of this patch. I assume this has to do with translating custom menu items? I think some PHP comments are in order to provide some background on the rationale.

When this can be used to translate custom menu items, shouldn't these go into a separate domain?

Oh, yes, missing rationale. The rationale of this patch is to *fix* a localization bug in the menu system. Because all menu items are gathered and stored in the menu table, you need to return menu item titles and descriptions with hook_menu(), which are possible to display in all languages. Therefore t()-ing titles and descriptions in hook_menu() is prohibited, because you get menu items stored in one language, not possible to translate menu items into another language. So we need to store English source strings and translate menu items and descriptions runtime.

Description is simple, since if we have it, we can simply t() it for now. Title is a bit more complex since sometimes we have user defined strings in need of some sanitisation, and so on. So we need a callback stored in the database to invoke when the title needs to be "translated". This patch has nothing to do with translatable user defined menu items yet. In fact it introduces a misuse of t() for user defined menu item titles and descriptions, but because we have no way to translate them to another language in core yet, we are swallowing that up for now.

I just noticed, that we also have descriptions for user defined menu items which are "displayed when hovering over a menu item" according to the help text, so we can have descriptions for menu items anytime, not only on the admin interface.

Or to be shorter, the rationale is very similar to the "later t() with watchdog" patch, where strings needed to be stored in English, and translated later. The menu issue is more complex because we might need a custom callback.

Changed title to be more descriptive of the issue at hand

Thanks for the explanation. Can you document this a bit in the PHP code? It's helpful for people who want to get up-to-speed on the menu system. Two or three sentences of text should be enough to make people understand faster what this is about and why it is done on output. Thanks Goba.

If the menu hook or the menu $item struct is documented in the code, we should probably document these two parameters as well. If not in the code, it should go into the handbook here on drupal.org.

Other than the documentation issue, this patch is RTBC. Feel free to mark this RTBC once you went over the PHPdoc/code comments once more. Thanks.

OK, updated patch:

- updated to latest Drupal 6.x-dev
- added code comments to _drupal_translate() that it also does translation :)
- added code comments to above the title translation code why we do it there

Was unable to find documentation in the source code about the $items arguments.

Updated online documentation:

- updated all subpages of "the new menu system", removing t() from titles and descriptions: http://drupal.org/node/102338
- added new info to the "upgrading" part: http://drupal.org/node/103114
- added extensive docs with core examples, explaining how titles and descriptions are handled: http://drupal.org/node/140311

New patch attached.

Thanks Gabor! Committed to CVS HEAD.

note- I'm think I'll have to have to redo the menu.inc part in the patch to enable the multiple menus.

Can you follow up here: http://drupal.org/node/137767

@bdragon: grabbed your extractor.php improvements, worked on it for a day to get it updated to the current potx-cli.php/potx.inc implementation, and committed your changes to the potx module (although commented out your menu implementation for now, as it is not appropriate for Drupal 5). Seems like you really did doubled the speed of the extractor. Thanks! Please submit issues against the potx project in the future, so we can get use of your improvements more smoothly.

This change was never documented at http://drupal.org/update/modules/6/7

I found this issue via a lot of cvs archeology while trying to figure out what's going wrong with #818136: Some strings in update.module are not translatable. Seems like perhaps it's just an extractor bug (or at least, input that the extractor is going to have a hard time finding), but if any of the localization experts in here care to take a look, that'd be much appreciated. ;)

Thanks,
-Derek

Sorry, I'm an idiot. ;) That's because this change happened in D6 and I was looking in the wrong place. ;) /me facepalms.

That said, if anyone who's familiar with how all this works could take a look at #818136: Some strings in update.module are not translatable that'd be Real Nice(tm)...

Thanks,
-Derek