Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webflo’s picture

webflo’s picture

Assigned: Unassigned » webflo
Status: Active » Needs work
Berdir’s picture

Title: Implement UI for tmgmt_i18n_string » Source Plugin: i18n Strings UI
Berdir’s picture

Component: Code » Core
Status: Needs work » Needs review

Does this still apply? I see that the blocking issue has been commited a while ago, yay!

Berdir’s picture

Status: Needs review » Needs work

Yes, it applies, but it doesn't work:

Fatal error: Call to undefined function i18n_object_by_name()

I guess that function was renamed or something, i18n likes renaming functions :p

Also, integration with the translate tab would be great. You said that you have some views-based code for the overviews? That sounds interesting as well..

Berdir’s picture

Ah, yes it has been renamed to i18n_get_object() in a later patch.

When changing that then it kinda works, it lists my item types and correctly shows translated. Given that with i18n_string, the source is always the default language, I think we don't know the show either "Source" nor the language column for that, no?

webflo’s picture

Berdir’s picture

Status: Needs work » Needs review

Would like to see what the testbot does with this, setting to needs review. The testbot still seems to depend on 7.x-1.5, not sure why exactly, so I guess that i18n_object/i18n_get_object() conditional is still required... probably need to ask rfay for a manual dependency rebuild.

Status: Needs review » Needs work

The last submitted patch, tmgmt_i18n_string-ui-1445682-2.patch, failed testing.

Berdir’s picture

"Call to undefined method i18n_object_wrapper::get_strings()", I guess this depends on some uncommited i18n code?

inventlogic’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, tmgmt_i18n_string-ui-1445682-2.patch, failed testing.

inventlogic’s picture

This is now failing on a TMGMT string test so can the patch submitter re-look at this patch?

miro_dietiker’s picture

We would be happy if you can take a look too. Webflo is currently very busy and the patch was also just a start.

Berdir’s picture

And old, this doesn't include the work that he did in Munich. Where he, I think, changed everything again :)

inventlogic’s picture

I was directed to this patch from Dynamic Properties issue 1700162 which said this patch would help in translating the labels for the properties.

I had a look at the patch and as it stands at the moment it does not pick up any of the Dynamic Properties label strings.

Also am I right in that I think everything that it does pick up is manually translatable some where else?

So this patch is just a strings workflow UI for TMGMT?

Its still failing on Call to undefined method i18n_object_wrapper::strings_update() which makes no sense as the i18n string functions are available.

Anyway not what I am looking for.

Berdir’s picture

You are in the right issue but this patch is not yet ready. If you have a budget, I suggest you contact @webflow to sponsor the work he's doing.

inventlogic’s picture

I figured out I can setup multiple templates in Dynamic Properties one for each language.
The translate option for the Dynamic Field can be used to fill in the language specific template.
Then its just a matter of filling in the language specific data into the template.
So sorry I wont need this patch just yet.

miro_dietiker’s picture

@inventlogic: It's a pity to invest time to create workarounds instead of real solutions. This way, no solutions will improve ever. Good luck with your project.

inventlogic’s picture

@miro_dietiker: true and thanks.

miro_dietiker’s picture

We are referring here to "i18n source" which might have some special cases in addition to the general locale table.
However i guess we should start with a general locale implementation first, supporting all text groups, and then customize the i18n strings text group by leveraging i18n specific features.

miro_dietiker’s picture

Assigned: webflo » blueminds
Priority: Normal » Major

This has been excalated to us and we will need to take care of it to make this source plugin work.
A basic version is enough... We can always improve it later!

Also note that in past, almost all projects that wanted to use TMGMT resulted in the first job to try to submit translation of menu items... It was always very frustrating to tell the clients "no, it's only content for now ... plus other entities ... almost anything, except menus / i18n strings..."

blueminds’s picture

I played with the patch and can say it works (with a few fixes) for menus, vocabularies and such. It does not work for menu links, taxonomy terms, etc... Reason is that i.e. menu links do not have records in the i18n_string but individual items are created for each language. The current code does not handle this. So what I understand that should be done:

- provide functional translation source overview page (now each lists the same content) that also can work with menu items and tax. terms with some search possibilities
- extend logic to be able to receive translation for all i18n sources

I could not find anything else that I think should be part of the basic version.

Berdir’s picture

Menu links and taxonomy terms tepends on the localization setting on the menu/vocabulary.

I think localized is the case that we want to support here, that means that there is a single term/link with language undefined, then you have a i18n_string translation.

Other cases are not supported by us, at least not now because those usually mean that there isn't a 1-1 translation of the menu links but they differ for each language. So we can't really provide a translation anyway.

And yes, we want an overview and an integration with the translate tab, to be able to translate a single menu item without going through the overview.

blueminds’s picture

Status: Needs work » Needs review
FileSize
9.38 KB
16.09 KB

Here is patch making the basics functional. However this is not final. It needs following:

- general clean up and refactor - there is a lot of commented out code, or some unused code, needs comments..
- currently following types are implemented: menu, menu items, taxonomy vocabs, taxonomy terms. We need to decide what other types we want to implement (I guess there are others like blocks, forum, paths...) It would be also nice to provide some pluggable design so that we do not need to "if" each type
- provide tests coverage for menu items, taxonomy terms, possibly others that we will implement

blueminds’s picture

Patch includes following:

- integrated into the translate tab of i18n
- tests coverage
- cleaned code + documentation

Berdir’s picture

Quick review, will test this soon.

+++ b/sources/i18n_string/tmgmt_i18n_string.moduleundefined
@@ -6,6 +6,16 @@
+ * Gets i18n string types which are implemented form tmgmt translation.

