SEF on: Switcher broken => Switching to another home page impossible

As I have merged #5135, this will anyway need update

Its updated. I will have a look at the rest later.

Anyway, this would not be B/C at all. Also we would have other places in code where the prefix is used and not the language tag. Not for this series imho.

Right now, it depends on your definition of BC. Because it still parses the old URLs and it also still processes the old URLs, since there are to many extensions out there, that are using the current Joomla style of generating these URLs. If you can live with the change for the current non-SEF URLs, then this is still BC.

As long as we propose in Global Configuration SEF as an option, this would not be B/C.

As I said earlier: The old URLs still work. We would only be introducing new URLs for the future.

If existing URLs and features continue to work as they are today, there's no need for a config option IMO.

With these latest changes, this would be B/C. These changes require #5264 to be applied, too.

Besides these changes, I just wanted to make you aware of it that we are not using the language parameter of any article in our system right now in the routing... Actually, I searched the whole system and out of 35 files, we are only using the language parameter in 3 of those and there are maybe 10 files where we are not even using the category ID, thus making the resulting URL complete garbage. Will write a PR for that...

Test: not good here
I first applied #5264

SEF on:
Switching between homes does not work (When url lang code is set to off for default site lang)
Switching between associations => 404
SEF off:
we don't even get the site url:
http://localhost:8888/?lang=en-GB

Please check this again. I invested several hours into this now to work mostly through it and this should be good now. I would call this merge-ready right now. Again, as I wrote earlier, this requires #5264 to work.

@weeblr-dev Please have a look over this, too. Would this still work properly with your system?

Simple testing with sh404SEF didn't show any issue with this patch, together with #5264

I had to fix some logical errors in here. I set the language in the URL correctly in the preprocess step now in the query, then the component router can find the right Itemid based on the query (including the language) and then the plugin can handle the URL processing in the normal buildRule step, cleaning up after itself in the postprocess step.

I also merged in the changes from #5264 so that this PR can be tested. When #5264 is merged, this PR will get an update to show the files that only affect this feature and nothing else.

I had a problem with too many redirects (page didnt load) but than I updated all Joomla files to newest dev version and after applying patch everything seems to be ok.

Yes, please make sure that you test this with the latest version from staging. There have been several changes in staging that are required for this to work, so you can't simply copy over the plugin file and be done with it. You need a current copy of staging for this.

@test OK

On the latest staging, no change on generated Urls with the applied patch.

/j3/en/
/j3/en/home-en-gb/9-category-en-gb/2-article-en-gb.html
/j3/en/home-en-gb/9-category-en-gb.html

/j3/es/
/j3/es/home-es-es/11-categoría-es-es/4-artículo-es-es.html
/j3/es/home-es-es/11-categoría-es-es.html

/j3/ar/
/j3/ar/الصفحة-الرئيسية/8-المجموعة-ar-aa/1-إدراج-رابط-مقال-ar-aa.html
/j3/ar/الصفحة-الرئيسية/8-المجموعة-ar-aa.html

/j3/fa/
/j3/fa/خانه/10-مجموعه-fa-ir/3-مطالب-fa-ir.html
/j3/fa/خانه/10-مجموعه-fa-ir.html

/j3/zh/
/j3/zh/首页/12-分类-zh-cn/5-文章-zh-cn.html
/j3/zh/首页/12-分类-zh-cn.html

@Hackwar

I just have tested this PR. URLs in SEF mode are correct for my test web site, which has same structure and content as my web site (see my GitHub profile).

But there is one difference in behavior compared to before the patch.

Before patch:

When I add to a URL e.g. for German a language parameter for another language, e.g. "http://localhost/joomla2/de/music.html?lang=en" and enter this into the address field of the broswer, then I get the English content without any redirect, i.e. the URL in the browser stays the same "http://localhost/joomla2/de/music.html?lang=en", but the content shown is equal to what i see when navigating to "http://localhost/joomla2/en/music.html".

After patch:

When I add to a URL e.g. for German a language parameter for another language, e.g. "http://localhost/joomla2/de/music.html?lang=en" and enter this into the address field of the broswer, then the lang parameter is ignored and I see the German content.

If this change is the desired behaviour, then my test can be considered as successful, but if not, then it failed.

Please let me know how I shall update my test result.

@infograf768 would have to answer that. From a URL-theory standpoint I would say that it is the correct behavior.

@Hackwar @infograf768 Yes, I would say the post-patch behavior is more correct than then pre-patch, because in post-patch case language parts in URL (folder and parameter) both fit to the language of the displayed content, but in pre-patch case not. But I did not want to decide this alone and so left my test result open until I get some confirmation.

P.S.: Maybe it has an impact on the test that I have same aliasses for everything in all languages, have menu item assiciations for everything and menu items for every single article, so I don't use category or article assicoations, and I do not remove the default language code from the url, so my URLs are like e.g. de/music/recordings.html, en/music/recordings.html and ru/music/recordings.html.

@Hackwar Maybe I should set my test result to success and let the one who merges this PR decide if to reset my test result based on the comments above and a hint on these comments in the comment to the test result, so we are ready to commit soon because having 2 successful tests? If so, let me know here pls.

That sounds right.

@test Tested with success. There is a slight change in behavior regarding the lang parameter, see my comments and discussion with @Hackwar above.

Concerning @richard67 weird urls mixing sef and non sef, I would say the right behavior is after patch.
I need more time to test this patch though.

@Hackwar
One question first as this patch also modifies the Core components
Is this B/C with eventual third party components using the native multilingual funtionnality?

