[#7271] - [CODE REVIEW] - languagefilter.php
- Closed
- 13 Jul 2015
- Medium
- Build: staging
- # 7271
- Diff
- smz:LanguageFilter
- Success The Travis CI build passed Details
User tests: Successful: Unsuccessful:
Description
This is a code review of the "Language Filter" system plugin.
The main objective was code simplification and optimization.
At the same time some small issues/bugs have been solved.
The major differences can be found in the parseRule() method where, I think, the logic is also better documented. getLanguageCookie() too has some major mods.
Less important modifications have been introduced in onUserBeforeSave(), onUserAfterSave(), onUserLogin() and onAfterDispatch()
Testing instructions
- Have a multilingual site with at least three languages installed
- Test front-end multilingual functionalities (including logging in with users that have a specific language assigned)
- Verify correct browser language detection
- Verify correct menu items and component association
- Verify that everything else is OK, no regression exist and some small glitches are gone
Potentially solved issues (please test...)
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Labels |
Added:
?
|
||
Do we have problems with Travis and PHP 5.5?
Updating dependencies (including require-dev)
- Installing squizlabs/php_codesniffer (1.5.6)
Downloading: connection... Failed to download squizlabs/php_codesniffer from dist: Could not authenticate against github.com
Now trying to download from source
- Installing squizlabs/php_codesniffer (1.5.6)
Cloning 6f3e42d311b882b25b4d409d23a289f4d3b803d5
and also:
No output has been received in the last 10 minutes, this potentially indicates a stalled build or something wrong with the build itself
No. Travis has just been really flaky as of late.
OK, Thanks!
Will it retest automatically or have I to make some changes to trigger it?
Someone with permission has to reset the job. I just did it.
Great! Thanks, Michael! 
Thanks Robert! I missed that, and I will surely look at it and see if there is anything I can bring in!
As you're familiar with languagefilter.php you'll probably may also give a look at my code and see if it looks all-right to you (if you have some time, of course...).
A bit busy atm but sure will do.
zero-24
- | Category | ⇒ | Multilanguage |
Although it looks like a simplification, I am not sure that checking browser language in the private function getLanguageCookie() is very semantic...
Although it looks like a simplification, I am not sure that checking browser language in the private function
getLanguageCookie()is very semantic...
Agreed!
What could be a better name for that function?
- getPreferredLanguage ()
- guessLanguage()
- getLanguage()
- getCookieOrBrowserLanguage()
- getClientLanguage()
- ...................................................
Ouch... I made some mods but Travis got stuck again with PHP 5.5...
Can someone, please, re-submit the job?
Thanks!
done
Thanks, Robert!
Apparently good 'ol Travis now got stuck on PHP:5.3, but don't worry: I'll have a minor commit for tonight and that will re-trigger it. I'll just be sure to cross my finger while committing...
In theory I have some more small mods to push, but as they are limited to code style and comments fixing, and Travis finally got through with all PHP versions, I'll defer those minor mods to a second time, if and when there will be consensus that this is valid code worth merging.
Is that OK?
As it is now, code looks like working for me.
With latest commit I fix an unreported (I think) issue:
While in non-SEF mode there were several cases in which home pages were not detected and as a consequence the rel='alternate' links were not generated.
I also modified those links to be of the form /index.php?lang=sef, without any redundant reference to item IDs. For consistency, I also plan to apply a similar patch to the "Language Switcher"
I think this PR solves issue #7047 too. If testers will confirm this, that issue could be closed
@infograf768 a question: are there cases in which we want the language switcher filter to function for languages that do not have an associated home page? If not, I can make further simplifications...
@infograf768 a question: are there cases in which we want the language switcher filter to function for languages that do not have an associated home page? If not, I can make further simplifications...
No. A content language without a home page is like a bicycle without a saddle...
Will look at your last iteration of the PR as soon as possible, but
While in non-SEF mode there were several cases in which home pages were not detected and as a consequence the rel='alternate' links were not generated.
I first would have to be able to reproduce this.
Easiest way to reproduce the issue with rel='alternate' links in home pages, non-SEF mode, is to go to the naked domain (default language, cookie language or browser detected language do not matter): you will not find alternates in there. Now switch to another language and then back to the original one: alternates will be there (and ugly...).
@infograf768 I forgot to mention that if you test this for fixing the non-SEF home page rel=alternate issue, it would be better if you test it together with #7286
This is not a requirement, but then there will be two kinds of links to home pages, the "short ones" in the rel=alternate links and the "long ones" in the Language Switcher. Those links will be both "seen" by crawlers and I think it will be unpredictable which one will be indexed by search engines.
Having the "short form" indexed would be a lot better as this will not change even if you'll change the home pages menu itemid (or even component) at a second time.
Easiest way to reproduce the issue with rel='alternate' links in home pages, non-SEF mode, is to go to the naked domain (default language, cookie language or browser detected language do not matter): you will not find alternates in there.
This does not change with your patch.
Now switch to another language and then back to the original one: alternates will be there (and ugly...).
They are changed indeed but is it B/C ?
Also how is sef concerned by the language switcher when no SEF?
Looks like we need to add htmlspecialchars here btw:
// Current language link
case ($i == $this->current_lang):
$language->link = htmlspecialchars(JUri::getInstance()->toString(array('path', 'query')));
break;
Otherwise we get this for associated items (below for it-IT)
<!DOCTYPE html>
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="it-it" lang="it-it" dir="ltr">
<head>
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<meta http-equiv="content-type" content="text/html; charset=utf-8" />
<meta name="generator" content="Joomla! - Open Source Content Management" />
<title>Italiano - Multilingua passo dopo passo</title>
<link href="/testwindows/trunkgitnew/index.php?option=com_content&view=category&id=32&Itemid=151&lang=it&format=feed&type=rss" rel="alternate" type="application/rss+xml" title="RSS 2.0" />
<link href="/testwindows/trunkgitnew/index.php?option=com_content&view=category&id=32&Itemid=151&lang=it&format=feed&type=atom" rel="alternate" type="application/atom+xml" title="Atom 1.0" />
<link href="http://localhost:8888/testwindows/trunkgitnew/index.php?option=com_content&view=category&id=12&lang=en&Itemid=122" rel="alternate" hreflang="en-GB" />
<link href="http://localhost:8888/testwindows/trunkgitnew/index.php?option=com_content&view=category&id=11&lang=fr&Itemid=121" rel="alternate" hreflang="fr-FR" />
<link href="http://localhost:8888/testwindows/trunkgitnew/index.php?option=com_content&view=category&id=27&lang=de&Itemid=143" rel="alternate" hreflang="de-DE" />
<link href="http://localhost:8888/testwindows/trunkgitnew/index.php?option=com_content&view=category&id=32&Itemid=151&lang=it" rel="alternate" hreflang="it-IT" />
<link href="http://localhost:8888/testwindows/trunkgitnew/index.php?option=com_content&view=category&id=21&lang=es&Itemid=131" rel="alternate" hreflang="es-ES" />
<link href="http://localhost:8888/testwindows/trunkgitnew/index.php?option=com_content&view=category&id=36&Itemid=180&lang=mk" rel="alternate" hreflang="mk-MK" />
<link href="http://localhost:8888/testwindows/trunkgitnew/index.php?option=com_content&view=category&id=41&Itemid=192&lang=ta" rel="alternate" hreflang="ta-IN" />
Now, to be honest: it took ages to get the regressions out of the languagefilter. I am frightened by your changes.
I can't reproduce the fact that you can't reproduce... Honestly, I consistently get the issue w/o this patch and consistently don't get it when the patch is applied... That's curious... How can we get out of this?
I don't think we have a B/C commitment for non-SEF URLs (but I may be wrong...). In any case I sincerely think that the short-form URLs for home pages are a lot preferable for search engines, for the reasons I explained above.
You're right that there is a dis-alignment between current link and "other links" (the htmlspecialchars thing...), but I think we should fix it the other way around: it is having HTML entities in URI that is wrong. Unsure about this, but will check...
Changes are always a little bit frightening, but if you give an impartial look at my code you'll see (or at least that's my opinion) that it is much more structured than the current one and this will be a big plus also for future enhancements/modifications...
Depends what the before/after URL is and if it breaks functionality. If it gave a 200 before it should work after. See the B/C statement for URLs.
@mbabker no functionality is broken here: where you used to get a 200 you'll still get a 200
@infograf768 I'm checking the htmlentities thing and indeed it is wrong to have those in URLs. URL encoding is different from HTML enconding and the right function to use in PHP is urlencode() (http://php.net/manual/en/function.urlencode.php)
As the wrong URLs comes from JRoute::_ I'm afraid we have a problem there... will check.
it is having HTML entities in URI that is wrong. Unsure about this, but will check...
As you can see in source for rss, atom, opensearch, form actions, etc., we usually use html entities in urls in Joomla.
yes, but I'm not sure this is correct (per RFCs and w3.org standards)...
from a practical point of view it seems there is no problem: browsers do interpret those & and we are using them since ages, but... will check...
hmmm...
$string = this&that
htmlspecialchars($string) = this&that
htmlentities($string) = this&that
urlencode($string) = this%26that
It seems there are different point of view on this:
- commonly ampersand is encoded as
&in URLs - according to PHP and other sources it should be encoded as
%26
For sure a simple & is wrong: will fix that (with our current convention of using &)!
Note that we have this issue in current code too: no big issues but the generated HTML will not pass HTML validators
& is OK according to w3.org: http://www.w3.org/TR/html4/appendix/notes.html#h-B.2.2
Actually they recommend that:
...HTTP server implementors, and in particular, CGI implementors support the use of ";" in place of "&" to save authors the trouble of escaping "&" characters in this manner
.. but this is of course out of question!
Latest commit fix ampersand encoding issue for current language URLs in onAfterDispatch()
I'm anyway dubious if this shouldn't be directly fixed in AbstractUri::toString() and, obviously, assure we don't make a double encoding elsewhere. [OK, I know the answer: B/C... 
]
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2015-07-13 08:09:05 |
| Closed_By | ⇒ | smz |
| Status | Closed | ⇒ | New |
| Closed_Date | 2015-07-13 08:09:04 | ⇒ | |
| Closed_By | smz | ⇒ |
I have updated #7427
I also discovered (not patched) at the same time that the alternate were not displayed for the home pages when sef was off when we use short urls of the type http://mysite.com/index.php?lang=xx while long home page urls (The ones from the Home menu item) the get the alternate fine.
... pinging @infograf768!