? Success

User tests: Successful: Unsuccessful:

avatar smz
smz
28 May 2015

Steps to reproduce the issue

  • have a site with at least three content languages published (eg. en-GB, fr-FR, it-IT)
  • in Language filter have the cookie lifetime set to one year
  • in front-end switch to one of the two non-default languages (e.g., it-IT)
  • exit the browser
  • unpublish the content language to which you had previously switched (e.g. it-IT)
  • visit the naked domain (i.e: no language code specified)

Expected result

As you have a language cookie for a now unpublished language you should be routed to the default language

Actual result

an invalid redirection occours
cookie issue

Additional comments

Apply this patch and the behavior will be as expected

I took the occasion to factor-in some code and create two new public private methods: setLanguageCookie() and getLanguageCookie()

avatar smz smz - open - 28 May 2015
avatar smz
smz - comment - 28 May 2015

@infograf768, @dgt41: please, have a look at this...

avatar smz smz - change - 28 May 2015
The description was changed
avatar infograf768
infograf768 - comment - 28 May 2015

@test
This works for the naked domain, not for any other url, for which I get a 404, before and after patch, (in the default site language. Normal behaviour).

imho it is no use to check when setting the language cookie as it can't be set if the content language does not exist.
So would only be of use for getting it.

avatar smz
smz - comment - 28 May 2015

@infograf768

This works for the naked domain, not for any other url, for which I get a 404, before and after patch, (in the default site language. Normal behaviour).

Yes, normal and expected behavior: it would be wrong to serve any other URL (think of search engines: if you unpublished a language for your site you want all the URLs associated to that language expunged from search engines...)

imho it is no use to check when setting the language cookie as it can't be set if the content language does not exist.

You're right it is not worth to do that check as long as the method is called only by inside this class (where we have control of what we do), but as it is a public method (and I can imagine a reason or two for this being useful), I thought it would be safer to check for that...

avatar smz
smz - comment - 28 May 2015

... but as we actually do that check when we get the cookie, it wouldn't do any harm even if we set an invalid cookie... so you're right: I'll get rid of that check!

avatar Bakual
Bakual - comment - 28 May 2015

I like the set and get methods for the cookie. That bugged me already when I read the code last time :+1:
However I wouldn't make the methods public. They should be private as nobody in their right mind should call a method from a plugin class in their code. It's not library code.

Didn't test the fix itself :smile:

avatar smz
smz - comment - 28 May 2015

@Bakual
there is (questionable...) code outside of PlgSystemLanguageFilter where the cookie is read:

  • \libraries\cms\helper\helper.php
  • \libraries\cms\helper\content.php

My idea is that if that code must be maintained we could use the getLanguageCookie() method in it.

But you're right: nobody in their right mind should call a method from a plugin class, so another possibility could be (but I've not explored the feasibility of it) to move this new methods to the JLanguage or JLanguageHelper classes

avatar joomla-cms-bot joomla-cms-bot - change - 28 May 2015
Labels Added: ?
avatar smz
smz - comment - 28 May 2015

... for the time being I accepted both your suggestions and:

  • removed the pointless check
  • made the new methods private
avatar smz smz - change - 28 May 2015
The description was changed
avatar infograf768
infograf768 - comment - 28 May 2015

looks fine now.

avatar smz
smz - comment - 28 May 2015

@infograf768
thanks!

Now, there is a bit more: I think I can prove that if the language cookie is set for the session only (i.e. not year long) it is actually useless to set it.

Can I go for it or is it better to have this merged and then face this aspect?

Actually, if this goes on a "fast track and gets merged, I think I have more optimization bullets in my gun for the language filter...

avatar smz
smz - comment - 28 May 2015

No: removing the language cookie for the session imply modifying the code for those two libraries I cited above (JHelperContent::getCurrentLanguage() and JHelper::getCurrentLanguage()), so it is definitely better to have this merged asap, if everybody agrees...

avatar Bakual
Bakual - comment - 28 May 2015

Thanks for making the methods private.
The other uses in the helper files for sure look wrong. In an ideal world, there should be no checks for this cookie outside the plugin.

avatar smz
smz - comment - 28 May 2015

... In an ideal world, there should be no checks for this cookie outside the plugin.

I totally agree! But a globally accessible method for getting the current language is indeed useful and another one would be a method to know if the current page is a real, true home page.

I'm considering adding those methods and I'd really like to get suggestions about to which class they should belong...

avatar Bakual
Bakual - comment - 28 May 2015

Actually, I think there is no need for a method to get the current language. You can use JFactory::getLanguage()->getTag() to get the currently active language.

As for the homepage, you can use JMenu and it's getDefault() method to retrieve the default menuitem for a given language.

avatar smz
smz - comment - 28 May 2015

Actually, I think there is no need for a method to get the current language. You can use JFactory::getLanguage()->getTag() to get the currently active language.

Ah, yes, of course you're right!! :flushed:
Maybe we should correct those two (apparently botched) methods to just be proxies to that...

As for the homepage, you can use JMenu and it's getDefault() method to retrieve the default menuitem for a given language.

I don't think I can agree on that: by that test every page for which a "better fit" (a nearer menu item) is not available is considered a "Home page" and that's the reason why modules published on the home page are often displayed in unforeseeable situations. Here too, in the language filter, we have to make further checks to see if the current page is the real home page. NoNumber's Advanced Module Manager has specific code to cope with this and have modules published only in the real home page...

avatar Bakual Bakual - close - 29 May 2015
avatar Bakual Bakual - change - 29 May 2015
Status New Closed
Closed_Date 0000-00-00 00:00:00 2015-05-29 09:54:07
Closed_By Bakual
avatar Bakual
Bakual - comment - 29 May 2015

Merged into staging, Thanks!

Add a Comment

Login with GitHub to post a comment