This is fully B/C. The change that is introduced here is, that the core components don't have to do a lookup for the SEF language part, but simply hand in the ISO language code. The changed plugin then validates that ISO code in the preprocess step and in case it is already the SEF code (for example for third party extensions) it replaces that SEF code with the corresponding ISO code.

When SEF URLs are enabled, the actual processing is done and based on the ISO code the SEF portion of the URL is added. Then the component routing is done and after that, the ISO code is either removed (in case of SEF URLs) or replaced with the SEF language code (in case of non-SEF URLs).

This means that existing components can still do the (unnecessary) extra work of adding the SEF language code as language parameter to the URL. In that case, the plugin replaces that code with the ISO code in the first step.

Now we could construct a situation where the change to the ComponentHelperRoute classes breaks backwards compatibility, but that is so far fetched, that a developer who created such a situation should simply never again be allowed near a computer...

I just saw that JHelperRoute was not updated yet. That class is... It should be improved now.

I'm not a programmer, but I did manage to install the 'Joomla patch tester' :-)
Just tried this patch with a recent J3.4dev nightly build and the latest VirtueMart version, where both the Joomla content and the shop are bilingual. I did not notice any changes in the SEF urls.

When SEF urls are disabled, it seems that it does not catch the associated categories in the other language, so I'm redirected to the home page when I select another language. (Not shure if this is normal with SEF set to 'Off')

Also got an error message when using the Joomla search form. After submitting a search, the error message briefly shows up (see attached screenshot) but thereafter continues to display the results.

I had to delete all pre-existent cookies to avoid getting 404s when displaying categories menu items. Then if worked for a while and I get it again.

I confirm the issue concerning associations with SEF off or ON, for items as well as categories. I may get 404s or weird results like empty categories. It is unstable.

Although I can't display the error for search, it makes sense as $lang_code is not defined.

NOTE: Looks like we now have a new cookie for admin and site: SLnewses. Bizarre, not related to this PR as I get it too on a monolingual site.

I can confirm the error message when searching and I fixed it. But I can not confirm any cookie issues or weird association issues.

Here is some additional information concerning the association issue (I'm not shure if this is useful).

Example for the association problem with Joomla SEF set to "OFF":
An english language link to articles in category blog layout:
http://localhost/myroot/index.php?option=com_content&view=category&layout=blog&id=14&Itemid=111&lang=en
When I click on the flag for the other language (de), the url changes to this:
http://localhost/myroot/index.php?lang=de&Itemid=106
and displays this message in the content area: "There are no articles in this category. If subcategories display on this page, they may contain articles."
...instead of going to the associated german link::
http://localhost/myroot/index.php?option=com_content&view=category&layout=blog&id=9&Itemid=106&lang=de

Hope this helps a little bit.

This last iteration of the PR totally breaks a multilingual site. It's now impossuble to even switch from one language to the other, even after deleting all past cookies.

"The last iteration of the PR totally breaks a multilingual site"
Same here, when SEF urls are disabled. With SEF urls enabled, it still seems to work fine in my case.
I wonder if this issue is related to the problem: #5817

In my case, the issues are also present when SEF is enabled.

@test I have tested again with a clean new installation with no sample data + additional languages installation with staging plus this PR applied, and I can confirm problems reported by others above.

@Hackwar Here a description of the behavior I discovered - I hope it can help you to get an idea what the problem could be:

After installation I just have switched off SEF URLs in Global Configuration.

When I change language with the language switcher module or by entering the URL with the desired language parameter in the browser address field and press enter, it seems it always needs 2 times until the content is displayed in the right language.

E.g. I have current page = http://localhost/joomla3/index.php?lang=de and German menu and featured example article are shown, then I change in browser address bar ?lang=de to ?lang=en and press enter, and the page loads, and the URL in the browser is not changed back to the German one, i.e. it still is the previously entered English one, but still German menu and article are shown. Now I just press enter again in the adress bar and - voila - see the english stuff correctly.

Or if I change URL in address bar ?lang=de to ?lang=en and press enter, and still see German content as described before, and then change ?lang=en to ?lang=ru, I see the english content, I.e. it is always 1 language change behind.

This is indipendent from which browser I use.

Adding associations for the menu items does not change anything on this behaviour.

P.S.: I just have rolled back your PR on the test environment I have used for the test before so it is a current staging, and now all works well, i.e. the problems came definitely from this PR.

@Hackwar I've looked a bit into your code in plugins/system/languagefilter/languagefilter.php and there is one thing which confuses me:

In function preprocessBuildRule, you use "$lang = $uri->getVar('lang', $this->default_lang);"

But $this->default_lang is initialized first time in function parseRule.

And as far as I understand, the order of processing will be preprocessBuildRule and then buildRule:

$router->attachBuildRule(array($this, 'preprocessBuildRule'), JRouter::PROCESS_BEFORE);
$router->attachBuildRule(array($this, 'buildRule'), JRouter::PROCESS_DURING);

Or am I wrong?

This should not be related to the problems discovered here but I do not really understand it.

@richard67 $this->default_lang is initialized first time in the constructor of the language filter.

Another little thing, not causing or solving any problems is: in the constructor if (!$this->default_lang) has been removed in an earlier commit against staging, for it is a reminiscent of the static variables). I'll do that PR against Hannes'repo too, so that piece of code cannot be reintroduced by accident in this PR.

@HermanPeeren I cannot see it being initialized there. Maybe really something has been removed?

@richard67 you are right, I was looking wrong, also for the application of #5531 in Hannes' repo, which has been done.

