[D6] I18N Selection Page

This kind of functionality is very welcome. There seems to be no easy way in Drupal (6) to achieve this.

Looking at the attached screenshot (http://drupal.org/files/issues/ScreenShot136.jpg), some questions pop up:
1) To which page are the users redirected after selecting a language on the selection page? Ideally, they would be redirected to the initial URL but with the language prefix inserted.
2) The options under 'Use cookie to remember your language': can you elaborate what exactly the differences are between the three options? (e.g. the selection page is not bypassed when you select 'Oui', or is it? If not, what is the difference with selecting 'Non'?)
3) Does the selection page have a 'Do not ask again next time' checkbox?

Rgds,
tvb

In some countries that have several official languages, the only political correct way of serving the visitor the website in his preferred language is by letting that visitor choose the language explicitly (instead of letting Drupal decide using url, user-agent, ip address, etc).

So I think Pol's module is quite useful for this

Hi Pol,

I reviewed the code and noticed some issues:

• i18n_selection_page.info line endings are wrong, include \r.

• // This is the name of the cookie. You can change it if you want.
  define('LANGUAGE_COOKIE_KEY', 'language_prefix');
  

  If this is a variable users might edit, it should be stored as a system variable. Users in general should not be editing module files, as that complicates updates. I'm not sure why this would need to vary between sites.

  If you're going to keep it as a constant and remove the suggestion to change it, constant names should prefixed with module names to avoid conflicts. For example, if a module named "language_cookie" defined the same constant, the two modules couldn't work together. The same name conflict can happen in the cookie name as well. To avoid these problems, it should be something like this instead: define(' I18N_SELECTION_LANGUAGE_COOKIE_KEY', 'i18n_selection_language_prefix'); You'll also need to change that everywhere the constant is used.

• i18n_selection_page_requirements() should be in the .install file rather than the .module file.
• i18n_selection_page_body.tpl.php contains text that isn't wrapped in t(), so it can't be translated. This seems especially odd given the nature of the module. Is this intentional?

Okay, this looks good to me.

Sorry to do this, I'm setting it back to needs work. But the change I think should happen is small - the rest are suggestions.

Required changes

The administration menu callback should probably use "administer site configuration" - which implies the user can change something - rather than "access administration pages" which is about viewing but not changing configurations.

Suggested changes

Please take a moment to make your project page follow tips for a great project page.

This code seems hard to follow/maintain:

80 switch ($behavior) {
81 case 9:
82 case 10:
83 case 18:

I suggest adding some comments explaining what the special numbers mean. Since they are used in various places around the code you could add the comment to the top of the .module since that is a likely spot to be found.

I noticed some very small code style issues. Please run the coder module on "minor" setting to help catch these. The coding standards have even more information in this area.

There seems to be something wrong with the loading of the theme / preprocess functions.
On a clean D6 site i get a WSOD when choosing "Template only".
When choosing "Template in theme", the links are not shown - like this: http://grab.by/b4Iw
I ran this through a debugger, and it doesn't arrive in the preprocess functions. If I find time later I might try to find the cause of the problem, but thought I'd let you know already.

In i18n_selection_page.admin.inc you are passing variables into the t() function, whereas this should be a literal string.

Because t() is designed for handling code-based strings, in almost all cases, the actual string and not a variable must be passed through t().

See http://api.drupal.org/api/drupal/includes--common.inc/function/t/6

In i18n_selection_page.admin.inc line 41: you should use notation with brackets according to Drupal coding standards.

In i18n_selection_page.admin.inc line 65: you should probably use a placeholder for the variable in t() (not sure if this is also true for constants, but better safe than sorry)

This is a useful module. Keep up the good work ;)

hello, where can i download the module?

@euroba It's not available for download yet. It needs to be reviewed and then approved and then Pol can turn it into a full project.

This issue should have been in "needs review" status where it could get reviewed.

@Pol, please be sure to set it back to needs review when you're ready for more reviewing.

i think it will be suitable for my purpose, see http://drupal.org/node/1344084
i hope i can use this module as soon as possible.

best regards

Automated review (Please keep in mind that this is primarily a high level check that does not replace but, after all, eases the review process. There is no guarantee that no other issues could show up in a more in-depth manual follow-up review.)

Review of the 7.x-1.x branch:

