Custom line item types created with this module do not allow their name to be translated (as opposed to other line item tipes).

The API example in commerce_line_item clearly wraps the name in t():

function hook_commerce_line_item_type_info() {
  $line_item_types = array();

  $line_item_types['product'] = array(
    'type' => 'product',
    'name' => t('Product'),
    'description' => t('References a product and displays it with the SKU as the label.'),
    'product' => TRUE,
    'add_form_submit_value' => t('Add product'),
    'base' => 'commerce_product_line_item',
  );

  return $line_item_types;
}

I think an exception to the "no user defined strings in t()" rule is in order here.
Patch will follow

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rv0’s picture

Status: Active » Needs review
FileSize
660 bytes
nvahalik’s picture

Issue tags: +commerce-sprint
rv0’s picture

Status: Needs review » Needs work

Meanwhile, I've seen other modules use the i18n approach instead of a plain t() so ignore my patch.

smccabe’s picture

Core D7 does this same thing, content types created through code use t(), see the book module for example. But when a content type is created through the create content type form, it just pulls the form value in directly.

The solution for core is to use the i18n module which alters the form and adds a tt() wrapper around the name which is i18n's configurable string version of t(). I attached a patch which checks if i18nstrings is available and if so, uses the tt() function.

If required I can also add in a bit to the install file to go over previously created line item types and do them as well, I wasn't sure if that made sense for the module at this stage.

smccabe’s picture

Status: Needs work » Needs review
mglaman’s picture

Status: Needs review » Needs work
+++ b/includes/commerce_custom_product.admin.inc
@@ -94,6 +94,10 @@ function commerce_custom_product_line_item_type_form($form, &$form_state, $line_
+  if(module_exists('i18nstrings')){
+    $line_item_type['name'] = tt("lineitemtype:" . $line_item_type['type'] . ":name", $line_item_type['name'], language_default('language'), TRUE);
+  }
+

There is commerce_i18n_string, which seems to do same concept, but uses existing API. Also I'm not sure if this solves the overall issue. When you export these line items in Features, the code does export them as translatables. Items wrapped in t() are translatable because they are in code. These patches will save a translated version only, instead of exposing them as possible values to translate.

I also can't find references to tt.

smccabe’s picture

Status: Needs work » Needs review

Having it output it features isn't really relevant here, the problem is that this is a string that can be added through the UI and never be in code. If you create your line items in code this is never an issue because t() would work fine, but you can't use t() with a dynamic field.

For the patch I referenced how the i18n module does this for node entities and did the same thing for line item entities. If you look at commerce_i18n_string you'll see that it does the same module check.

tt() and i18n_string() are/were analogous and the node code used tt() so I used the same. I actually don't see the i18n_string() function existing in the i18n_string module any more so maybe this needs to be fixed in commerce core? Some research indicates it was tt, then they changed to i18n_string, then after some backwards compatibility uproar they changed back, although I found the info somewhat confusing.

Thanks for the review!

smccabe’s picture

Ahh actually I think I understand the tt vs i18n_string thing, I was on the default clone version from git, which isn't the same as the current release version, which uses i18n_string. The i18n module doesn't seem to follow branching the same as most modules do.

Anyways, I will change and use commerce_i18n_string, patch attached.

mglaman’s picture

Nice! That looks great. I'm still new to the multilingual bit, I thought it needed to exist somewhere in code for it to be picked up. If rv0 can confirm I say we RTBC this!

rv0’s picture

I'm not maintaining any commerce sites that use this at the moment. Have you tested it @mglaman?

TimRutherford’s picture

Status: Needs review » Needs work

Couldn't get the patch to work on Drupal 7.41 with Commerce Kickstart 7.x-2.31. Could translate other line item types just fine, but not the custom ones. After adding the new line item type, the string is found in the Translate interface but giving it a translation does nothing. Think this will need to be looked into a bit more.

TimRutherford’s picture

Status: Needs work » Needs review
FileSize
1.25 KB

Here's a new patch that fixes the issue.

There were some major issues with using i18n_string(). It creates a separate translatable string in the translation interface (because the line item creation makes one by default), so then the user would have to add the translation twice. You'd also need to alter the 'theme_line_item_type_admin_overview' theme and call i18n_string() on the line item name to get it to display.

Instead of doing all that I figured it would be best to use the default translatable string and call locale() to translate it. So I used hook_theme_registry_alter() to redirect the 'theme_line_item_type_admin_overview' callback to my own custom function; its the same as the original but with a locale() call around the line item name.

mglaman’s picture

Status: Needs review » Needs work
  1. +++ b/commerce_custom_product.module
    @@ -191,3 +191,29 @@ function commerce_custom_product_line_item_type_delete($type) {
    + * Theme callback for line_item_type_admin_overview, overridden from
    + * commerce_line_item_ui.types.inc.
    

    Can cut out the "overridden from.." to keep it on one line.

  2. +++ b/commerce_custom_product.module
    @@ -191,3 +191,29 @@ function commerce_custom_product_line_item_type_delete($type) {
    +  $output = check_plain(locale($line_item_type['name']));
    

    Replace locale with t().

TimRutherford’s picture

Status: Needs work » Needs review
FileSize
1.19 KB

Here's the updated patch based on comment #13.

I was wondering why t() is applicable here, from the documentation I read (https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/t/7), t() is only for translating static strings, not user generated input; but it is working in this case. Care to elaborate?

mglaman’s picture

Because locale only works if Locale module is enabled :)

liamanderson’s picture

Status: Needs review » Reviewed & tested by the community

Patch #14 tested successfully on Kickstart.