The default_lang is not anymore initialised in the constructor in https://github.com/Hackwar/joomla-cms/blob/languagerefactor/plugins/system/languagefilter/languagefilter.php ). Before we had:
$this->default_lang = JComponentHelper::getParams('com_languages')->get('site', 'en-GB');
but: if it is initialised in parseRule(), then it is alright too, for the parseRule is always applied first, before the preprocess and build. This looks alright to me.

I changed my last comment on Github, but don't see it immediately being updeted in JIssues tracker.

@HermanPeeren Yes, it always takes a bit time until comments in issue tracker are updated from GitHub.

@Hackwar I have added a bit debug output to the languagefilter.php here:

public function preprocessBuildRule(&$router, &$uri)
{
    $lang = $uri->getVar('lang');
    echo('preprocessBuildRule (1): $uri=' . $uri . "<br />");
    $uri->setVar('lang', $lang);

    if (isset($this->sefs[$lang]))
    {
        echo('preprocessBuildRule (2): $this->sefs[$lang]->lang_code=' . $this->sefs[$lang]->lang_code . "<br />");
        $lang = $this->sefs[$lang]->lang_code;
        $uri->setVar('lang', $lang);
        echo('preprocessBuildRule (3): $uri=' . $uri . "<br />");
    }
}

and here:

public function buildRule(&$router, &$uri)
{
    $lang = $uri->getVar('lang');
    echo('buildRule: $uri=' . $uri . '<br />');
...

And look what I get:

Mind the URL with itemid 103 for the German contentand lang variable set, but in the main menu and the article still the English id 102 is used, and there is no lang variable set.
And is it by purpose that you set the lang variable in the URL to the language code, e.g. en-GB or de-DE (see preprocessBuildRule debug outputs (2) and (3))?

Hi @richard67,
yes, there is still an issue in the code, which I apparently introduced in Hackwar@8d9c08c in order to fix the issue that @infograf768 described before that. I will have to look into that one again.
The order of initialising the variables is right. The parse rule is always called at the beginning of a page call and it should always only be called once. (We had code in Joomla that did this for example in mod_login again for very wrong reasons.) The build rules are executed on the URLs to be build and that is done for each URL.
It is also correct that the URL has the language code in it until right before the end. It is actually very bad for us that we were taking the language code and then replacing that in the ContentHelperRoute classes with the SEF code, since that meant that we would have to parse the URL again to get to the language code again to for example look up the right Itemid. So this change rightly moves this step into the router instead of having this in the HelperRoute classes. The language code is replaced with the SEF code (or unset if in SEF mode) when the postbuildprocess step is done.

@Hackwar Ok, let us know when this PR is ready for re-test.

Ok, I did some changes. Please test again.

@Hackwar I've checked your changes by review and they made sense to me, but when testing again the behavior did not change, i.e. is still as described before when I changed my test result from success to failed.

I also think the behavior didn't change. Applied the patch to the latest J3.4dev nightly build.
Another Example:

I select an english article (which has an associated german article)
http://localhost/myroot/index.php?option=com_content&view=category&layout=blog&id=11&Itemid=108&lang=de

Click the british flag (I would expect that the url doesn't change in this case).
Shows an url with the 'en' language tag, but at the same time a german language message (Es gibt keine Beiträge in dieser Kategorie. Wenn Unterkategorien angezeigt werden, können diese aber Beiträge enthalten.)
http://localhost/myroot/index.php?lang=en&Itemid=113

Click the german flag
Shows an url with the 'de' language tag, but at the same time an english language message (There are no articles in this category. If subcategories display on this page, they may contain articles.)
http://localhost/myroot/index.php?lang=de&Itemid=108

Now when I continue to toggle between the british and the german flags, it continues to display the 'en' url with the german message and vice versa.

I can't reproduce this on my end. How are you testing this? Are you using the latest staging or is this an "older" Joomla (3.3.6 or a version of staging prior to this month) together with the patchtester?

I'm currently using this nightly build (not the staging snapshot) together with the patch tester:
http://developer.joomla.org/cms-packages/
I this wrong? I thought the patch currently does not require previous patches.

The patch does require that the PR #5264 is applied. That PR has been merged on January 9th. So you need to make sure that your copy that you are testing with is from a point in time after that.

Not tested, but a thing I don't understand in the code of your current version of the languagefilter parseRule()-method: when redirecting (lines 338-375) you won't reach the cookie-setting (lines 384-391). Shouldn't only the redirection-url be set and the actual redirection be done after the setting of the cookie?

I should have tested it first before writing this down, but maybe it is relevant; otherwise just ignore it.

Another thing: if a language-sef is found in lines 301-307, but it should be removed (lines 309-314), because it is the default language and we can remove the prefix, then $found = false. So now we are going to redirect (lines 338-375) and first thing that is done is to detect the browser-language... which may not be the default language.

That could also be an explanation of why you cannot reproduce the reported error: maybe your browser language is the same as your default language and maybe this setting is different for other testers.

@Hackwar Regarding test environment: I always used latest staging (latest means just before fetched with a pull) and use TortoiseGit to apply the patch using the diff file from the link in the issue tracker. I do not use the patch tester. The TortiseGit I let use the git.exe which comes with GitHub for Windows. I always use Beyond Compare (best diff tool ever) to briefly check the differences before I use them for test, so I am pretty sure the patch for this PR here was always applied.

I am confused now! I have reverted the patch of this PR in my test environment used for the (unsuccessful) tests before, pulled all latest changes from staging since yesterday, applied the patch with this PR again and voila: all works, both changing language with language filter or by entering URL with "?lang=xx" works.

And when I close broswer and open again and go to the site root URL again, the cookie setting is appplied so I get the language I had before.

=> All fine!

So WTF was wrong when I did the same yesterday? Earth rays? Bad horoscope?

Or was the diff file which I saved the one from my broswer cache? I am almost sure it was not, because I checked the delta with Beyond Compare and was sure what I have seen was what I could see in GitHub.

Or is there some issue with old cookie still being used by the browser?

@Hackwar As long as this is not clear I am not sure how to alter my test result now: "Tested successfully" because it worked today, or "Not tested" because we still do not know why it did not work yesterday?

Maybe I should wait what other testers say after having tested it again?

I am not sure now what to do, please advice.

Ahhh sorry (blush): I had forgotten to apply the patch for this PR again! Forget my comment above please! It still does not work!

Tried again today with latest staging copied over J3.4beta2dev. Still no luck. Language switching works correctly with/without SEF enabled and #5264 only. But #5140 still brakes non-SEF language switching in my case.

I've done some further refactoring. Please test again.

SEF on
Remove URL Language Code on: the module switcher keeps presenting the possible urls (home or associated) without the language code => broken

Remove URL Language Code off: I keep getting 404s can't find category for associations.

For me same as with my previous tests, i.e. with both SEF on and SEF off the article and menu still show in the previous language, and only a second load of the page can make these switch to the new language.

@Hackwar,
Finally I can report a successful test. :-)
After applying the latest staging snapshot including your two commits of today, language switching works correctly with both, SEF URLs enabled/disabled.

Note: Just applying #5140 with the patch tester again using a three day old staging snapshot didn't work (With that I got the same result as Richard reported today)

@Gitjk I used latest staging and diff file from github for this PR, not old stuff and patchtester.

I was finally able to reproduce the problem that it showed the content in one language and the static text in another. I also found a solution for it. Who would have thought that the current code parses the language from the URL several times, where only the first try counts... For those that want to follow along: Look at the constructor of this plugin. Until now, that constructor did all the parsing from the URL and then set whatever it found as the "correct" language. Since JFactory::getLanguage() is called in several plugins before the actual component and modules are run, the language is actually set before onAfterInitialise. Since JLanguage does not have a clear() or reset() method, we can not simply set a new language after that. Instead we have to kill the object in JFactory::$language and then let JFactory fill that with an updated object upon the next call. This sucks.

Please test if this improves the issues that you have.

Hmm, it seems to be better now with the caregory name and the article details and the menu, but the page header still needs a second page load to update from the old to the new language.
For example after changing language from English to Russian:

And then after page reload:

This is true for both SEF URL on and off.

P.S. I took the Russian example because the German page header is the same as the English one ("Home").

P.P.S.: If I add menu item associations (which are missing in the multilingual fresh installation), the problem described with the page header is not there anymore, i.e. all works. But adding article associations (but no menu associations) does not help. I.e. currently it requires menu associations to have the pahe header (h1) in the correct language, after changing language, else (no menu item associations) it is always 1 language change back.

My "successful test" was a faulty one. After applying the latest staging I forgot to re-apply #5140 again. Got fooled by the patch tester, which was still saying "Patch applied" (but actually the patch was overwritten by copying the latest staging over it).

However, after applying the patch again, it basically still works in my case with both SEF URLs enabled/disabled. Just compared the file 'languagefilter.php' in my installation with the one in the current version of the pull request on Github, to make shure I've got the latest version.

Only small issue I still see is, that when SEF URLs are disabled, the first click on a menu item pointing to a category holding german language articles, loads the page showing a long URL:
http://localhost/mysite/index.php?option=com_content&view=category&layout=blog&id=11&Itemid=108&lang=de

Now clicking the british flag loads the english language version of that article with this URL:
http://localhost/mysite/index.php?lang=en&Itemid=113

Now clicking the german flag loads the german language version again, but this time with a short URL:
http://localhost/mysite/index.php?lang=de&Itemid=108

After that, toggling between the German/English category keeps the short URLs.

Same issues as stated above...:

SEF on
Remove URL Language Code on: the module switcher keeps presenting the possible urls (home or associated) without the language code => broken!!!
https://www.dropbox.com/s/6ogfx0vp1giabrf/iscreenoff_record.mp4?dl=0

Remove URL Language Code off: I keep getting 404s can't find category for associations.
reloading the page now solves it.
https://www.dropbox.com/s/5yvk2d2cw55vrd1/iscreen%20record.mp4?dl=0

@Hackwar
I suggest you DO test your code on an existing multilingual site (with associations) updated to staging + your PR before changing again your PR by code review.

@infograf768 I am testing the code on such a site and it works on my side. I wouldn't ask you to test if I hadn't tested it on my side and had it working.

Therefore, I guess we have to find out why it is really broken here (multilingual site working perfectly since 1.6 and updated since) and works on your test site...

Just an idea. @Inforgraf768 why dont you send @hackwar a copy of your
multilingual test site - that way you both know you are testing the code
against exactly the same data

On 2 February 2015 at 08:55, infograf768 notifications@github.com wrote:

Therefore, I guess we have to find out why it is really broken here
(multilingual site working perfectly since 1.6 and updated since) and works
on your test site...

—
Reply to this email directly or view it on GitHub
#5140 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

ok, did further changes and improvements. As I said, it is working fine on my end. If you still encounter issues, please provide me a copy of the site in question or detailed instructions how I can recreate the problem.

Please also notice #5950 and #5952

Still see this issue in the code:

• given:

  • default site language = de-DE,
  • remove default prefix = on,
  • default browser language = en_GB,
  • detect language = on

• when: I want to GET a SEF-URL starting with /de/

• then: in the parseRule-method of the languagefilter-plugin:

  • line 258 $sef='de';
  • line 270 isset($this->sefs[$sef]) == true;
  • line 273 $found = true;
  • line 274 $lang_code = 'de-DE';
  • line 281 $found=false; !!!!!!!!!
  • line 307 !$found
  • line 310 $lang_code = false;
  • line 325 looks for $lang_code in cookie, which is not neccessarily 'de-DE', so could be wrong! and if not there then
  • line 313: $lang_code = 'en-GB' which is wrong... if not found in line 313 then line 321 would give the right 'de-DE' string.

Sorry, no time now to help fix it now, but still wanted to report it.
All in all I think you did a great job untangling this spaghetti, but my feeling after reading this code is that it still needs some refactoring, to make the logic and flow more obvious.

@hackwar
Just add in mod_languages

<?php var_dump($language);?>

after line
https://github.com/joomla/joomla-cms/blob/staging/modules/mod_languages/tmpl/default.php#L33
and test what you get in frontend for 'link'.
SEF on, playing with Remove URL Language Code off and on.

Please contact me on skype. I will send you a dump of db

@HermanPeeren Line 281 is only reached if you enabled to hide the language prefix for the default language and the URL segment fits our default language. In that case it is correct that we did not find the right URL, but should redirect to the right URL without the prefix. However, you might be right that the cookie value is not the one that we want. Will investigate.

According to the comments $found indicates whether the language is found, not that "the right url" is found. In line 281, the language is found, but the url should be redirected. So $found should stay true, but another flag ($redirect or something) should be set. In that way the language is not searched for a second time in line 307 - 325 (with possibly wrong results).

Another side-effect of setting $found to false in line 281 is that the found language-sef is not cut off from the start of the path in line 285 - 289. So, the language slug that is found in line 307 - 325 (which could be wrong) is put in front of the original path, which already had the language-sef as first part in the above example.

Reasoning about the code is especially difficult because of the nested conditionals. An OO solution would be refactoring conditionals to polymorphism, but that would mean that those callbacks (parseRule and buildRule) would be put in a separate class, where for instance a Factory Method or Strategy Pattern would be used to get the correct subclass. http://sourcemaking.com/refactoring/replace-conditional-with-polymorphism

A disadvantage of such a refactoring (and maybe of OO in general) is, that your algorithm is scattered over different classes, which makes it still difficult to keep an overview. I was thinking about using closures instead to get more concise code, using ideas from FP.

As a side-remark: in a project I'm working on I need translated segments in the whole url. I'm working on that as a context sensitive grammar (which I resolve in two steps to context insensitive grammars). In the current Joomla routing the segments are not mutually influencing each other, so that is a simpler case (difficult enough, though); the only context for an url in Joomla now, are the variables of cookie, default browser language and default system language. I'd like to exchange some ideas about that.

@infograf768 was so kind to send me his multi-language test environment and I worked a bit on this. I've solved a lot of issues and also those that @infograf768 described. On my side, switching between languages works both on the homepage as well as on subpages, together with associations both between menu items and content items. Last but not least, removing or adding the default menu item works as well. So after these changes, I ask you to test this again and give me feedback. I hope that I caught everything now...

Did you find the difference between your and @infograf768 setup. It might
help other testers

On 3 February 2015 at 10:45, Hannes Papenberg notifications@github.com
wrote:

@infograf768 https://github.com/infograf768 was so kind to send me his
multi-language test environment and I worked a bit on this. I've solved a
lot of issues and also those that @infograf768
https://github.com/infograf768 described. On my side, switching between
languages works both on the homepage as well as on subpages, together with
associations both between menu items and content items. Last but not least,
removing or adding the default menu item works as well. So after these
changes, I ask you to test this again and give me feedback. I hope that I
caught everything now...

—
Reply to this email directly or view it on GitHub
#5140 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

No, I did not find the difference. I don't see anything that is especially different. However there seem to be a few issues with the cookies, which I solved, which are based on a wrong URL being handed over for the cookie. This should be solved now.

A minor difference in the setup between @infograf768 and @Hackwar, that might not be in the exchanged multilingual test environment, could be the default browser language. To keep that variable the same too you could use a Vagrant box.

Looks MUCH Better now SEF on or out, url language code yes or no :)

Still need to test browser settings vs default site language.

SEF off: some differences in urls
after PR
/index.php?option=com_content&view=article&id=160:activer-le-plugin-syst%C3%A8me-filtre-de-langue&catid=11:joomla-multilangue&lang=fr&Itemid=121

before PR : the name of the category is not added in the slug
/index.php?option=com_content&view=article&id=160:activer-le-plugin-syst%C3%A8me-filtre-de-langue&catid=11&lang=fr&Itemid=121

That is a different issue and is based on JLanguageAssociation, which forces the catslug in, even though we removed it everywhere else. I would not attribute this to this PR.

hannes, as I said, you have above before and after PR. :)

There you go. Should be "fixed".

I've tested again, and things look much better now, but there is still an issue with the button and the password field of the login module (but not wuth the rest of this module ... strange):
Those still are one language change behind, as I described before with my other tests for other elements of the page.
See this screenhsots after switching from English to Russian, with SEF URLs = on:

I feared that a bit. The issue is, that JLanguage is extremely dirty. It is initiated way before the parse routine discovered the right language and thus uses some (to me) unknown mechanism to get the current (not necessarily right) language. That means that for example all system plugins are loaded and their language files are loaded, as well as the core language file. When I then set the language with JLanguage::setLanguage() to a new value, this does not clear the old strings and re-read them. That is basically a bug in JLanguage. Please don't force me to rewrite JLanguage in this PR, too.