• Run coder to check your style, some issues were found (please check the Drupal coding standards):
  

  Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards
  
  sites/all/modules/pareview_temp/test_candidate/i18n_selection_page.module:
   +33: [minor] Comment should be read "Implements hook_foo()."
   +102: [minor] Comment should be read "Implements hook_foo()."
   +168: [minor] indent secondary line of comment one space 
  
  Status Messages:
   Coder found 1 projects, 1 files, 3 minor warnings, 0 warnings were flagged to be ignored
  

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards):
  

  
  FILE: .../all/modules/pareview_temp/test_candidate/i18n_selection_page.admin.inc
  --------------------------------------------------------------------------------
  FOUND 2 ERROR(S) AND 6 WARNING(S) AFFECTING 8 LINE(S)
  --------------------------------------------------------------------------------
   20 | WARNING | A comma should follow the last multiline array item. Found: )
   29 | WARNING | A comma should follow the last multiline array item. Found: )
   42 | WARNING | A comma should follow the last multiline array item. Found: )
   50 | WARNING | A comma should follow the last multiline array item. Found: )
   59 | WARNING | A comma should follow the last multiline array item. Found: )
   69 | ERROR   | Missing function doc comment
   81 | ERROR   | Missing function doc comment
   91 | WARNING | Line exceeds 80 characters; contains 81 characters
  --------------------------------------------------------------------------------
  
  
  FILE: ...es/all/modules/pareview_temp/test_candidate/i18n_selection_page.install
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   8 | WARNING | Format should be * Implements hook_foo().
  --------------------------------------------------------------------------------
  
  
  FILE: ...tes/all/modules/pareview_temp/test_candidate/i18n_selection_page.module
  --------------------------------------------------------------------------------
  FOUND 14 ERROR(S) AND 13 WARNING(S) AFFECTING 24 LINE(S)
  --------------------------------------------------------------------------------
     2 | ERROR   | Missing file doc comment
    18 | WARNING | A comma should follow the last multiline array item. Found:
       |         | MENU_VISIBLE_IN_BREADCRUMB
    26 | WARNING | A comma should follow the last multiline array item. Found:
       |         | MENU_NORMAL_ITEM
    33 | WARNING | Format should be * Implements hook_foo().
    48 | WARNING | Avoid backslash escaping in translatable strings when
       |         | possible, use "" quotes instead
    49 | WARNING | A comma should follow the last multiline array item. Found:
       |         | //'config' =>
       |         | 'admin/config/regional/language/configure/selection_page'
  
    49 | ERROR   | Missing space after the // 
    58 | ERROR   | "require_once" is a statement not a function; no parentheses
       |         | are required
    58 | ERROR   | File is being conditionally included; use "include_once"
       |         | instead
    65 | ERROR   | Inline control structures are not allowed
    66 | ERROR   | Inline control structures are not allowed
    67 | ERROR   | Inline control structures are not allowed
    68 | ERROR   | Inline control structures are not allowed
    69 | ERROR   | Inline control structures are not allowed
    70 | ERROR   | Inline control structures are not allowed
    75 | WARNING | Line exceeds 80 characters; contains 115 characters
    75 | ERROR   | Missing space after the // 
    76 | ERROR   | Missing space after the // 
    89 | ERROR   | Missing space after the // 
    95 | ERROR   | Missing space after the // 
    97 | WARNING | A comma should follow the last multiline array item. Found: )
   106 | WARNING | Line exceeds 80 characters; contains 92 characters
   118 | WARNING | Line exceeds 80 characters; contains 105 characters
   119 | WARNING | Line exceeds 80 characters; contains 121 characters
   120 | WARNING | Line exceeds 80 characters; contains 119 characters
   130 | WARNING | Line exceeds 80 characters; contains 154 characters
   140 | WARNING | Line exceeds 80 characters; contains 201 characters
  --------------------------------------------------------------------------------
  
  
  FILE: .../all/modules/pareview_temp/test_candidate/i18n_selection_page.pages.inc
  --------------------------------------------------------------------------------
  FOUND 6 ERROR(S) AND 1 WARNING(S) AFFECTING 4 LINE(S)
  --------------------------------------------------------------------------------
   22 | ERROR   | Missing space after the // 
   25 | ERROR   | Inline control structures are not allowed
   26 | ERROR   | A whitespace must prefix the item assignment operator =>
   26 | ERROR   | A whitespace must follow to the item assignemtn operator =>
   26 | ERROR   | There must be a single space before an operator statement
   27 | WARNING | Line exceeds 80 characters; contains 98 characters
   27 | ERROR   | Missing space after the // 
  --------------------------------------------------------------------------------
  
  
  FILE: ...all/modules/pareview_temp/test_candidate/i18n_selection_page.themes.inc
  --------------------------------------------------------------------------------
  FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
  --------------------------------------------------------------------------------
    8 | ERROR | Missing function doc comment
   12 | ERROR | Missing function doc comment
  --------------------------------------------------------------------------------
  
  
  FILE: ...s/pareview_temp/test_candidate/includes/i18n_selection_page.helpers.inc
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   82 | WARNING | Line exceeds 80 characters; contains 98 characters
  --------------------------------------------------------------------------------
  

• README.txt is missing, see the guidelines for in-project documentation.
• @file doc block is missing in the module file, see http://drupal.org/node/1354#files .
• Comments: there should be a space after "//", see http://drupal.org/node/1354#inline
  

  i18n_selection_page.module:76:    //drupal_set_message("Redirect to $url",'warning');
  

