Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#14 | commerce_custom_product-translate_names-2343623-14.patch | 1.19 KB | TimRutherford |
Comments
Comment #1
rv0 CreditAttribution: rv0 commentedComment #2
nvahalik CreditAttribution: nvahalik at Centarro commentedComment #3
rv0 CreditAttribution: rv0 commentedMeanwhile, I've seen other modules use the i18n approach instead of a plain t() so ignore my patch.
Comment #4
smccabe CreditAttribution: smccabe as a volunteer and at Acro Commerce commentedCore 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.
Comment #5
smccabe CreditAttribution: smccabe as a volunteer and at Acro Commerce commentedComment #6
mglamanThere 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.
Comment #7
smccabe CreditAttribution: smccabe as a volunteer and at Acro Commerce commentedHaving 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!
Comment #8
smccabe CreditAttribution: smccabe as a volunteer and at Acro Commerce commentedAhh 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.
Comment #9
mglamanNice! 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!
Comment #10
rv0 CreditAttribution: rv0 commentedI'm not maintaining any commerce sites that use this at the moment. Have you tested it @mglaman?
Comment #11
TimRutherford CreditAttribution: TimRutherford commentedCouldn'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.
Comment #12
TimRutherford CreditAttribution: TimRutherford commentedHere'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.
Comment #13
mglamanCan cut out the "overridden from.." to keep it on one line.
Replace locale with t().
Comment #14
TimRutherford CreditAttribution: TimRutherford commentedHere'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?
Comment #15
mglamanBecause
locale
only works if Locale module is enabled :)Comment #16
liamanderson CreditAttribution: liamanderson at Acro Commerce commentedPatch #14 tested successfully on Kickstart.