BTW: The only reason that it worked prior to this PR is, that the languagefilter plugin in Joomla by default is the first plugin to be called and the discover code was in the constructor of the plugin. That meant that it was called before JLanguage was first called to autoload other language files.

@Hackwar I won't force you to rewrite JLanguage, and maybe it really is like this that with your PR bugs appear now which were there before but for lucky circumstances did not appear. But if none of your PRs for the routing fixes that, and it will go into the release, then nobody will ask whos fault it was when things do not work anymore. So I think I should wait for some comments of the other testers, especially @infograf768, before I change my test result, because now with this strange behavior similar to a random generator or some programmatical dependencies on earth rays and planet constellations I cannot say the test was ok and feel good with that, even if the cause is not this PR. But let me know your opinion in this pls., I don't want to be in the way to come foreward with this. And thanks for your work, from my point of view it is not your fault that you fall into those traps created by the obscure old implementation. I had such stuff often enough at work and know what a shit it can be. But what does not kill us makes us stronger.

@richard67
Just wanted to note that the 'Login Form' switches language correctly upon the first click if I select a different language flag in my case. In case of the button, in my case the default 'Log In" is an image which remains the same in all languages. If I change the default setting in the module to 'Text', the language change becomes visible and also works on the first click.

@Gitjk In my case not, and the button is not an image and changes language, too. I use a clean new installation of staging + this PR, with additional languages de-DE and ru-RU added at installation and with the default test data required for multilanguage. maybe you have another login form, or not test with a language like Russian or Chinese, where the difference is obvious because not all the same "Log in" for the button?

