2.x initial merge

Subscribing

subscribing.

subscribing

Subscribing.

I applied that patch and installed a fresh D7 with the patched translation module. Now, the translation page (http://localhost/d7/node/1/translate) of an article is giving the following error:

Fatal error: Cannot use string offset as an array in C:\project7\d7\sites\all\modules\translation\translation.handler.node.inc on line 24

subscribing

subscribing

does Translation module replace Internationalization module in Drupal 7?
sorry the project page did not clearly indicate so would like to confirm

Thanks for pointing to those! I'll try to help/review ASAP.

Given that we're still changing field API and also TF functionality in core HEAD, I think it would be wise to still defer work here, until we can be a bit more sure that we don't have to chase HEAD every other day. Agreed?

Subscribing...

subscribing... any idea when a next patch will follow?

My first review, checking the rough edges only, didn't really come to translation.admin.inc:

+++ translation.module	11 Aug 2010 01:14:11 -0000
@@ -1,93 +1,210 @@
+    'comment' => array(
+      'object keys' => array(
+        'human readable id' => 'subject',

Why does a property called "human readable id" have a value that references an entity property of the entity's title? That looks odd. :)

+++ translation.module	11 Aug 2010 01:14:11 -0000
@@ -1,93 +1,210 @@
+    'node' => array(
+      'translation' => array(
+        'translation' => array(
...
+    'taxonomy_term' => array(
+      'translation' => array(
+        'translation' => array(

Do I have to understand the duplicate/recursive 'translation' keys?

+++ translation.module	11 Aug 2010 01:14:11 -0000
@@ -1,93 +1,210 @@
+          'base path' => 'user/%user',
+          'edit path' => 'user/%user/edit',
...
+          'edit form' => TRUE,

It looks like

1) 'edit path' could default to '!basepath/edit'

2) 'edit form' could default to TRUE. Though I'm not sure what this property means and why comment doesn't have it (because comments have an edit form, too).

+++ translation.module	11 Aug 2010 01:14:11 -0000
@@ -1,93 +1,210 @@
+      if (!isset($entity_info[$entity_type]['translation']['translation']['base path'])) {
+        $entity_info[$entity_type]['translation']['translation'] += array(
+          'base path' => "entity/{$entity_type}/%translation_menu_entity",
+          'load arguments' => array($entity_type, 2),
+        );

It seems that this patch already contains a common/generic translation functionality for any fieldable entity, performed on dedicated menu paths provided by Translation module. I'd consider that a pretty advanced use-case already and am wondering whether we may want to defer that advanced/generic functionality to a separate follow-up issue/patch?

+++ translation.module	11 Aug 2010 01:14:11 -0000
@@ -1,93 +1,210 @@
+        $entity_info[$entity_type]['translation']['translation']['view path'] = $entity_info[$entity_type]['translation']['translation']['base path'];

I already wanted to suggest 'view path' instead of 'base path' for hook_translation_info(), but now I suddenly see that base path is taken over as view path. I don't understand the difference between both.

+++ translation.module	11 Aug 2010 01:14:11 -0000
@@ -1,93 +1,210 @@
+        'page callback' => 'translation_overview',
...
+      $items["$path/translate/overview"] = array(
+        'title' => 'Overview',
+        'type' => MENU_DEFAULT_LOCAL_TASK,
+        'weight' => 0,

I wonder why "overview" instead of "List"? (both in title and callback)

Also, default local tasks should always have a weight of -10; most often eliminating the need to set special weights for other tasks.

+++ translation.module	11 Aug 2010 01:14:11 -0000
@@ -1,93 +1,210 @@
+      $items["$path/translate/%translation_language/%translation_language"] = array(
+        'title' => 'Translate',
...
+      $items["$path/translate/delete/%translation_language/%translation_language"] = array(
+        'title' => 'Delete',

Each "task" should have a verb, not only the delete task. There are two ways, either prefix the verb (as for delete here), or append the verb (normally done if the tasks belong to something considered a fixed thing/"entity").

We should also keep in mind that we may need to add other tasks in the future.

So either

$path/translate/edit/%/%
$path/translate/delete/%/%

or

$path/translate/%/%/edit
$path/translate/%/%/delete

I wonder why there is no "add" task; suspect that there's no real difference between edit and add (yet)?

+++ translation.module	11 Aug 2010 01:14:11 -0000
@@ -1,93 +1,210 @@
+    'file' => 'translation.admin.inc',
+    'file path' => $include_path,

translation.admin.inc should be located in the module's root folder.

Not really sure about the entire layout chosen here. I don't think there's a need to put sub-modules into an additional "modules" sub-directory; ./translation_node/translation_node.module is just fine.

+++ translation.module	11 Aug 2010 01:14:11 -0000
@@ -95,338 +212,319 @@
+/**
+* Implements hook_permission().
  */

I wanted to ignore this for now, but there are quite a lot of wrongly formatted PHPDoc comments we have to fix at some point.

+++ translation.module	11 Aug 2010 01:14:11 -0000
@@ -95,338 +212,319 @@
+    'translate all entities' => array(
+      'title' => t('Translate all entities'),

Should we rename this permission to "translate any entity"? Views and Panels had this very similar and security-problematic issue once.

+++ translation.module	11 Aug 2010 01:14:11 -0000
@@ -95,338 +212,319 @@
+      // Perform theming here because the default theming needs to set system
+      // messages. It would be too late in the #post_render callback.
+      $output['#translation_unavailable'] = theme('translation_unavailable', array('element' => $output));
+      $output['#post_render'][] = 'translation_unavailable';

This is odd.

Also, this seems to invoke a theme function directly, but the theme function just displays a message. So why isn't the message simply set in translation_field_attach_view_alter() already instead of a #post_render callback?

Lastly, I wonder why we are setting a message at all? When rendering 100 entities, this may potentially display 100 messages?

+++ translation.module	11 Aug 2010 01:14:11 -0000
@@ -95,338 +212,319 @@
+  $args = array('%language' => t($GLOBALS['language_content']->name), '%id' => $handler->getHumanReadableId());
+  drupal_set_message(t('%language translation unavailable for %id.', $args), 'warning');

As already mentioned for hook_translation_info(), it seems like %id and getHumanReadableId() are not refering to an ID, but an entity's title. Why not simply %title and getTitle() ?

+++ translation.module	11 Aug 2010 01:14:11 -0000
@@ -95,338 +212,319 @@
+function translation_field_attach_insert($entity_type, $entity) {
...
+    $handler = translation_get_handler($entity_type, $entity);
+    $handler->initTranslations();
+    $handler->save();
...
+function translation_field_attach_update($entity_type, $entity) {
...
+    $handler = translation_get_handler($entity_type, $entity, TRUE);
+    $handler->updateTranslations();
...
+function translation_field_attach_delete($entity_type, $entity) {
...
+    $handler = translation_get_handler($entity_type, $entity);
+    $handler->clearTranslations();

Didn't look at the handlers yet, but

1) "init" + "update" + "clear" sound a bit arbitrary to me. I also wonder why we need those "Translations" suffixes. "update" is the only that makes sense, although I suspect it's not that kind of "update" I am currently thinking of, as these are low-level Field API hooks. "clear" should likely be "delete". No idea what "init" means or could be.

2) Note that there's a 3rd argument TRUE in the update hook. If that's intentional, it might deserve an inline comment.

+++ translation.module	11 Aug 2010 01:14:11 -0000
@@ -95,338 +212,319 @@
+    $translations = $handler->getTranslations();

Same as above: why "get" instead of "load"? In short, I'd prefer to see usual CRUD method names here, i.e., insert(), load(), update(), delete(). Only use more elaborate method names for non-standard non-CRUD functions, such as getTitle() or similar.

+++ translation.module	11 Aug 2010 01:14:11 -0000
@@ -95,338 +212,319 @@
+function translation_edit_form_submit($form, &$form_state) {
+  $info = translation_edit_form_info($form);
+  $handler = translation_get_handler($info['entity type'], $info['entity']);

It looks like we could store $info and perhaps even $handler as custom #info and #handler properties within $form.

+++ translation.module	11 Aug 2010 01:14:11 -0000
@@ -95,338 +212,319 @@
+  $handler->setOutdated($outdated);

How about expire() ?

+++ translation.module	11 Aug 2010 01:14:11 -0000
@@ -95,338 +212,319 @@
+function translation_form_field_ui_field_settings_form_alter(&$form, $form_state) {
...
+    '#type' => 'radios',
+    '#title' => t('Multilingual settings'),
+    '#options' => array(TRUE => t('Translatable field'), FALSE => t('Language neutral field')),

Will there ever be more multilingual choices? If not, I think this should be a simple checkbox instead of radios.

At the very least, the internal values should be 1 and 0 instead of TRUE and FALSE, I think.

+++ translation.module	11 Aug 2010 01:14:11 -0000
@@ -95,338 +212,319 @@
+ * @param $update
+ *   Instances are statically cached: if this is TRUE the wrapped entity will
+ *   be replaced by the passed one.
...
+function translation_get_handler($entity_type, $entity, $update = FALSE) {
...
+  if (empty($entity_id)) {
+    $update = TRUE;
+  }

$update is normally called $reset.

Also not sure why $update (reset) is set/enabled internally if the passed $entity has no id - that's odd. :)

+++ modules/translation_node/translation_node.info	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,10 @@
+name = Content translation (Node)
+description = Allows nodes to be translated into different languages by creating a new node for each translation.

oh, just wanted to ask why we have a entity-specific sub-module, but yeah, this just seems to be the old Translation module 1.x.

Somehow, however, I thought we'd intermix this functionality into the new module, instead of a separate module. But that's totally not a priority right now, I guess.

+++ modules/translation_upgrade/translation_upgrade.install	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,249 @@
+function translation_upgrade_start() {
+  // @todo We need a token against CSRF here.

Let's just use a regular form and manually invoke the upgrade. Anything else is too complex, and auto-upgrading may also not what people want. Users may mistakenly enable the module.

+++ includes/translation.admin.inc	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,447 @@
+function translation_admin_form_submit($form, $form_state) {
+  menu_rebuild();

Should also clear entity info cache?

+++ includes/translation.admin.inc	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,447 @@
+  // Ensure that we have a coherent status between entity language and field
+  // languages.
+  if ($handler->initOriginalTranslation()) {
+    // FIXME!
+    field_attach_update($entity_type, $entity);
+  }

Odd. Do we need to open a core issue for that?

+++ includes/translation.admin.inc	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,447 @@
+  // @todo: Do we want only enabled languages here?
+  $languages = language_list();

All available (including disabled) looks correct to me.

+++ translation.api.php	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,48 @@
+        'class' => 'TranslationCustomEntityHandler',

Shouldn't "class" be "handler" ?

+++ includes/translation.handler.inc	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,518 @@
+  public function getTranslations();
...
+  public function setTranslation($translation, $values = NULL);
...
+  public function removeTranslation($langcode);

I'm still trying to understand what's actually going on, but it looks like these methods should be renamed from "Translations" to "Language" -- they are loading, saving, and deleting values for a certain language, right?

Either way, we have to do something on those method names... all of them are called similarly, but do entirely different things. If everything else fails (though that's unlikely), then just removing the "Translation(s)" suffixes would help a lot already.

+++ includes/translation.handler.inc	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,518 @@
+  public function getBasePath();
...
+  public function getEditPath();
...
+  public function getViewPath();

A single getPath($type) would be sufficient. Also not sure whether we need a method for that at all though.

+++ includes/translation.handler.inc	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,518 @@
+  public function load() {
+    if (isset($this->entityId)) {
+      $this->loadMultiple($this->entityType, array($this->entityId => $this->entity));
+    }
+    else {
+      $this->entity->{$this->getTranslationsKey()} = $this->emptyTranslations();
+    }
+  }
...
+  public function getTranslations() {
+    $translations_key = $this->getTranslationsKey();
+
+    if (!isset($this->entity->{$translations_key})) {
+      $this->load();
+    }
+
+    return $this->entity->{$translations_key};
+  }

This code confuses me :)

It looks like both methods are the same as load(), just that load() does not check whether it already loaded currently.

+++ includes/translation.handler.inc	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,518 @@
+  public function save() {
+    $this->writeTranslations();
...
+  protected function writeTranslations() {

hum... it's very hard to review all other handler methods, since closely related methods are defined miles away from each other. Can we keep related methods together?

Powered by Dreditor.

subscribing...

subscribing...

Just started to use the new interface.
I made some small changes to the current patch get it working without warnings.
Changes I made:
translation.module
Removed [entity keys][label] definitions in translation_translation_info().
Usually hook_entity_info has this already set - by using array_merge_recursive() in translation_entity_info_alter the duplicated definition led to an array in [entity keys][label].

translation.admin.inc
In translation_overview() the variable $path was used for different purposes - what led to misleading links. I changed one to $translatePath.

includes/translation.handler.inc
Changed method getLabel() to use the core function entity_label() for loading the label.

Currently there are other issues / questions I'm working on:

• Is there a smart way to translate the node title? It's not a translatable field.
• Clean URL's seem partially broken for the original language - but I've to drill down further to confirm that. Maybe it's another module which causes my current trouble.

What about committing the current state into the CVS? Would make future changes more traceable.

I'm totally sorry -- wasn't able to spare some time to review + test this patch once more. Please note that http://drupal.org/node/363367 states

However, if patches are unnecessarily hold-off because of missing reviews, then they should be committed, so development can go on. Most often, developers are involved in a bunch of other projects.

If you feel comfortable with the possibility of potentially doing major changes after the initial commit, then I'd say we should simply move on. After all, the current patch/code went through at least some reviews already (including the core TF UI issue).

Not sure whether we already created the DRUPAL-7--1 branch to hold the identical code as Translation module in core (which will allow us to compare core's code to our last known code), but in any case, we should ensure that HEAD contains the most recent code of the core Translation module (and branch that into DRUPAL-7--1 afterwards) before turning HEAD into 2.x (this patch).

Thanks for your hard work on this, plach!

EDIT: Let's do @das-peter's changes in a follow-up issue/patch.

Thank you both for the fast feedback.
I'm on standby and create the patch, as soon as the repo is up to date. :)

Awesome! Thank you, plach!

That's great - thanks!
Follow up issue created here: #920826: Several fixes after the initial merge