Localization of exports

I am a little confused on what we are talking about here. This is what I understand from talking with some folks on IRC, but I do not have extensive experience with multilingual sites, and I am still kind of confused as to what is the best solution, even after about an hour of talking and searching.

* Any names/descriptions/etc that we provide in the module definitely need to be run through t().
* If we are providing an exported version, strings should be provided with the t() wrapper if they will live in code, but if they will put back in the database, this is not a good idea. But I don't think we can determine what will be done with the export. This is fuzzy at best.
* For user-defined strings, we should not be using t() for user-defined values, there are two cases to handle this.
** Values that we store with variable_set() can be automatically translated through the i18n module. This is done by going to the same page but in the different language. Unfortunately this only works for variables in the Drupal system which we don't really have many.
** What I understand as the preferred way is to implement the tt() function that is provided by the i18n module. This requires a wrapper function to check for the function (otherwise we have the module as a requirement). Example of wrapper for notifications module. Issue and example provided by privatemsg module.

@tmcw, maybe you have already asked him, but you work with @josereyero and I think he may be the best person to ask this all about.

Hey @tmcw, I don't doubt this is right, but I am still a little confused on why you are doing it this way? Using a variable in t() is almost always a bad idea. It almost makes sense if we were just running it through for the coded layers, but this will get run for all user supplied layers. I don't see what was wrong with the way it was done before. Maybe I am just missing something.

To reiterate what I was saying above. Any strings we have in code need to be in t(), and for user-provided strings, we need to implement the tt() function that is in i18n, but user-defined strings cannot go through t().

I don't think this is fixed.

Your commit runs all titles and descriptions through t(), including user-supplied strings. Running user-supplied strings through t() is a bad idea, as Gabor, pretty much the expert of Drupal translation suggests in this post.

When I refer to how it was done before, I mean that all our coded layers were run through t() which is the right thing to do, while the user-supplied string did not implement translation.

What we run into is that user-supplied strings are not translated. As I mentioned above and have done a fair amount of research on, there is no built-in way for Drupal to handle the translation of these strings. The i18n module can help. It could automatically handle translation if these were values that were in variables, but since we are storing them ourselves, we have to use a method like tt(). I am curious what your reservations are.

The final problem of translation is handling exports (the actual focus of this issue), which there is no real precedence that I have been able to find. (It would be cool to know how Features does this.) But running user-supplied values through t() is not the answer. In an ideal world, we would be able to know if the export was being put into code or not. If it was, we could provide the t() function, otherwise if it was going to live directly in the DB, then our implementation of tt() would handle translation.

I really think this is wrong and should be reverted. I was thinking about doing a blog post about all this and asking the open question "Whats the best way to handle localization of exports" which hopefully I can get done soon. But, I'll also ask around DrupalCon next week.

I look forward to your more in-depth response.

Just a few things to note. That article is about 4 years old, and its main point is about not using t() for user-defined strings which still applies, but it was before the i18n module had a decent solution.

The tt() solution does not have to introduce a dependency. We can easily create a wrapper function for translation that only does translation if i18n is available. This is how other modules currently do it.

The current state of the code puts user-defined layers into the translation database. This is bad because someone could enter their layer in Spanish and it would totally screw up the flow of translation which is based off of English being the index. Also, if someone edits a user-defined layer, the user ends up with both strings in the translation database. Also, the way you have written this, we have to maintain strings twice in code.

I think its important to use the built-in core translation functions where appropriate, then utilize the i18n module for user-defined strings as there is plenty of precedence in other modules for this. As far as how to handle exports, I think we need to think about this more and make sure we find a solution that does not mess with the current way of doing things. Again, looking at other established modules is a positive step.

Tom asked for my input here as this is certainly one of the murkier areas of best-practice in Drupal.

Some points to consider

• It is true that using t() on "user-defined" strings has been frowned upon. However, this has traditionally meant strings like site_name or other loose, easily changed configuration. In the case of exportable objects/configuration, which often amount to site building or even replacements for traditional Drupal "coding" (think of how a few exported Views can replace whole hook_menu entries and page callbacks) using t() against strings in these components has become a standard solution for localization.

• Defining the localization function dynamically, like

  $t = function_exists('tt') ? 'tt' : 't';
  

  Would be nice and downgrade gracefully. Unfortunately, one of the main reasons to use t() in exports is to allow the potx extractor and other programmatic translation tools to detect the strings in code. Using a wrapper/proxy function is probably not a good idea in this light.

• IMHO, the best module to look at at this point in this regard is the work happening on Views 3.x (http://drupal.org/node/357529). Views has several important concepts with regards to the interaction between exportables, plugin classes, caching and localization.

@yhahn thanks for the input.

I think Views is a good solid module to base our decision off of. We should probably also look at the changes getting into D7 to see what the best path is as well. Hopefully I can take a look at this soon.

Alright, after reading that very long thread, I have much better insight, and still don't feel like we are doing this the right way. Note that t() is bad because once a string as been put through t() it cannot be edited or deleted from translation table/interface. "While it "works", the Core [translation] plugin is not recommended, as it doesn't support updates to existing strings."

To some up what is the best way, this is the help comment in the Views patch:

+   *  - 'translatable' => TRUE/FALSE (use a localization plugin on display and
+   *      wrap in t() on export if true),

So, here are the key points.

1. Create a system to mark fields as translatable. This is non-trivial, though the majority is fairly straightforward, there are big edge cases like behaviors input fields.
2. On display, user data marked as translatable should be run through the translation interface. This needs to use tt() if there and t() if not. We also need to be clear to the user that they should use the i18n module if they want to do translation correctly. And also, on this front, when displaying things that live in both code and database, we need to do a simple check to not translate the data (as it will already be run through t()).
3. For exports, we wrap the data that is marked as translateable in t(). This will make all exports that live in code fully translatable, then on the import side, we simply need to strip out the t() part, and part 2) will handle translating the rest.

I have nothing against t(). But its main use is for static strings in code. It can be used outside of that but we run into problems, the main one being that given the core translation system, a user cannot edit or delete a string once it has been put in the translation system. This is a problem if someone updates the title of a preset or layer after translating it, as well as simply spamming the translation table. And in my suggestion in #17, I state that we should default to t() to translate user-defined strings, though like in Views and in other modules notify the user that this is not the best way.