@richard67
Ah, sorry. I was mistaken again. I mixed up the button with the icons. However, after I changed the module configuration and saved it, my button continues to change language correctly. In my case I have the orginal Joomla Login Form (Language "All", Id 16) unpublished and use copies of the module for each language. One is set to (Language "German", Id 56) and the other one is set to (Language "English", Id 57). My two language switcher modules have Ids 20 and 21. So I think in my case the language switcher loads before the Login Form (correct me if the latter is wrong - I'm not a programmmer).

@Gitjk I think the reason that it works for you is simply that you have 1 separate login module for each language, while I use the original module for for language "All". I don't think the module ID order has any influence on it.

I just unpublished my two language specific login modules and published the orginal one with language "All". Still changes language and button correctly. (I'm currently testing with a copy of my J25 live site updated to latest J3 staging plus #5140. It also works if I add #5950 and #5952 to that)

#5963 implements the necessary changes to JLanguage to work.

I can't repoduce here a mod_login set to ALL languages which would not be displayed in the correct language interface:

But, I have to say that languagefilter plugin is loading first here on my test site

See #5963 (comment) for further tests

@test Together with #5963, the issues I had here with previous tests are solved.

So do we have 2 successfull tests with that?

"So do we have 2 successfull tests with that?"
You can count my last test (from 2 days ago) as "successful". I didn't find any remaining problem in my case.

I implemented the code like @mbabker proposed. Does this need new tests then? Considering #5996 would it still be possible to add this to 3.4.0?

Yes, it needs new tests.

Ok, I alter my test result and will test later again.

Tested with latest staging and #5140 +#5950 +#5963

System - Language Filter plugin settings:
Language Selection for new Visitors: Site Language
Automatic Language Change: No
Item Associations: Yes
Remove URL Language Code: No

No problems when SEF URLs are disabled.

SEF URLs enabled
Toggling back and forth between german/english VirtueMart categories (clicking the flags) initially works, but upon the second change it goes back to the homepage. With Joomla articles, switching language by clicking another flag always redirects to the homepage. Looks like associations are broken again.

Note that applying #5250 now breaks language switching and apparently other things.
http://localhost/mysite/index.php?option=com_content&view=category&id=10&Itemid=107&lang=de (first call)
http://localhost/mysite/index.php?lang=de&Itemid=107 (second call)
Switching to English shows
http://localhost/mysite/index.php?lang=de&Itemid=108&option=com_content
instead of: http://localhost/mysite/index.php?lang=en&Itemid=112

@Gitjk @Hackwar As far as I understood, the retest of this PR here should be done without #5963 because it has been closed in favour of #6016, which has no functional impact on tests and so is not required, or am I wrong?

I am just testing with latest staging and latest version of this PR here but without #5963, and I have same issues with the page header as mentioned in my comment from Feb. 1st above (http://issues.joomla.org/tracker/joomla-cms/5140#event-62182).

Applying #6016 makes no difference in the results in my case. But I just noticed that applying latest #5140 now breaks my two (one for each language) "Homepage" links which are of menu item type 'Menu type alias' in a top menu. The two 'original' "Home" links (with language flags) are in a side menu.
Clicking the "Homepage" in the top menu triggers a 404 while in English (default site language is German), both with SEF On/Off. Clicking the corresponding link while in German, works with SEF On, but with SEF Off adds an '...&option=com_content...' into the URL and displays a list of content categories along with the number of articles in those categories.

Correct #6016 is just deprecating a method. Everything in this PR should be self-contained again

@test
Sorry to say, but associations are now broken again => 404 (SEF on) for categories.

@Hackwar
on my db, test using the
Schritt für Schritt zur Mehrsprachigkeit menu item and then click on another language flag in the switcher.

There is still a JLanguage::setLanguage() call in the onUserLogin event (Is that even necessary? I know, probably unrelated to this PR in general, but saw it looking for associations related stuff).

Which is happening first, the plugin's onAfterDispatch event or the parseRule stuff? I haven't the slightest idea how that stuff even works, but my guess just from a quick glance through the patched plugin is that the associations are being processed before the JLanguage switch in parseRule.

@mbabker There is also still a SetLanguage() call in the constructor of JLanguage itself, see my comment on your meanwhile merged PR for depreceating the function.

@mbabker Ah, ok, I see it is removed with #6018 . Fine.

New test, after #6016 and #6018 in.
No more issues with associations SEF on.

Needs tests for other aspects.

FYI:
onUserLogin switches language (and home page) on the fly depending on the registered user preferred site language.

I think that last test truly baffles me. The only practical difference with those two patches not merged versus them being merged is a call to JLog::add().

@infograf768 Have you reverted #5963 on your testing environment before testing again?

Hmm...
Now when I start over again with a fresh staging copied over my site, I can apply #5140 with the patch tester, but applying #6016 and #6018 both result into a fatal error.

Fatal error: Class 'JLanguage' not found in ...mysite\libraries\cms\application\administrator.php on line 278
Call Stack

Time Memory Function Location

1 0.0000 143752 {main}( ) ..\index.php:0
2 1.0920 2314248 JApplicationCms->execute( ) ..\index.php:42
3 1.0920 2314336 JApplicationAdministrator->doExecute( ) ..\cms.php:251
4 1.0920 2314736 JApplicationAdministrator->initialiseApp( ) ..\administrator.php:116

Guys when testing with staging you DO NOT need to apply 6016 and 6018. As they are already merged into staging as unrelated improvements. Please only test THIS PR

@wilsonge
Thanks for your reminder. :-)

Applying #5140 only to latest staging currently works with both, SEF On/Off in my case. Except one thing I've mentioned earlier today already:

But I just noticed that applying latest #5140 now breaks my two (one for each language) "Homepage" links which are of menu item type 'Menu type alias' in a top menu. The two 'original' "Home" links (with language flags) are in a side menu.
Clicking the "Homepage" in the top menu triggers a 404 while in English (default site language is German), both with SEF On/Off. Clicking the corresponding link while in German, works with SEF On, but with SEF Off adds an '...&option=com_content...' into the URL and displays a list of content categories along with the number of articles in those categories.

When I enable Joomla debug while trying to reproduce the 404, it tells me this:
404 - Component not found.
Call stack

Function Location

1 JApplicationCms->execute() ...mysite\index.php:40

2 JApplicationSite->doExecute() ...mysite\libraries\cms\application\cms.php:251
3 JApplicationSite->dispatch() ...mysite\libraries\cms\application\site.php:230

4 JComponentHelper::renderComponent() ...mysite\libraries\cms\application\site.php:191

(When I also enable Joomla language debug, I also get various javascript errors like these ones:
Script: http://localhost/j34avm303/media/jui/js/jquery.js:5195
Script: http://localhost/j34avm303/media/jui/js/jquery.js:4130
But I'm pretty shure that's a different story not related to 5140, because if I remember correctly, I've noticed these kind of javascript errors earlier already - for example when using J3.3.6)

I still have the issue with the page header, see above http://issues.joomla.org/tracker/joomla-cms/5140#event-62182

@richard67

@infograf768 Have you reverted #5963 on your testing environment before testing again?

#5963 was not in my test site, my last test was staging (including #6016 and #6018) + this PR here

I've looked into the issue with the wrong heading from @richard67. To clarify, that is not an issue of the sitename of the language, but the wrong menu item being selected at some point during the parsing of the router.
The issue in this situation is, that the language object is not only cached in JFactory, but also in the application object. So it had to be replaced there, too.

I also added $this->app = JFactory::getApplication(); in the constructor, in order to get around the issue described in #5981. For the moment, I would rather add that nasty bit of code then to have this PR not included at all into 3.4.

@infograf768 onUserLogin does not switch the language on the fly. That event is fired, the new language is set in the cookie and then the user is redirected. The JLanguage object at that point in time is in fact in an inconsistent state. This only works because the user is redirected after the login and thus a completely new request is fired.

Long story short: Please test again.

@test
Tested again. Result: Positive - No issue anymore in my case.

@Hackwar
At my first attempt today I thought that nothing changed in my case since yesterday. But since I do trust your coding skills, I tried to find out if I made a mistake somewhere. Turned out that I had assigned a wrong menu item alias to one of my 'home' links in my top menu. Sorry for creating extra work for you by not detecting this earlier. (It's wrong on my live site since J1.6, but nevertheless works with J3.3.6 or lower)
So, if you made your last change because of my previous test result from yesterday, it might not be necessary anymore.

@Hackwar

This only works because the user is redirected after the login and thus a completely new request is fired.

No p with wording. 'On the fly' just meant for me the switch is done as should as soon as logging.

Was OK for me as I said in
#5140 (comment)

but will test again later as you added some code.

@Gitjk If it worked before, it has to work now, too. So the change was necessary. Regarding the change that I did: This was mainly for @richard67. It seems as if we are retrieving the language object at some points from the application object and that object was not updated. Anyway, it might be worth looking into to refactor all our code to use the language object from the application object instead of JFactory::getLanguage() (where applicable). That should make testing easier...

@Test Success with non-SEF and SEF URLs, with and without menu or item or category associations, with or without remove default language IRL code.
@Hackwar I had other issues, too, which I did not describe above because I thought they are related to the same reason as for the page heading problem, and I was right: With your latest corection, all works.

Now someone with appropriate privileges has to update test result for @Gitjk and reset those of @anibalsanchez and @dgt41, and test count is ok.

Thanks @richard67 done.

To be really sure we maybe should wait until @infograf768 has tested, because he seems to have a different test setup and maybe test things which I have forgotten - not that I knew uf such things at the moment, but to be really sure his expertise could be helpful I think.

@Hackwar

The following test are done with:

• staging multilanguage install
• clean multilanguag install of your branche

with

• en-GB
• ru-RU
• de-DE

Here are some of my results here:

• Option ?lang=en don't work
• search produce a 404 error. (try here: http://localhost/staging/index.php/en/component/search/ to search Материал; it works on staging)

Your branche

Staging

• Switching from language to language using the multilanguage module works ok here.
• Changing language on login works.

@hackwar can i add a PR against yours to fix some CS or should we do this after merge?

I can confirm @zero-24 's issue with search, but only with SEF URLs on. With non-SEF URLs it works.

But the option ?lang=en works for me, i.e. I can change language by changing the option's value in the URL field ot the browser and then loading the page for the new URL.

P.S. Shall I alter my test result again now?

But the option ?lang=en works for me, i.e. I can change language by changing the option's value in the URL field ot the browser and then loading the page for the new URL.

sure?

what i did is:

• access the default page (german)
• http://localhost/languagerefactor/
• add the ?lang=en
• http://localhost/languagerefactor/?lang=en
• result is: http://localhost/languagerefactor/de/?lang=en and the german text (works on staging)

  ---

  _{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5140.}

@zero-24 Ah, you mean with SEF URLs on change language by using the option? Well, I noticed with my tests before that this did work with pure staging but not with this PR, see above http://issues.joomla.org/tracker/joomla-cms/5140#event-59674 and discussion with @Hackwar on this comment, but I was told by @infograf768 that the behavior of this PR is the better one, see his comment: http://issues.joomla.org/tracker/joomla-cms/5140#event-59854.
Beside this, I'll alter my test result because of the issue with search.

Regarding the error with the search component which was reported by @zero-24 I have following output from the Joomla! debug console:
Errors

**Category not found**
Call stack
#   Function    Location
1   JApplicationCms->execute()  JROOT\index.php:40
2   JApplicationSite->doExecute()   JROOT\libraries\cms\application\cms.php:251
3   JApplicationSite->dispatch()    JROOT\libraries\cms\application\site.php:230
4   JComponentHelper::renderComponent() JROOT\libraries\cms\application\site.php:191
5   JComponentHelper::executeComponent()    JROOT\libraries\cms\component\helper.php:360
6   require_once()  JROOT\libraries\cms\component\helper.php:380
7   JControllerLegacy->execute()    JROOT\components\com_content\content.php:16
8   ContentController->display()    JROOT\libraries\legacy\controller\legacy.php:728
9   JControllerLegacy->display()    JROOT\components\com_content\controller.php:104
10  ContentViewCategory->display()  JROOT\libraries\legacy\controller\legacy.php:690
11  JViewCategory->commonCategoryDisplay()  JROOT\components\com_content\views\category\view.html.php:72
12  JError::raiseError()    JROOT\libraries\legacy\view\category.php:127
13  JError::raise() JROOT\libraries\legacy\error\error.php:254

Regarding being able to switch between languages: It should in general not be possible to override URL segments with query parameters. So if you have SEF URLs enabled, you should not be able to change the language by anything but changing the prefix. Otherwise you could have a german page with english JLanguage object and that might at some point result in issues, starting with different language strings being used and ending with language specific methods that pose a security risk if for example UTF8 chars are not handled properly.

@zero-24 Please do CS stuff afterwards. As long as Travis is okay with the current code, I'd rather not change it again and enforce another round of tests just because of that.

Regarding the search bug: Fixed it.

@test Success. Search works now after last correction.

@zero-24 Please do CS stuff afterwards. As long as Travis is okay with the current code, I'd rather not change it again and enforce another round of tests just because of that.

ok. I will add a PR after the merge here.

Regarding the search bug: Fixed it.

Thanks looks fixed with: Hackwar@b8127ed

Regarding the bug: Again the old implementation gets us here. In the old implementation, the first part of the URL was always parsed as a language and in the actual parsing routine, nothing could majorly be broken. Now however, I was first checking if we were called via a POST request. If that is the case, the rest would not be checked. But a POST URL also contains a SEF part and thus we first need to discover that part and then check if we are called via POST.

Please note that the last commit is really simply just a codestyle commit and does not require new tests.

Now that we have 2 tests by @richard67 and @zero-24, could this be set to RTC?

Hmm, from my point of view it is not only code style to remove this unused if, it is a refactoring. I trust you, but I know by experience from myself that errors can happen even with simple things. So I will retest anyway and let you know the result.
Regarding RTC: I also want to have this into 3.4, but as we have seen it is such a interdependent thing and - even after your large cleanup, millions of thanks for this - still some kind of complex, so I would feel safer if @infograf768 had a successful test, too. Just my opinion.

@richard67 The code was the following:

$found = false;
if (!$found) {
//do stuff
}

That if will always execute, come hail or storm, so it was simply a useless line of code.

@Hackwar Yes, I just have checked it with Beyond Compare, which is a bit more useful than the stupid GitHub diff, and it looks safe.

@Hackwar You know the code change above was clear to me also by the GitHub diff, but it was not so obvious if not accidently anything else had been changed within the if beside indention, e.g. because you have hit the del key with your head when falling asleep after this much work in this PR ;-) Maybe call me paranoid ;-)

No problem.

@test successful - on my bilingual site everything still works after the latest change. Didn't detect any issue when switching languages with SEF enabled/disabled.

@test tested it on a site with three different languages. Couldn't find any issues while looking around and testing the language stuff. Looks good to me.

@test

looks fine now here with my usual tests.

Based on several testing i think we can move it to RTC now. Thanks to all for the hard work on this!

Thanks. Merged with a9c7eb2

Thank you