Ticket #252 (closed patch: fixed)

Opened 11 months ago

Last modified 10 months ago

Language selection does not persist

Reported by: misja Assigned to:
Priority: high Milestone: 0.9.1
Component: i18n Version: 0.9.0
Severity: critical Keywords: patch, review, helpwanted
Cc: ewout, renato, dramirez, rho Patch Included: 1
Review Stage: accepted

Description

Changing the preferred language via account settings does not get saved.

Attachments

include_order.diff (2.0 kB) - added by renato on 01/14/08 04:43:51.
reset_user.diff (0.6 kB) - added by renato on 01/14/08 05:52:03.
translation.diff (1.8 kB) - added by ewout on 01/17/08 20:49:43.
Yet another try…
translation2.diff (1.9 kB) - added by renato on 01/19/08 03:09:01.
same as ewout's patch, but including rho's suggestion

Change History

01/14/08 03:18:06 changed by renato

Looking at the user_flags table, it does get saved, but keeps displaying everything in the wrong language.

01/14/08 04:43:51 changed by renato

  • attachment include_order.diff added.

01/14/08 04:46:17 changed by renato

  • type changed from defect to patch.

This seems to be caused by wrong order in loading libraries at includes.php. See comment in the forum.

01/14/08 05:52:03 changed by renato

  • attachment reset_user.diff added.

01/14/08 19:25:09 changed by renato

  • keywords set to patch, review, helpwanted.
  • priority changed from normal to high.
  • severity changed from major to critical.

In includes.php, the gettext library (mod/gettext/lib.php) is included before setup.php, where the global $USER is declared (and filled up), so that when the gettext library tries to use $USER->ident to check if the user is logged in to get his locale, it's never get correctly at this point. The order change was committed to repo in r1461, without aparent reason (to me).

Reverting that change led to a blank page, that I tracked to the following code in setup.php (near line 414):

if (!isset($PAGE->menu )) { $PAGE->menu = array();}
if (!isset($PAGE->menu_sub )) { $PAGE->menu_sub = array();}
if (!isset($PAGE->menu_top )) { $PAGE->menu_top = array();}
if (!isset($PAGE->menu_bottom)) { $PAGE->menu_bottom = array();}

It's very strange that that should cause problems (anybody knows why??), but commenting that out makes everything work! And also solves (partially!) this bug. See patch include_order.diff.

The /_userdetails page, though, was still untranslated (or better, translated to defaultlocale).

Investigating a bit, I found that mod/users/index.php calls require_login(), which calls cookied_login() (those two in elgglib), and this resets the $USER structure ignoring other variables already set (as locale).

I don't know if cookied_login() is being used to authenticate people or not, so I wrote instead:

if(!isset($USER))
    $USER = $user;

so that $USER is only reset if it's not there already (quite improbable!). I really don't know the implications of this, I'm adding this patch too (reset_user.diff), but would be glad if someone who knows better the auth pieces could take a look!

01/17/08 20:49:43 changed by ewout

  • attachment translation.diff added.

Yet another try...

01/17/08 22:58:03 changed by renato

  • cc set to ewout, renato.

I tested this patch and it seems to work fine.

(follow-up: ↓ 6 ) 01/18/08 12:23:19 changed by dramirez

  • cc changed from ewout, renato to ewout, renato, dramirez.

If I don't remember bad the reason because I changed the include order from the gettext library is because the i18n messages for logging.

If we remove the gettext calls from setup.php (only related to the logging functions) or move that function to another place I think the problem could be solved.

I will give it a try and let you know.

(in reply to: ↑ 5 ; follow-up: ↓ 7 ) 01/18/08 13:26:38 changed by ewout

Replying to dramirez:

If we remove the gettext calls from setup.php (only related to the logging functions) or move that function to another place I think the problem could be solved. I will give it a try and let you know.

Diego, the last patch, translation.diff seems to solve the problem. Do you understand why? It simply moves the gettext include after the setup.php include (since a lot of global variables like $CFG are used in gettext).

(in reply to: ↑ 6 ; follow-up: ↓ 8 ) 01/18/08 15:32:15 changed by rho

  • cc changed from ewout, renato, dramirez to ewout, renato, dramirez, rho.

Replying to ewout:

Diego, the last patch, translation.diff seems to solve the problem. Do you understand why? It simply moves the gettext include after the setup.php include (since a lot of global variables like $CFG are used in gettext).

That's because gettext use $USER->locale value, and that it's overrided when $USER = $user;

But, cookied_login() is called when the session has expired. But $USER is a reference to $_SESSIONUSER?, before cookied_login() will be setup by guest_user(). With if (!isset($USER)) $USER = $user;

User will be logged but will be mantain $USER as guest, right? I didn't take a closer look.

Another solution, without moving gettext that could be break something on setup.php, could be make a wrapper function like:

function session_user_from_object($user) { global $USER; $user = (array)$user; foreach ($user as $attr => $val) { $USER->$attr = $val; } }

This will update session user attributes without override others like $USER->locale

Needs testing ;)

01/19/08 03:09:01 changed by renato

  • attachment translation2.diff added.

same as ewout's patch, but including rho's suggestion

(in reply to: ↑ 7 ; follow-up: ↓ 10 ) 01/19/08 03:15:01 changed by renato

Replying to rho:

Another solution, without moving gettext that could be break something on setup.php, could be make a wrapper function like:

This is not going to work, because $USER will never have the correct locale with gettext being loaded before setup.php. cookied_login interferes only when require_login() is called. Your solution to that functions is much better, though, I created a new patch using it instead.

I made a quick test here and it still works fine.

01/21/08 16:25:28 changed by ewout

  • haspatch set to 1.

(in reply to: ↑ 8 ) 01/24/08 08:51:24 changed by misja

  • review_stage set to unreviewed.

Replying to renato:

I made a quick test here and it still works fine.

The latest patch works for me.

01/24/08 13:46:58 changed by ewout

  • status changed from new to closed.
  • resolution set to fixed.
  • review_stage changed from unreviewed to accepted.

commited Renatos last patch in r1527.

02/06/08 14:00:07 changed by ewout

  • status changed from closed to reopened.
  • resolution deleted.

In addition, as suggested by Renato earlier, this

//if (!isset($PAGE->menu       )) { $PAGE->menu        = array();}                                                                                           
//if (!isset($PAGE->menu_sub   )) { $PAGE->menu_sub    = array();}                                                                                           
//if (!isset($PAGE->menu_top   )) { $PAGE->menu_top    = array();}                                                                                           
//if (!isset($PAGE->menu_bottom)) { $PAGE->menu_bottom = array();}                                                                                           

in setup.php, seems to be necessary. Does that make sense to anybody?

02/07/08 04:10:28 changed by dramirez

  • status changed from reopened to closed.
  • resolution set to fixed.

I removed the gettext calls from setup.php it must to fix this problem