• ./includes/i18n_selection_page.helpers.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function _i18n_resolve_lang() {
  function _i18n_detect_lang_from_url() {
  function _i18n_detect_lang_from_cookie() {
  

• ./i18n_selection_page.module: The description on the line after the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
  

  36- */
  

• ./includes/i18n_selection_page.helpers.inc: The description on the line after the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
  

  10- */
  26- */
  40- */
  58- */
  74- */
  86- */
  

• ./i18n_selection_page.admin.inc: The description on the line after the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
  

  10- */
  

• ./i18n_selection_page.install: The description on the line after the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
  

  11- */
  28- */
  44- */
  

• Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
  

  
  i18n_selection_page.info
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./i18n_selection_page.module ./themes/i18n_selection_page_body.tpl.php ./themes/css/i18n_selection_page.css ./includes/i18n_selection_page.helpers.inc ./i18n_selection_page.pages.inc ./i18n_selection_page.admin.inc ./i18n_selection_page.install ./i18n_selection_page.themes.inc ./i18n_selection_page.info
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Manual addition: Find the 6.x-1.x results attached.

HTH, dave

D7 related: I did not get that at first sight, really sorry!

Will check the changes in 6.x asap then. Thank you for clarifying!

Review of the 6.x-1.x branch:

• Run coder to check your style, some issues were found (please check the Drupal coding standards). See attachment.
• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

manual review:

• i18n_selection_page_uninstall(): why do you unset a cookie here? The cookie is only removed from the admin's browser session that uninstalled the module. So it is pretty useless.
• i18n_selection_page_menu(): 'administer site configuration' is a very generic permission, why can't you reuse a more fitting permission from i18n?
• i18n_selection_page_init(): The arithmetic you are doing in there seems rather complex, so I don't think it increases the readability. Also the variable values are rather obscure if you look at them independently.
• "Implementation of the hook_form_validate().": This is not a hook, it is a form handler. See http://drupal.org/node/1354#forms
• i18n_selection_page_admin_submit(): as for the uninstall function, you can only remove the cookie for one admin session, so it is quite useless.
• i18n_selection_page_admin(): why do you need $error here? Just return the empty array as you it in the first if statement.
• i18n_selection_page.css: formatting errors, please see the CSS coding standards http://drupal.org/node/302199
• i18n_selection_page_init(): why do you check $_SERVER['SERVER_ADDR'] in there? Add a comment to explain it.

http://drupal.org/node/1381494#comment-5513060

Hello Pol,

I have tried the module on an internal development site with two languages.

Setup:
- Bilingual D6 site in dutch (nl) & french (fr) (nl is default language; english is disabled).
- There are two panel nodes in 1 translation set used as homepages (node/4463 for nl; node/4464 for fr). Both panels have fpanel1 as URL alias.
- Both panel nodes are overwritten by panels to display panes based on the language of the node being viewed (via a visibility rule).
- site_frontpage is set to fpanel1 (set in dutch interface; site_frontpage is not multilingual enabled because it may conflict with domain access module).

I don't know whether this is the recommended setup for a bilingual site, but this is how it is configured currently. It works fine using http://example/nl or http://example/fr : all content panes are displayed correctly.
But this setup does not provide for a neutral language selection page in case there is no language prefix found in the URL or in a cookie. This is where the i18n selection page module fits in.

These are my comments:
1) When the browser requests http://example for the first time (no cookie available), the redirect message displays the alternative links http://example/nl/fpanel1 for dutch and http://example/fr/node/4463 for french. But node id 4463 is the dutch version in the translation set, not the french version (4464). The result is a page with mixed french and dutch content.
Is it possible to have the french link use the alias also?

2) When a cookie has been set to use the french language and upon requesting http://example, there is redirection to http://example/fr/node/4463. This is however the dutch version in the translation set instead of the french version. (see point 1 also)

3) When the redirect message is set to by displayed in the template ('Template in theme'), the template also displays page elements in the default language (menu, sitename...) while the user has yet to select a language. I suppose this is because the URL of the page has a language prefix inserted (http://example/nl/language_selection?destination=node%2F4463).

4) When no cookie has been set and upon requesting a view without a language prefix (eg. http://example/view instead of http://example/nl/view or http://example/fr/view), there is an immediate redirection to the version in the default language.
This is also the case for a regular node (http://example/node/44 redirects to http://example/nl/node/44).
My expectation is that the redirection message is displayed in this case, but it isn't.

5) The module does not seem to work without clean URL's. Maybe it should be listed as a requirement somewhere?

Workarounds:
I found this out earlier this week. Installing the Global redirect module with 'Language path checking' enabled, offers a workaround for 1) and 2). It alters the node ID so that it corresponds to the language prefix.

That's it for now. I hope this is the kind of feedback you are looking for!

Rgds,
tvb

(No change of status because of http://drupal.org/node/1381494#comment-5513060)

This URL is not functioning: http://git.drupal.org/sandbox/Pol/1191004.git
I really need the cookie functionality for my website. It's incredible that is not builtin into Drupal.
Where I can download your module?
Many thanks