Last modified: 2012-10-28 12:30:42 UTC
Review Diff lib extension for deployment on Wikidata. It can be found on Gerrit in the mediawiki/extensions/StickToThatLanguage project. See https://www.mediawiki.org/wiki/Extension:Stick_to_That_Language for more info.
Running regexs on final html output is a little icky ;)
(In reply to comment #1) > Running regexs on final html output is a little icky ;) So let's rewrite the skinning system so we don't have to :)
Two other things immediately jump out: * Class "Ext" seems likely a really vague name and has the possibility of conflicting with another name. * Rather than checking for the HTTP_USER_AGENT to see if it's a web request, it would be better to see if php_sapi_name() != 'cli'
(In reply to comment #3) > Two other things immediately jump out: > * Class "Ext" seems likely a really vague name and has the possibility of > conflicting with another name. Ignore that nitpick--I'm not used to looking for namespaces in PHP.
( version_compare( $wgVersion, '1.16', '>=' ) && isset( $wgExtensionAssetsPath ) && $wgExtensionAssetsPath ) ^ Is there any point adding pre 1.16 compatibility? It just seems pointless supporting a version that was EOL (from "our" POV) since November 2011, when you're primarily developing for "trunk" (or thereabouts) wikis?
"/Wikibase/$dirName"; // FIXME: has to be adjusted as soon as extension moves! Well extension has moved... ---- OnUserGetDefaultOptions hook: Anon users get a big list of default preferred languages. The moment you log in (at least on http://wikidata-test-repo.wikimedia.de which is where I'm testing this from) the list of preferred languages goes to nothing. The default preference should be the same as what logged out users get imo.
How does this interact with squid caching? My understanding is that having the uselang parameter disables squid caching (So in effect, this extension disables squid caching on essentially all pages), this may be a "feature" (Purging every language code seems unrealistic) but should be documented if that is the case.
(In reply to comment #5) > ( version_compare( $wgVersion, '1.16', '>=' ) && isset( $wgExtensionAssetsPath > ) && $wgExtensionAssetsPath ) > > ^ Is there any point adding pre 1.16 compatibility? It just seems pointless > supporting a version that was EOL (from "our" POV) since November 2011, when > you're primarily developing for "trunk" (or thereabouts) wikis? +1, Even more so given the hook "GetLocalURL::Internal" which is used by this extension was added in 1.19.
Line 141 of StickToThatLangauge.php needs a \ before User (no point me making a commit just to fix that) Why are we using isset on a global? It's defined in DefaultSettings.php as false, so it'll be bool (false)|string
I have fixed mentioned stuff in https://gerrit.wikimedia.org/r/17113 - Chad: > Rather than checking for the HTTP_USER_AGENT to see if it's a web request, it would be better to see if php_sapi_name() != 'cli' I just remembered that I can simply use $wgCommandLineMode now. A global implying whether tests are running currently would be nice though. - Reedy: > Why are we using isset on a global? and > Is there any point adding pre 1.16 compatibility? I guess not, except if you consider people copying that \Ext class for their own extensions who support compatibility. I copied this from one of my other extensions. But now I altered it to not use $wgExtensionAssetsPath for all cases. $wgExtensionAssetsPath is only set to false during LocalSettings, and that is where we would require it already. The cleanest solution for all of that resource loader registration stuff would probably be if it would require a $wgExtensionAssetsPath relative path... - Bawolff: > Well extension has moved... good point, just fixed that path as well. > How does this interact with squid caching? First, this extension causes cache fragmentation, so we have one version of each language in the parser cache. If I am not mistaken, it should be possible for squid caching to still be active, it might even work with the current setup. > Anon users get a big list of default preferred languages. The moment you log > in (at least on http://wikidata-test-repo.wikimedia.de which is where I'm > testing this from) the list of preferred languages goes to nothing. The > default preference should be the same as what logged out users get imo. We thought about that, but decided against it at some point. The point was, that displaying the largest 10 Wikipedias languages would significantly increase the chance of more anon users finding their language immediately. For logged in users on the other hand, it is very unlikely that one user speaks all of those languages, we just know the user knows the language which is chosen as its interface language, so we pre-select that one as default instead.
>First, this extension causes cache fragmentation, so we have one version of >each language in the parser cache. If I am not mistaken, it should be possible >for squid caching to still be active, it might even work with the current >setup. Note that the parser cache and squid cache are totally separate. I believe that MW disables squid caching for any request with the uselang parameter (However, when i looked for the code to accomplish this in MW, I could not find it, but i didn't look all that hard. on enwikipedia in my own testing, and request with uselang generates a cache miss). Furthermore if squid caching wasn't disabled for those urls, you would end up with a lot of stale results, since those urls are not purged. Hence I don't neccesarily think (imho) it's a bad thing that the squid cache is essentially for all intents and purposes disabled with this extension, but at the same time that's something that should be mentioned explicitly in the extensions docs if it is the case. >For logged in users on the other hand, it is very unlikely that one user speaks >all of those languages, we just know the user knows the language which is >chosen as its interface language I don't think that's a particularly good assumption. How many people even know Special:preferences exists. (OTOH it may be a good asumption if the language selected via this extension that the user creates an account with carries over to be the interface language, but SUL probably would make that not be realistic to happen). ---- >Running regexs on final html output is a little icky ;) Further on that note, what if MW (or an extension) outputted a form that had a literal '>' in one of its attributes. My understanding is that unescaped '>' characters are valid in an attribute (albeit I haven't been able to find anywhere in MW that does this [or at least does that and is manipulatable by the user] for form elements, although in theory I believe the HTML class would if it was used for form elements). This would at the very least alter the forms html into something invalid and unintended. And furthermore would quite possibly be an XSS issue [in particular if $wgWellFormedXml is off [See HTML::expandAttributes], smarter minds than me might be able to think of security issues even with that setting on] .
Bawolff: I have fixed the issue with the '>' in a separate patch-set, see https://gerrit.wikimedia.org/r/#/c/17889/ > should be mentioned explicitly in the extensions docs if it is the case Alright, I added two lines about caching to the README, one about the squid, hope it is helping. About your concerns regarding the languages displayed on top of the list for logged-in users, I will mention it to the team and we can think about it again. Personally, I think neither of both is a perfect solution and it could be done much better somehow. Perhaps by communicating to the user that more languages can be chosen, also allowing to do this directly within the language selectors list of languages. But that would mean more work and it doesn't have high priority right now.
AFAIK Diff has been reviewed and installed. Is StickToThatLanguage still relevant?
no. ULS fulfills our needs, although perhaps StickToThatLanguage is useful elsewhere for third-parties, etc.
