Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for #6904 --> Fixed the check on published content languages. #7130

Closed
wants to merge 7 commits into from

Conversation

smz
Copy link
Contributor

@smz smz commented Jun 6, 2015

There is no need to go to the database: we already have $this->lang_codes setup to perform this check...

This also remove the newly introduced protected $db = null, so that we do not create B/C legacy.

@smz
Copy link
Contributor Author

smz commented Jun 6, 2015

Please see #6904

Evoking @infograf768... 😄

This is probably OK and in any case it will do no harm...
@smz
Copy link
Contributor Author

smz commented Jun 6, 2015

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


// The user 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 (!array_key_exists($lang_code, MultilangstatusHelper::getSitelangs())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@infograf768 is there a need to keep this line or is it OK to be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've think over this for quite a long time as I'm currently reviewing the whole language filter and in the future I'll probably propose a code clean-up (but this is another story...):

Yes, I think this is a correct check as it could be the case that a language is installed and published, but the site admin had unpublished the home menu for it. This is probably a malpractice but I think there maybe be scenarios where this can be legit.

Only thing is that maybe we can perform this check beforehand, in the construct, where we set up $this->lang_codes and $this->sefs...

I have to think over that anyway, so I think it is correct to check that here, at least for the time being..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, c*ap!

I was talking about: !array_key_exists($lang_code, MultilangstatusHelper::getHomepages()) ... while you probably were talking about !array_key_exists($lang_code, MultilangstatusHelper::getSitelangs())

sorry...

@infograf768
Copy link
Member

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
[...]

@smz
Copy link
Contributor Author

smz commented Jun 6, 2015

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!

@smz
Copy link
Contributor Author

smz commented Jun 6, 2015

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.

per @infograf768 request, added check:
!JFolder::exists(JPATH_SITE . '/language/' . $lang_code)
@smz
Copy link
Contributor Author

smz commented Jun 6, 2015

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

@smz
Copy link
Contributor Author

smz commented Jun 6, 2015

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... 😄

@zero-24 zero-24 changed the title Fix for #6904 Fix for #6904 --> Fixed the check on published content languages. Jun 6, 2015
@infograf768
Copy link
Member

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

screen shot 2015-06-07 at 07 45 25


screen shot 2015-06-07 at 07 46 46


screen shot 2015-06-07 at 07 50 39

Changing this behavior would not be B/C.

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

@smz
Copy link
Contributor Author

smz commented Jun 7, 2015

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:

Step My code Your code
user visit site without cookie English English
user switches language to French French French
user logs in French English

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...

@infograf768
Copy link
Member

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

@smz
Copy link
Contributor Author

smz commented Jun 8, 2015

Fixed!

smz added a commit to smz/joomla-cms that referenced this pull request Jun 8, 2015
+ some minor optimizations
@infograf768
Copy link
Member

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....

@smz
Copy link
Contributor Author

smz commented Jun 9, 2015

@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...

@roland-d
Copy link
Contributor

roland-d commented Jun 9, 2015

@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?

@smz
Copy link
Contributor Author

smz commented Jun 9, 2015

@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?

@infograf768
Copy link
Member

@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.

@jwaisner
Copy link
Member

@test

PR works as intended.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/7130.

@zero-24
Copy link
Contributor

zero-24 commented Jun 12, 2015

RTC Thanks 😄


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/7130.

@zero-24 zero-24 added the RTC This Pull Request is Ready To Commit label Jun 12, 2015
@infograf768
Copy link
Member

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

@Bakual Bakual added this to the Joomla! 3.4.2 milestone Jun 16, 2015
wilsonge added a commit that referenced this pull request Jun 16, 2015
@wilsonge
Copy link
Contributor

Merged with 28c4327

@wilsonge wilsonge closed this Jun 16, 2015
@smz
Copy link
Contributor Author

smz commented Jun 16, 2015

Thanks to all!

johanjanssens pushed a commit to joomlatools/joomla-fork that referenced this pull request Jun 19, 2015
@smz smz deleted the RRF_PlgSystemLanguageFilter branch July 4, 2015 03:00
@zero-24 zero-24 removed the RTC This Pull Request is Ready To Commit label Oct 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants