Please see #6904

Evoking @infograf768...

Utterly strange: smz@327cbfa passed Travis but it had mismatched parenthesis...

Good find! $this->lang_codes is much better then checking again the db.
BUT some errors...
1. first error
We had a good reason to use default_lang if no user language before checking for existence of the language and its content language, and if a problem with default_lang, loading current_lang which we are sure it exists. This is because $default_lang IS the user default site language if not set differently in the user profile.
This patch took off the first check:

if (empty($lang_code))
            {
                $lang_code = $this->default_lang;
            }

1. There is another mistake: the PR does not check anymore if the site language itself for that lang_code is enabled (Extensions Manager). We do still need:
   if (!array_key_exists($lang_code, MultilangstatusHelper::getSitelangs())

2. New: I also found out that we need to check if the site language concerned exists in the SITE/language/ folder. To test, manually change the name or delete the site language for one of the site languages concerned when logging. (We may need to add that check in other parts of languagefilter as well as mod_languages).
   Therefore this is what I propose for that part:

        if ($this->app->isSite() && $this->params->get('automatic_change', 1))
        {
            $assoc = JLanguageAssociations::isEnabled();
            $lang_code = $user['language'];

            // The user language is not set. Therefore it is the default site language.
            if (empty($lang_code))
            {
                $lang_code = $this->default_lang;
            }

            // The language has been deleted/disabled or the related content language does not exist/has been unpublished
            // or the related home page does not exist/has been unpublished
            if (!JFolder::exists(JPATH_SITE . '/language/' . $lang_code)
                || !array_key_exists($lang_code, MultilangstatusHelper::getSitelangs())
                || !array_key_exists($lang_code, $this->lang_codes)
                || !array_key_exists($lang_code, MultilangstatusHelper::getHomepages()))
            {
                $lang_code = $this->current_lang;
            }

            // Try to get association from the current active menu item
[...]

Hi, Jean Marie!

This patch took off the first check:

true: my concern was not to negate a possibly already performed check of the browser language. We'll probably need to introduce a new temp variable... I'll look into that.

Concerning if (!array_key_exists($lang_code, MultilangstatusHelper::getSitelangs()) I don't think this is needed at all as we've already performed that check when we've set up $this->lang_codes in the class construct (line 73)

New: I also found out that we need to check if the site language concerned exists in the SITE/language/ folder.

Isn't this being a bit overprotective? Wouldn't that be possible only if a site admin had deleted that folder? And in case this is really needed/advisable, wouldn't it be better to perform that test beforehand in JLanguageHelper::getLanguages()? In any case and to be on the safer sides of things I'll follow your advice and introduce it.

Thanks!

JM, to be honest I think using $this->current_lang in case there is a problem with the user lang is correct:

We set up $this->current_lang in parseRule where it has already been checked against valid values and it will contain the browser detected value in case this is the source of our "preferred" language.

Here, in onUserLogin(), we've surely been through that and if we set here $lang_code to $this->default_lang we negate the benefit of browser detection already performed by parseRule.

another possible scenario (without browser detection involved):

• default language is jp-JP
• user defined language is en-AU (which has been unpublished)
• user has gone to the front-end and switched (language switcher) to en-GB, which is available
• user log in
• if we set $lang_code to $this->default_lang instead of $this->current_lang user will be redirected to the jp-JP UI

btw, if someone else is interested in the code review of the language filter I'm working on (I already submitted that to JM...), it is visible @ https://github.com/smz/joomla-cms/tree/LanguageFilter and of course I accept advices/criticism/PRs

I don't think it is ready yet to be proposed as a PR here. Who knows, maybe for 3.5...

Concerning if (!array_key_exists($lang_code, MultilangstatusHelper::getSitelangs()) I don't think this is needed at all as we've already performed that check when we've set up $this->lang_codes in the class construct (line 73)

You are right. We can take off that check.

But I insist on using

 // The user language is not set. Therefore it is the default site language.
            if (empty($lang_code))
            {
                $lang_code = $this->default_lang;
            }

as this corresponds to an admin parameter: let or not the frontend user choose his/her default site language or, when letting the choice, user choosing the default language

Changing this behavior would not be B/C.

As for browser detection, it is separated from user login.

JM, I obeyed to your request, but, respectfully, I'm still unconvinced:

Assuming that:

• site has two languages: en-GB (default), fr-FR
• users are not allowed to select their front-end language
• Browser detection is off (or user has an en-GB browser)

This will be the behavior:

I sincerely think my version is more respectful of user's wishes and if this is not what the behavior used to be, well, from my point of view I think we are fixing a bug, not breaking B/C.

Not allowing users to select language in their profile, should not mean to force the default language down their throat...

Please do not delete the comment:

            // The language has been deleted/disabled or the related content language does not exist/has been unpublished
            // or the related home page does not exist/has been unpublished

Fixed!

You can take off
+ // This is deliberate: see PR #7130

@smz Something that must not be forgotten: if someone wants the current language to always be loaded when logging, then it is just a matter of setting "Automatic Language Change" to No....

@infograf768
Comment removed

@smz Something that must not be forgotten: if someone wants the current language to always be loaded when logging, then it is just a matter of setting "Automatic Language Change" to No....

Exactly! In that block of code we are only dealing with the case when "Automatic Language Change" is permitted, and that's why (unless there is a technical reason I'm missing) I'm against your interpretation that the default behavior should be using the default language. In my opinion the default behavior should be "do nothing" and hence use $this->current_lang, which is exactly the thing we do in the following check (when the required requested language is unavailable).

BTW, but this another story, I find that the string describing the "Automatic Language Change" option is misleading. It says:

This option will automatically change the content language used in the Frontend when a user site language is changed.

IMHO it should say something like:

This option will allow the automatic change of the content language used in the Frontend when a user logs in (based on his/her preferences/profile).

... or something similar...

@smz

I'm against your interpretation that the default behavior should be using the default language.
To me, that is what the default language should be used for. What else is the use of a default language?

@roland-d

The code, as it is, implements @infograf768's and your interpretation. Apparently there is a majority (of very much esteemed joomlers) that thinks this is the way it has to be and thus I'll accept the fact that my interpretation is somehow illogical. Point taken and no problems for me.

... but you can't convince me!

Once again and for the last time:

There is a very nice Japanese site about, let's say Manga, to which I want to participate. Unhappily I can't read nor write Japanese, but, luckily for me, the site is also available in Italian. The site administrator has even been so kind to allow registered users to set-up their preferred language. Nice! I go to that site and in order to register I switch the site language to Italian (otherwise I can't understand a word... not even where the login fields are). Wow, now I can read! I register and then I log in with the intent to then modify my profile and set up Italian as my default. WTF... I can't read anything again.... Ufffff.... back to the language switcher in order to set-up my preferred language...

Ah... and then why if my preferred language (Italian) is deleted and I then switch to French (which I can understand), I log in and I'm kept in French?

@smz : concerning your example with the Japanese/Italian site.

Nice! I go to that site and in order to register I switch the site language to Italian (otherwise I can't understand a word... not even where the login fields are). Wow, now I can read! I register and then I log in with the intent to then modify my profile and set up Italian as my default.

1. If Automatic change is set to yes, the admin of the site will have given (hopefully) the possibility for a newly registered user to define when registering his/her preferred site language (Italian or Japanese) and therefore, after logging, the user will stay in the Italian interface... If this possibility is not given, it means the admin of the site has a reason to want anyone to use the default site language when logging.
2. If Automatic Change is set to NO, then the user will remain in the Italian interface.

Note, concerning the tip:

This option will automatically change the content language used in the Frontend when a user site language is changed.

This is also true. To test, just login and then change in your profile the default site language: you will be redirected on save to the said language. I agree the tip could be more explanatory, but I am afraid some people will not like it as they always insist that a tip is not a doc...

As it is, this PR is now OK for me. @roland-d rtc for me.

@test

PR works as intended.

RTC Thanks

As this is just a code simplification, I guess it can go in 3.4.2

Merged with 28c4327

Thanks to all!