My suggestion is completely based from what is being done in Views, except it is simpler, as Views is using a plugin system to be able to replace the translation mechanicsm. There is a fair amount of precedence for using i18n's tt() for user-defined strings (and t() if that is not available). I definitely don't want to "pioneer the art of translating exports", but I have spent many, many hours looking into this and I feel this is the best way forward until a system is in place in core or in i18n that supports exports better (which there isn't). Honestly I feel I have spent much more time ensuring that this solution is more sustainable and in sync with other work that you have.

What is currently in the module is hackish and incomplete, IMO. It uses the method of an unused function and duplicate strings to try to manage translating layers, when there are many other things that need to be translated as well.

I agree that putting effort into a system that denotes something as translatable would be better focused on something higher up. But there is nothing that is currently there. This is all fairly uncharted territory. I think we could get around this by just making a limited number of things translatable and hard code it in there.

On module adoption, no. The idea that I have only had is that we build something that utilizes the largest efforts in the community, hence tt(), and make sure that whatever system we have can be easily switched out or removed until something better comes along.

And, yes, I would rather focus on other things, but if we are going to make sure this module is fully translatable then we need to do it right and sustainable, but you continue to argue for an incomplete approached.

So, here is an initial patch. Not really ready for production, but you can start to see the direction that I would like to go in.

* It only addresses layers, though everything will need to be addressed.
* Currently, it is marking a field as translatable in the schema definition. This would be perfect except that we have serialized data in fields, and we want to be able to translate values defined in forms in behaviors. Views designates things as translatable in its own way, but it has a much more robust architecture for parts (and its not translating everything).
* A openlayers_tt() has been added to be a wrapper around translationg of user defined strings. This is only used on layers that are not in code. Also this defaults to t() if tt() is not present.
* Exports of layers now get the t() function added to the translatable fields. I am not sure how Feature will handle this; something to look into. I did have to clone the ctools export function to be able to do this.
* Imports of layers strip out the t() so that data living the database can be translated as described above.

I think this is a pretty solid mechanism to be able to have this module fully translatable, that is not too heavy. I think more in the future, we would want to bug ctools to be able to support marking something as translatable and adding t() in export.

Bad logic in openlayers_tt(), new patch.

Hey @tmcw, it would be nice to get some review on this before I go for the big patch (forgot to mark as Needs Review). Thanks.

For the record, I don't agree with this approach. I think both are hacks and not great solutions, and it is more a fault of the translation infrastructure. I don't particularly like the approach of translating on render as it removes the usual Drupal standard of using t() in code for translatable strings. It also means that strings are duplicated and increases maintenance. I do understand that my proposed method is a bit more complicated, but by no means anything unwieldy. Nonetheless, I don't want to argue on this issue anymore.

This needs to encompass styles as well.

I think its very important to ensure that documentation is clear on this as it goes against Drupal coding standards. Though it is not so absurd as the menu system, by default, runs things through t() (this makes me wonder does potx extraction take this into account).

And, of course, I agree that any CTools translation implementation should be used if/when available.

Are there any other user-defined strings that need translation?

My point with documentation was not that we should document that this is inferior or anything like that, just that it needs to be more accurately documented as it is not the standard way of creating code in Drupal:
http://drupal.org/cvs?commit=374676

Styles (and presets) still have interface strings and need translation. Yes, they do not show up on maps, but definitely do fall under the category of localization of exports. Here is a patch that address all the rest of things that need to be translated and introduces openlayers_translate() to utilize i18nstrings when available.

It would be nice to have another set of eyes on this. I will commit soon if no one has any objections. It should be pretty straightforward.

• Lots of precedent as discussed above, specifically the i18n handbook page: http://drupal.org/node/789286
• Specifically for Views, it is using a plugin system to allow for translation systems outside t() or i18nstrings(), which I feel is a bit overkill for our module. It would be nice to have, but I think it makes more sense to wait until there is a general solution to translation plugins. Also, with openlayers_translate(), it will make it trivial to drop something else in.
• True, this is mostly less efficient. My thought was that utilizing openlayers_presets(), one could get things quicker, but ctools handles all that anyway. I will update this.
• I am a little confused on the duplication of translation point. The patch removes the use of t() and utilizes openlayers_translate().
• Also, the patch should be translating on openlayers_layer_export_load() as the UI will display these things without fully loading the layer. Will update.

I am not sure how this patch has "lots of problems" as it totally implements everything that has been discussed and agreed upon above in its entirety instead of just a limited part of layers. I don't "want this issue to be dead, since every comment after #6 was just slowly deciding that #6 was the appropriate action", I want it to be done right and complete.

As that handbook points out, the Notifications and Case TRacker modules do it this way. Current Case Tracker imlpementation:

/**
 * Translate user defined string. Wrapper function for tt() if i18nstrings enabled.
 * 
 * The string id for case states will be: case:[realm]#[csid]:name
 * 
 * @param $name
 *   String id without 'casetracker', which will be prepended automatically
 */
function casetracker_tt($name, $string, $langcode = NULL) {
  return function_exists('i18nstrings') ? i18nstrings('casetracker:' . $name, $string, $langcode) : $string;
}

The problem here is that 1) not all exportables are translatable (styles), 2) we are not translating responsibility because we are not using i18nstrings when available, and 3) documentaiton is inaccurate and incomplete. This patch should not affect how anything currently works.

Please read that Views ticket if you have not. 1) nothing has been committed to Views yet and 2) looking at the patch, the first 20 lines describes a plugin system (http://drupal.org/files/issues/views-357529-translatable_2.patch).

All in all, I think that this is kind of absurd, because we're apparently fixing a problem that doesn't exist. At the very best, this patch breaks existing translations of items in OpenLayers and it prefixes them in the t() table, in exchange for a big code increase and a somewhat-dependency on another module. I just don't get it - translations, in production, are working quite well for this module, and the reasoning for keeping this issue-ticket-war going is basically nil. Doing this 'right and complete' or 'non hackish' or whatever, I don't care, just seems absurd - there are no specific problems that this fixes and discussing it to death and changing the ticket status thirty times or whatever just doesn't make sense.

How does this break existing functionality? It just complete that sytem. What is being prefixed? The textgrouping that happens is only for i18nstrings and it is just an identifier so that translations can be managed better. This is not a very big code increase, most of that patch is documentation. And again, this will not change how any sites work currently. And finally, just because there has not been someone else saying anything does not mean that this issue is not resolved. Obviously, nothing has been committed as this discussion just keeps going. I don't see what your problem is with the latest patch (besides the couple things I mentioned in #33), it just completes what has been agreed upon.

tagged as beta blocker

I really don't see what you have against completing this issue, and I don't agree with removing the beta-blocker, but its not worth the argument. Here's a new patch.