User tests: Successful: Unsuccessful:
As you have a language cookie for a now unpublished language you should be routed to the default language
an invalid redirection occours
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()
@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.
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...
... 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!
I like the set and get methods for the cookie. That bugged me already when I read the code last time
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
@Bakual
there is (questionable...) code outside of PlgSystemLanguageFilter where the cookie is read:
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
Labels |
Added:
?
|
... for the time being I accepted both your suggestions and:
looks fine now.
@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...
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...
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.
... 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...
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.
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!!
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...
Status | New | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2015-05-29 09:54:07 |
Closed_By | ⇒ | Bakual |
Merged into staging, Thanks!
@infograf768, @dgt41: please, have a look at this...