@@ -13,44 +23,201 @@ function tmgmt_i18n_string_tmgmt_source_plugin_info() {
+ * Get db type of i18n string type.

Returns instead of Gets/Get

Also, we should probably talk about i18n object types as only i18n objects have a type and strings just have an arbitrary context.

+++ b/sources/i18n_string/tmgmt_i18n_string.moduleundefined
@@ -6,6 +6,16 @@
+function tmgmt_i18n_string_implemented_types() {
+  return array('menu', 'menu_link', 'taxonomy_term', 'taxonomy_vocabulary');

Can we add a @todo here that says Support more/all i18n object tpyes?

+++ b/sources/i18n_string/tmgmt_i18n_string.moduleundefined
@@ -13,44 +23,201 @@ function tmgmt_i18n_string_tmgmt_source_plugin_info() {
+ *   i18n string - must be "fixed" @see tmgmt_i18n_string_fix_type_naming(tmgmt_i18n_string_get_db_type.

"fixed" isn't very clear, should say what exactly you have to do, e.g. pass through function function_name(). @see should IMHO only be used at the end of a docblock, one per line. Function references within a text should just use function_name() as mentioned above.

Also > 80 chars.

+++ b/sources/i18n_string/tmgmt_i18n_string.plugin.incundefined
@@ -49,7 +47,23 @@ class TMGMTI18nStringSourcePluginController extends TMGMTDefaultSourcePluginCont
+    // We just saved the translation, set the sate of the job item to

state.

+++ b/sources/i18n_string/tmgmt_i18n_string.plugin.incundefined
@@ -49,7 +47,23 @@ class TMGMTI18nStringSourcePluginController extends TMGMTDefaultSourcePluginCont
+      return $item['#label'] . ': ' . $item['#text'] . ' (' . $job_item->item_id . ')';

Hm, the #text could be quite long?

Berdir’s picture

FileSize
56.05 KB

ok, started testing this.

Working:
- Overview looks fine, search works and translation status also seems to work
- Only the group types show up that are defined, term and vocabulary for example don't if i18n_vocabulary isn't enabled.
- Creation of jobs on the translate tabs works fine.

Not working/Improvable:
- I'm not able to create jobs on the overview. Nothing happens when I click on Request translation, the page just reloads
- You're using the last data item as the source label. However, you should be using the first as that has a much higher chance to actually be a label or so for that i18n object. See attached screenshot of a job item for the management menu. The fact that the job/item labels are check_plain()'d too often isn't your problem directly nor is the fact that the job label can easily get longer than 128 in which case the form validation fails but you at least wouldn't trigger these core bugs. I think that is more or less consistent with what the translatabe tab does. So as a summary, just "#text (id)", without label, of the first data item.
- There is no source uri defined, I think that is stored in the options somewhere? The result is that it's displayed as a link to the front page. Looking at i18n_string_object_wrapper, there's get_path(), get_translate_path() and get_edit_path(), We should probably use get_path()?
- There's no paging, so if you create 1000 terms, you get a pretty large and slow page. Not sure how hard it is to add paging, if there's a problem we can postpone it.

Ideas, probably for later:
- As mentioned above, we're actually doing i18n object integration which is the part that groups several i18n_strings into a group. There are also i18n_string strings that don't belong to a group. Translatable variables are such an example I think. So I'm wondering if we should provide a separte a separate source that interacts directly with single i18n_strings's that do not belong to a known group.

blueminds’s picture

Comments implemented, please see the patch.

blueminds’s picture

Previous patch has tests disabled.

Moreover I think we need some UI tests here as well...

miro_dietiker’s picture

Please provide more description.
So you have implemented all "working/improvable" above?
Just "ideas, probably for later" remaining?

seasonxue2013’s picture

Where can I get i18n_string?

alanburke’s picture

Status: Needs review » Needs work

admin/config/regional/tmgmt/i18n_string will fail if the language "Portugese, Portugal" is on the system,

PDOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'lt_pt' in 'on clause': 

Something to do with the language code having a hyphen, I think, but I haven't worked it out yet.

blueminds’s picture

Status: Needs work » Needs review
FileSize
5.97 KB
29.63 KB

Fixing the bug + providing UI test coverage.

Status: Needs review » Needs work

The last submitted patch, tmgmt_i18n_string-ui-1445682-7.patch, failed testing.

blueminds’s picture

Status: Needs work » Needs review
FileSize
29.5 KB

looks like test bot does not like the way the job id is determined from the url...

Status: Needs review » Needs work

The last submitted patch, tmgmt_i18n_string-ui-1445682-8.patch, failed testing.

miro_dietiker’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, tmgmt_i18n_string-ui-1445682-8.patch, failed testing.

blueminds’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, tmgmt_i18n_string-ui-1445682-8.patch, failed testing.

blueminds’s picture

Status: Needs work » Needs review
FileSize
29.62 KB

another try...

Status: Needs review » Needs work

The last submitted patch, tmgmt_i18n_string-ui-1445682-9.patch, failed testing.

blueminds’s picture

Status: Needs work » Needs review
FileSize
29.66 KB

and another try

Status: Needs review » Needs work

The last submitted patch, tmgmt_i18n_string-ui-1445682-10.patch, failed testing.

blueminds’s picture

Status: Needs work » Needs review
FileSize
29.42 KB

...

Status: Needs review » Needs work

The last submitted patch, tmgmt_i18n_string-ui-1445682-11.patch, failed testing.

blueminds’s picture

Status: Needs work » Needs review
FileSize
29.47 KB

...

Status: Needs review » Needs work

The last submitted patch, tmgmt_i18n_string-ui-1445682-12.patch, failed testing.

miro_dietiker’s picture

Not yet committed.. But once done, this is a starting list of followups we split from the current issue:
#2006416: i18n Strings: Support blocks
#2006422: i18n Strings: Support string record without object structure

Berdir’s picture

Status: Needs work » Needs review
FileSize
7.35 KB
29.25 KB

Here we go, this should be ready for an initial commit I think, explanations in next comment.

Berdir’s picture

+++ b/sources/i18n_string/tmgmt_i18n_string.moduleundefined
@@ -28,7 +16,9 @@ function tmgmt_i18n_string_tmgmt_source_plugin_info() {
-    if (isset($object_info['string translation']['type']) && in_array($object_type, tmgmt_i18n_string_implemented_types())) {
+    // @todo. Exclude objects that don't have a type: block, fields.
+    //   Currently not supported as they don't follow the standard pattern.
+    if (isset($object_info['string translation']['type'])) {

Instead of limiting it to a few, I just excluded the broken ones, which is block and field/field instance. Everything else is working now, including node types, contact categories, rules (see below), also tested one of my contrib modules (User Relationship types).

The problem with those that don't have a type is that they don't follow the standard pattern of type:id:property, block has module:delta, so basically a combined type and fields/instances are doing stuff like field_name:bundle:something (bundle isn't limited to the entity, so I guess this would be funny if you have multiple entity types with the same bundle ;))

+++ b/sources/i18n_string/tmgmt_i18n_string.moduleundefined
@@ -36,36 +26,10 @@ function tmgmt_i18n_string_tmgmt_source_plugin_info() {
-function tmgmt_i18n_string_get_db_type($type) {

The function was only used in a single place, when querying and there we can actually derive this information.

+++ b/sources/i18n_string/tmgmt_i18n_string.moduleundefined
@@ -74,11 +38,14 @@ function tmgmt_i18n_string_get_db_type($type) {
+  $info = i18n_object_info($type);
...
-  $select->condition('i18n_s.type', $db_type);
+  $select->condition('i18n_s.type', $info['string translation']['type']);
+  $select->condition('i18n_s.textgroup', $info['string translation']['textgroup']);

The db_type is actually the string translation type and we also need to filter on the textgroup. For example, both node types (textgroup node) and user relationship types (textgroup user_relationships) use type 'type'.

+++ b/sources/i18n_string/tmgmt_i18n_string.moduleundefined
@@ -117,7 +83,7 @@ function tmgmt_i18n_string_get_strings($db_type, $search_label = NULL, $limit =
-      if ((isset($object_info['string translation']['textgroup']['type']) && $object_info['string translation']['type'] == $type) || (!isset($object_info['string translation']['type']) && $object_type == $type)) {
+      if ((isset($object_info['string translation']['type']) && $object_info['string translation']['type'] == $type) || (!isset($object_info['string translation']['type']) && $object_type == $type)) {

Fixed a array nesting bug here that resulted in fatal errors with PHP 5.4 as it actually returned FALSE there.

+++ b/sources/i18n_string/tmgmt_i18n_string.moduleundefined
@@ -133,7 +99,8 @@ function tmgmt_i18n_string_form_i18n_string_translate_page_overview_form_alter(&
-  if (!in_array($object->get_type(), tmgmt_i18n_string_implemented_types())) {
+  $plugin_info = tmgmt_source_plugin_info('i18n_string');
+  if (isset($plugin_info['item types'][$object->get_type()])) {

Consistent way to check if the type is supported instead of relying on the (deleted) function.

+++ b/sources/i18n_string/tmgmt_i18n_string.moduleundefined
@@ -234,3 +201,18 @@ function tmgmt_i18n_string_translate_form_submit($form, &$form_state) {
+function tmgmt_i18n_string_i18n_object_info_alter(&$info) {
+  $entity_info = entity_get_info();
+  // Add a entity key to the object info if neither load callback nor entity
+  // keys are set and the object is an entity_type.
+  // @todo: Add this as default in EntityDefaultI18nStringController.
+  foreach ($info as $name => &$object) {
+    if (!isset($object['load callback']) && !isset($object['entity']) && isset($entity_info[$name])) {
+      $object['entity'] = $name;
+    }

This is ugly. entity.module provides generic i18n object controllers but they don't specify a load callback or entity type, i18n_string needs that to load the object behind it, no idea how it works currently, possibly it's passed in manually.

This should be fixed there but this works..

This was the main problem for rules_config, the other was an incorrect magic db_type calculation.

+++ b/sources/i18n_string/tmgmt_i18n_string.plugin.incundefined
@@ -52,8 +52,9 @@ class TMGMTI18nStringSourcePluginController extends TMGMTDefaultSourcePluginCont
-    if (!empty($data)) {
-      $item = array_shift($data);
+    if (!empty($data) && $keys = element_children($data)) {
+      $key = reset($keys);
+      $item = $data[$key];

This caused a lot of string as string index notices in PHP 5.4, not sure how it ever worked...

+++ b/sources/i18n_string/tmgmt_i18n_string.ui.incundefined
@@ -222,8 +222,8 @@ class TMGMTI18nStringDefaultSourceUIController extends TMGMTDefaultSourceUIContr
-    $form_state['redirect'] = array(url('admin/config/regional/tmgmt/jobs/' . $job->tjid,
-      array('query' => array('destination' => current_path()))));
+    $form_state['redirect'] = array('admin/config/regional/tmgmt/jobs/' . $job->tjid,
+      array('query' => array('destination' => current_path())));

This is what broke the testbot, that url() is wrong and it was then passed through url() twice.

Status: Needs review » Needs work

The last submitted patch, i18n-string-source-1445682-51.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
613 bytes
29.25 KB

Ups.

cgalli’s picture

Status: Needs review » Reviewed & tested by the community

applied patch and tested. works as described (after turning on the seperate module...)

Berdir’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed then, we already have the initial follow-ups I think. Yay, that was a fight ;)

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.