Enable Drupal for full multilanguage: i18n2core big patch

Some stuff I found while looking through the code is below. I have not tried the code in action yet, just reviewed the patch. Hope my findings/tips/questions help. Note that indentation problems I tried to point out will now show due to the nature of the filtering here...

- Why is language empty here? If it is because this allows for querying language independent variables, then this should be documented with a comment on the previous line. A space should be between = and ''.

-    $result = db_query('SELECT * FROM {variable}');
+    $result = db_query("SELECT * FROM {variable} WHERE language =''");

- Seems to be a bogus change, the WHERE should not have been removed:

-  db_query("DELETE FROM {variable} WHERE name = '%s'", $name);
+  db_query("DELETE FROM {variable} name = '%s'", $name);

- Why is i18n replaceable?

+        require_once variable_get('i18n_inc', './includes/i18n.inc');

- It would be nice to know why drupal_get_title() and drupal_set_title() was in path.inc, maybe they should stay there.

- System.install should have the CREATE TABLE statements also changed, not only the update path. BTW it is very nice from you to think update the update path up front :)

- Space should be around the = sign (in all SQL statements) AFAIK:

+    $ret[] = update_sql("UPDATE {node} SET language=''");

- $conf['i18n_variables'] should probably include a lot more, like user registration/activation/approval email text and such.

- I would write "i18n system bootstrap" and would simplify wording here. There is also a typo in there.

+/**
+ * i18n bootstrap system
+ * 
+ * This is the very minimum i18n bootstrap for multilingual sites
+ * Function to be called from bootstrap.inc before page cache
+ * Language must be set up after this function exits. This is needed this eary because there may be language dependent variables
+ */
+function i18n_bootstrap() {

- This behaviour should be noted in the settings.php comment. People might not expect to loose variables, if they comment out some of them for testing or some other reason in settings.php.

+          // Delete this variable for all languages as its not anymore in the collection
+          db_query("DELETE FROM {variable} WHERE name = '%s' AND language != ''");

- Performance wise this is surely a good idea, we will see how core comitters see it.

+  // This is a path.inc replacement to handle language in paths
+  $variables['path_inc'] = './includes/i18npath.inc';

- This only unsets variables in the current language. This is not consistent with the above deletion of variables in all languages. We should think about this.

+function i18n_variable_del($name) {
+  global $conf;
+  global $language;
+  
+  db_query("DELETE FROM {i18n_variable} WHERE name = '%s' AND language  = '%s'", $name, $language);
+  cache_clear_all('variables:'.$language);
+}

- This trims out the language from the path, but does not store it in $language, since i18n_get_lang_prefix() returns the language, and the return value is not used.

+function drupal_init_path() {
+  global $language;
+  
+  // Trim out language from path
+  i18n_get_lang_prefix($_GET['q'], TRUE);

- There are some serious indentation problems here. Two space indentation should be used (some lines are overly indented, some are under-indented). There are more similar problems later in the same function.

+      if (!isset($map[$path]) && !isset($map[$lang][$path])) {
+	      // Select aliases for all languages
+	      $result = db_query("SELECT dst, language FROM {url_alias} WHERE src = '%s' ORDER BY language", $path);
+	      if (db_num_rows($result)) {
+	        while ($current = db_fetch_object($result)) {
+	          if ($current->language) {
+	            $map[$current->language][$path] = $current->dst;
+              if ($current->language == $lang) {
+                $alias = $current->dst;
+              }
+	          } else {
+	            $map[$path] = $alias = $current->dst;
+	          }
+	        }
+          if ($alias) {
+            return $alias;
+          }
+	      } else {
+	        $map[$path] = $path;
+	      }  
+      }

- This small code is probably easier to integrate into the url() function directly, rather then as a separate function called with a conditional from url().

+function i18n_url($path, $query, $fragment, $absolute) {
+  global $language;
+  if ($language && variable_get('i18n_enable', 0) && !i18n_get_lang_prefix($path)) {
+    $path = $language.'/'.$path;
+  }
+  return array($path, $query, $fragment, $absolute);
+}

- Typo in phpdoc comment:

+/**
+ * Returns site wide default language
+ * If no languages are enabled, English will be returned. This may ve overriden by a variable
+ */
+function drupal_default_language() {

- Overindentation in the comment, plus attach the comment directly to the function, without a separating newline.

+/**
+ *	Gets language code for this page request, checking in order:
+ *
+ *	1. Path language - may be already set in $language
+ *	2. User language
+ *	3. Browser language, if browser language detection is enabled
+ *	4. Default system language
+ *
+ */
+
+function drupal_page_language() {

- browser_get_language() should be named differently. Either put it to the drupal_...() camp, or the i18n_...() functions. The browser prefix is confusing here and should not be used.

- The accept-language checking in browser_get_language() is quite simplistic and will not work in all cases. We tackled the problem at php.net with implementing what the RFC tells us about the format of that header. Look for the filling of the $browser_langs array in this code: http://www.php.net/source.php?url=/include/langchooser.inc Language codes are not necceserily two letter (even Drupal has pt and zh variants which are not two letter). So this assumption does not hold either.

- This is a rather broad reaching statement in i18n.module (and thus is not really true).

+ * @file
+ * Enables full multilingual sites.

- Comment indentation and attaching to function again.

+/**
+ *	Returns language code for this page request
+ *
+ */
+
+function i18n_get_lang() {

- i18n_get_lang() is not named according to Drupal conventions. Abbreviations are not used in Drupal. It should be i18n_get_language(). Apply this convention elsewhere (like i18n_get_lang_path()) if I missed other abbreviations.

- This should say *on* the settings page shouldn't it?

+    // Handle path when in i18n settings page and it's just been switched off
+    if (variable_get('i18n_enable', 0) && $_GET['q'] == $language.'/admin/settings/i18n') {
+      drupal_goto('admin/settings/i18n');
+    }

- This seems to be just a start :) BTW as you can see in other core modules a "You can" section is added to help texts, not a features list.

+      $output = '<p>'. t('The i18n module allows you to have multiple languages for your site content.') .'</p>';
+      $output .= '<p>'. t('Features:') .'</p>';
+      $output .= '<p>'. t('This module needs locale module for interface translation.') .'</p>';

- The description should point out that multiple language *content* and *settings* are handled by the module, not some general all covering multiple language support (like menus, taxonomy and such). There is also an indentation problem at the end.

+function i18n_menu($may_cache) {
+  $items = array();
+
+  if ($may_cache) {
+    $items[] = array(
+      'path' => 'admin/settings/i18n',
+      'title' => t('multi-language'),
+      'description' => t('Enable your site to handle multiple languages.'),
+      'callback' => 'i18n_admin_settings',
+      'access' => user_access('administer site configuration'));
+  
+  } 
+	
+	return $items;
+}

- Maybe add a link to the other language interfaces to edit the variable in other languages, or just add: "Switch language to edit different translations.".

+          $form[$name]['#description'] .= t('This is a <strong>multilingual variable</strong>.');

- i18n_form_alter() and _i18n_language_select() seem to have identation problems with form API arrays.

- Is _i18n_language_select() going to be used elsewhere? It is only used at one place now. Is there a need to have a separate function for it?

- i18n_supported_languages() seems to be quite simple to not exist IMHO. Removing it would also clear any possible confusion that Drupal supported languages and i18n supported languages are not the same.

- I would call $target as $path instead, which is in line with Drupal naming.

+function theme_i18n_link($text, $target, $lang, $separator = ' ') {

- Why is a setting for enabling the i18n functionality? I would expect i18n to work after I enable the module, not that I need to enable it, and then disable it before disabling the module. Drupal can tell you if a module is enabled, there is no need to have a variable for it which a user needs to check.

+function i18n_admin_settings() {
+  $form['i18n_enable'] = array(
+    '#type' => 'radios',
+    '#title' => t('Activate i18n system'),
+    '#default_value' => variable_get('i18n_enable', 0),
+    '#options' => array(t('Off'), t('On')),
+    '#description' => t('This will enable language in paths and multilingual variables. Make sure you switch it off before disabling this module.'));

I am sorry to say so, but this patch isn't better than what is going aroung now as i18n, ie a "solution" in the sense of "it works" more than in the sense of "it is good and core worthy".

Guess everybody expected that. :p

Can we split this up in smaller patches, and go one step at the time? It's kinda big right now.

By "expected that" I meant my reaction to the patch, not the patch itself, I had hoped for more.

Killes, I see this solution is not perfect. The i18n project contributors refined several solutions and came to this one. Nobody came around for years to suggest a better method. We can delay this indefinitely waiting for something perfect, or we can live with what we can do.

IMHO this is the same question as configurable node types. That feature is not perfect nor complete in core. Contrib can build on it and provide additional stuff. Now Drupal is not up to i18n for menus, variables, taxonomy, nodes, user profiles and stuff. We can try to tackle these all at once, but it is going to be a very huge (impossible) undertaking. Forms API wawing might mean something still :) A full i18n compatible Drupal would take a complete release or two. It is not going to happen in one good and core worthy patch. Also the remaining days until code freeze does not allow us to do more for the next release.

Last year in Amsterdam I've suggested two methods that I deem to be better to Jorge in person.

Jose, of course.

@Killes: are those written down somewhere? Could we look into the concept at least (no code required :)?

No, the ideas were all lost in the subsequent drinking binge... :p

Basically, one idea was to introduce another table similar tothe node_revisions table. The body would be moved the that table and there would be one to many relationship between the node and the node_revisions table on the one side and that third table on the the other.

We also considered to not add a third table but to add a language field to the node_revisions table.

Another possibility would be to add the one to many relationship as a key on (language, nid) directly in the node table. Basically a nid would not longer be unique in this case.

Basically, one idea was to introduce another table similar tothe node_revisions table. The body would be moved the that table and there would be one to many relationship between the node and the node_revisions table on the one side and that third table on the the other.

You mean that a revision of a node will have different language variants stored? What if I would like to fix text in a translated version and keep the revision history? If we advance the revision counter, then the original language variant and other translations will not be in sync (will relate to previous revisions).

We also considered to not add a third table but to add a language field to the node_revisions table.

How would you relate different translations to different taxonomy items (translated taxonomies supposed to have different tids)? Even different authors? Or just attaching the translator uid to the translation? Would you need to upload files (ie. illustrations in different languages) to the same original language node? How do you handle permissions (they are node level, not revision level). These last two also apply to the previous idea.

Another possibility would be to add the one to many relationship as a key on (language, nid) directly in the node table. Basically a nid would not longer be unique in this case.

Once again, how would you handle permissions for translators to their language variants (permissions are bound to nids)? Revisions are tied to nids, uploads are tied to nids, so the above attachment problem still applies.

I also gave this some second thought yesterday, and basically come to the conclusion that what we are aiming to here is another kind of multisite setup. We need first class nids, tids, variables, menu items and such, so we can reuse revisions support, node level permissions and all the node/taxonomy modules already implemented. Any translation could cover the original language version fully or can diverge from it significantly (different menu, different taxonomy, different nodes). Our solution differs from multisite setups in that:

• We select the 'site' (language) used based on request parameters, not just the URL
• We need fallbacks to use (ie. interface language, Drupal variable value and such) if not defined in the current language
• We need to relate node, terms, menu items and such together, so that langauge selectors can be provided if different language variants are available of an item

At first thought, it might be possible to implement i18n with multisite, if multisite is made more versatile (eg. runtime selection of the site used). We would need to share some tables, like modules used, users, sessions. We would need relation support for tables such as nodes, terms, menus etc.

But if you remember, i18n in a previous iteration used table prefixes (which is the basic of the multisite setup I am talking about) and it did not turn out to be working fine. Due to having different node tables for example, you would need to set up node level permissions in the different sites separately. You also need to upgrade the different datases separately (this is extremely tricky with shared tables since one language subsite would upgrade the shared tables and others will also try to but will fail). This is a general problem of how multisite is handled though. BTW another advantage of having one single site instead of multiple ones, is that you have a good language independent search index.

Jose prepared this patch, so we see where i18n is going. You see that the variables already have this multisite-like look in this patch (language column added, language dependent variables identified in an array and language based selection is used). Similar functionality is implemented for other objects in contrib.

You mean that a revision of a node will have different language variants stored? What if I would like to fix text in a translated version and keep the revision history? If we advance the revision counter, then the original language variant and other translations will not be in sync (will relate to previous revisions).

It is completely natural that the individual revisions won't be in sync. This just happens as you edit a node. What is important is that the current revision the one shown to the end-user. Complex workflow arrangements aren't supported by Drupal, yet. This is another issue to be dealt with elsewhere.

How would you relate different translations to different taxonomy items (translated taxonomies supposed to have different tids)?

That is what is wrong with the current approach in the first place: Taxonomy terms (or anything else) should have the same ID for the same thing.

Even different authors? Or just attaching the translator uid to the translation?

The uid of people doing the revisions is already stored.

Would you need to upload files (ie. illustrations in different languages) to the same original language node?

Yes.

How do you handle permissions (they are node level, not revision level). These last two also apply to the previous idea.

Permissions would stay as they are. It doesn't make sense to allow access to a node in one language and not in another one. If you are concerned about editing rights: This could be easily checked through a variety of ways.

Once again, how would you handle permissions for translators to their language variants (permissions are bound to nids)? Revisions are tied to nids, uploads are tied to nids, so the above attachment problem still applies.

Well, I don't see it as a problem. You can enforce the loading of the correct language per translator, for example.

I also gave this some second thought yesterday, and basically come to the conclusion that what we are aiming to here is another kind of multisite setup. We need first class nids, tids, variables, menu items and such, so we can reuse revisions support, node level permissions and all the node/taxonomy modules already implemented. Any translation could cover the original language version fully or can diverge from it significantly (different menu, different taxonomy, different nodes). Our solution differs from multisite setups in that:

* We select the 'site' (language) used based on request parameters, not just the URL
* We need fallbacks to use (ie. interface language, Drupal variable value and such) if not defined in the current language
* We need to relate node, terms, menu items and such together, so that langauge selectors can be provided if different language variants are available of an item

At first thought, it might be possible to implement i18n with multisite, if multisite is made more versatile (eg. runtime selection of the site used). We would need to share some tables, like modules used, users, sessions. We would need relation support for tables such as nodes, terms, menus etc.

Well, you are welcome to try what you describe. I wouldn't buy this as a multisite concept, though.

But if you remember, i18n in a previous iteration used table prefixes (which is the basic of the multisite setup I am talking about) and it did not turn out to be working fine. Due to having different node tables for example, you would need to set up node level permissions in the different sites separately. You also need to upgrade the different datases separately (this is extremely tricky with shared tables since one language subsite would upgrade the shared tables and others will also try to but will fail). This is a general problem of how multisite is handled though. BTW another advantage of having one single site instead of multiple ones, is that you have a good language independent search index.

See above, I don't think multisite is a viable solution. Sharing of certain tables might be, though. I however would greatly prefer to have everything consolidated into one site.

Jose prepared this patch, so we see where i18n is going.

The results are rather disappointing.

You see that the variables already have this multisite-like look in this patch (language column added, language dependent variables identified in an array and language based selection is used). Similar functionality is implemented for other objects in contrib.

I've had a look at the implementation of variable_set to get a feel for what Jose is trying to do. What do I see?

if (function_exists('i18n_variable') && i18n_variable($name)) {
+ i18n_variable_set($name, $value, $language);
+ } else {

The whole approach has a hackish air about it. It somehow remembers me of mambo, don't ask me why.

I feel very sorry that I asked Dries to accept the patch which introduced the current i18n hook. Why? It allowed the current hackish solution to exist and didn't force the people to think about a real solution.

Killes, nobody forces us not to think. We are moving with i18n closer to core, and this luckily opens up some discussions. You see you did not write your ideas down before (or you did not care to point me to them instead of reiterating them). But now we see them. This is great.

That is what is wrong with the current approach in the first place: Taxonomy terms (or anything else) should have the same ID for the same thing.... Permissions would stay as they are. It doesn't make sense to allow access to a node in one language and not in another one. If you are concerned about editing rights: This could be easily checked through a variety of ways.

IMHO what you miss is that there are different multilanguage scenarios.

Use case 1:
Site of a town in Germany. Local residents get news, information about local offices, opening hours and such. English/French visitors get an overview of the city, the major tourist attractions, some offices and opening hours. You see that the menu and taxonomy structure will overlap a bit, and to help translators, we should relate different language versions together. Complete subsites will only exist in one language. Compare http://www.berlin.de/ and http://www.berlin.de/english/index.html for a real world example.

Use case 2:
International newspaper having different editions around the globe, or different language local variants. Think of National Geographic or the Belgian Metro newspaper which is published in French and Dutch. You can also think of product manuals on a website. These ideally have the same content published, so original language and translated language variants always find their matching pairs. Structure should be the same, menus, taxonomy should be equal, just the actual text differs.

I think Drupal should support both. As far as I see from your intentions, you are solely focused on multilingual nodes. But multilingual sites are much farther from there. It can happen that a different language site variant is a complete translation, but it rather happens that whole categories should be removed/added in the translated variant, that the menu structure is at least slightly different and such.

You say people working on different languages for the town, newspaper or product manuals should have the rights to the original text. Newspaper teams or European governmental sites having stricter workflows (which is possible if we use first class nodes with the workflow module, if we explicitly not deny it with a restictred design) will not agree with you. I see translation projects having multistep workflows defined, so that a text's translated version needs to go through some hoops before being published.

See above, I don't think multisite is a viable solution. Sharing of certain tables might be, though. I however would greatly prefer to have everything consolidated into one site.

Why I say is that we should think of the translated version as a multisite scenario (conceptually!) is because different language variants of a site will not overlap with each other in their structure, forum topics, permissions, themes (logo and other images at least). They should overlap in users, and should fall back to a default language is several circumstances though. I perfectly agree that it would be best to have everything consolidated into one site. I just tried to point out that conceptually we are introducing multisite-like features (regardless of whether Jose's or some other route is taken).

I've had a look at the implementation of variable_set to get a feel for what Jose is trying to do. What do I see?

if (function_exists('i18n_variable') && i18n_variable($name)) {
+ i18n_variable_set($name, $value, $language);
+ } else {

The whole approach has a hackish air about it. It somehow remembers me of mambo, don't ask me why.

OK, it is possible to integrate the language dependant variable handling directly into variable_get/set. You will not be happy about performance, but then so be it.

You are free to come up with ideas for this issue beyond to the node system. What we are trying to do here is to intorduce a language column into relevant tables, so we keep all objects first class (which is a required outcome), and then apply language dependant data handling throughout Drupal. I don't see how your patterns on the node table extend to the menu, taxonomy, variables, profiles, cck fields and other objects. The language aware first class objects way offered here is one possible solution. I see you are not happy with the concept and not happy with the implementation.

We should probably discuss the broader concept first, before picking on implementation and then refine an implementation for the aggreed on concept. I don't think code is golden then discussion here. At least until we have such fundamentally different thinking about what should an i18n system be capable of. I would like to get the most out of the system.

Subscribing... and: Regarding to use case 1 in #13 such kind of "internationalization" should be done by path redirects. I don't see any magic in creating a menu item "English" (/english) displaying a custom page or taxonomy along with the submenu entries of /english. You even do not need i18n to accomplish this. However, menu items or taxonomy should support forcing a specific language so that blocks like in the given example won't be displayed in the default language.

After discovering Localizer module I'd rather prefer to get that complete module into core someday.