Last modified: 2014-04-25 07:06:47 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T59091, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 57091 - onSkinTemplateOutputPageBeforeExec hook use causes language button to show up on pages in mobile
onSkinTemplateOutputPageBeforeExec hook use causes language button to show up...
Status: REOPENED
Product: MediaWiki extensions
Classification: Unclassified
UniversalLanguageSelector (Other open bugs)
unspecified
All All
: Normal major (vote)
: ---
Assigned To: Nobody - You can work on this!
: mobile
Depends on: 56819
Blocks:
  Show dependency treegraph
 
Reported: 2013-11-15 01:08 UTC by Jon
Modified: 2014-04-25 07:06 UTC (History)
14 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Jon 2013-11-15 01:08:41 UTC
The addition of uls-p-lang-dummy to language_urls makes a broken language button appear on all pages including special pages. On mobile a language section is only shown if languages are available. This has been surfaced by a recent upstream of code we performed to make the mobile skin more compatible with default skins.

This is a hack and should be removed.

I can add another hack to mobile to counter this but I'd rather fix this issue at source if possible...
Comment 2 Jon 2013-11-15 01:22:40 UTC
Is UniversalLanguageSelector deployed on any wikis? If so it's quite urgent we find some kind of fix for this before the next deployment train on 21st, if not feel free to lower the priority.
Comment 3 Andre Klapper 2013-11-15 10:39:05 UTC
(In reply to comment #2)
> Is UniversalLanguageSelector deployed on any wikis?

On all since 07/2013: https://www.mediawiki.org/wiki/UniversalLanguageSelector/Deployment/Planning

> If so it's quite urgent we find some kind of fix for this before the next 
> deployment train on 21st, if not feel free to lower the priority.

Added Greg to CC and set as blocking bug 38865.
Comment 4 Gerrit Notification Bot 2013-11-15 18:38:21 UTC
Change 95648 had a related patch set uploaded by Jdlrobson:
Hack around ULS bug 57091

https://gerrit.wikimedia.org/r/95648
Comment 5 Jon 2013-11-15 18:38:53 UTC
Mobile now has a temporary workaround...
Comment 6 Amir E. Aharoni 2013-11-19 04:35:03 UTC
Ack, a non-hacky way to add the ULS gear icon is needed.

There is a similar issue in Wikidata: bug 57094.
Comment 7 Jon 2013-11-21 19:16:02 UTC
In a similar vein, I'm now seeing another phantom list item on beta labs:
<li id="wbc-linkToItem" class="wbc-editpage wbc-nolanglinks"></li>

http://en.m.wikipedia.beta.wmflabs.org/w/index.php?title=0.9078091581611335_Moved&title=0.9078091581611335_Moved

Is this caused by ULS as well?

Greg, Reedy this is potentially a blocker for deploying MobileFrontend
Comment 8 Jon 2013-11-21 19:21:03 UTC
Ahh this seems to be Wikibase client... I haved opened bug 57367 (however the original issue still needs to be addressed)
Comment 9 Siebrand Mazeland 2013-11-25 22:37:04 UTC
Jon, with https://gerrit.wikimedia.org/r/#/c/94899 for bug 56819 merged, are you now able to implement a future proof solution?
Comment 10 Jon 2013-11-26 17:27:27 UTC
So if I understand correctly you now annotate these phantom languages with a class? I haven't taken a closer look at the code yet and am not 100% clear why languages need to be used in this way (wouldn't a hook allowing you add HTML before or after the list be better?) but this still seems a little hacky to me and semantically incorrect.. a list of languages should only contain languages surely?

I'll take a closer look later today and retract my comments if I've misunderstood :)
Comment 11 Niklas Laxström 2013-11-27 18:55:17 UTC
(In reply to comment #10)
> So if I understand correctly you now annotate these phantom languages with a
> class? I haven't taken a closer look at the code yet and am not 100% clear

The languages have class, non-languages don't.

> languages need to be used in this way (wouldn't a hook allowing you add HTML
> before or after the list be better?)

The code generating the html for the list is duplicated at least in three places and it wouldn't easily allow forcing the section to show up.

> but this still seems a little hacky to me

Yes it is. The whole related code is filled with comments saying "hack". We were looking for a solution of least effort here, instead of big rewrites.

> and semantically incorrect.. a list of languages should only contain
> languages
> surely?

Only if someone declares it as a list of languages. I'd rather see it as a way to access different language versions without specifying the functionality ;)
Comment 12 Jon 2013-11-27 19:13:22 UTC
(In reply to comment #11)
> (In reply to comment #10)
> > So if I understand correctly you now annotate these phantom languages with a
> > class? I haven't taken a closer look at the code yet and am not 100% clear
> 
> The languages have class, non-languages don't.

Got it.

> > but this still seems a little hacky to me
> 
> Yes it is. The whole related code is filled with comments saying "hack". We
> were looking for a solution of least effort here, instead of big rewrites.
>
OK as long as that's clear. I understand the need for a solution of least effort - we encounter this problem all the time in mobile - but I do worry about hacks as MediaWiki is already full of them and this suggests hacks tend to stay around for a long time which makes me sad. I think this is a bigger picture question off topic for this thread - why are non-hacks so much effort?

> > and semantically incorrect.. a list of languages should only contain
> > languages
> > surely?
> 
> Only if someone declares it as a list of languages. I'd rather see it as a
> way
> to access different language versions without specifying the functionality ;)

Well someone /did/ declare it as a list of languages - the element id attribute value of the list itself is #p-lang-list :). uls-p-lang-dummy is not a language link so shouldn't be in that list period.

From mobile's perspective, we can iterate through the list of languages and strip out any elements which have don't have this new class (although to do that we will have to do a string search for each class in the list - and in pages with lots of languages that could be 200+ checks). Thus it's still not ideal, as most of these elements in mobile are inactive due to JavaScript and CSS not being loaded (it would actually be better not to add them in the first place if you detect you are in mobile mode - MobileContext::singleton()->shouldDisplayMobileView();). That said it's definitely more workable and future proof then the existing solution but seems an unnecessary step.
Comment 13 Siebrand Mazeland 2013-12-03 08:45:04 UTC
The current situation can be observed at http://en.wikipedia.beta.wmflabs.org/wiki/Main_Page. Can this issue be marked as resolved?
Comment 14 Ryan Kaldari 2013-12-06 19:09:48 UTC
Siebrand: Is it accurate that uls-p-lang-dummy has been removed from the language list in all cases now? (I don't see it currently.) Is there a gerrit change related to this?
Comment 16 Gerrit Notification Bot 2013-12-06 20:40:27 UTC
Change 99693 had a related patch set uploaded by Jdlrobson:
Override language_urls template data to avoid hook abuse

https://gerrit.wikimedia.org/r/99693
Comment 17 Siebrand Mazeland 2013-12-06 21:11:08 UTC
(In reply to comment #14)
> Siebrand: Is it accurate that uls-p-lang-dummy has been removed from the
> language list in all cases now? (I don't see it currently.) Is there a gerrit
> change related to this?

Ryan: The only change I'm aware of is Gerrit change #94899. As said before, I don't agree with how Jon's decided to frame this discussion, and the consequent classification of this issue. As far as I'm concerned, it's done with aforementioned merged patch set.
Comment 18 Ryan Kaldari 2013-12-06 21:27:35 UTC
Change #94899 just perpetuates the problem with another work-around. There should not be any reason why this item needs to be included in the language list. If you need some sort of trigger for adding the ULS gear, why not set a config var through the ResourceLoaderGetConfigVars hook? That's the normal way to handle such things.
Comment 19 Gerrit Notification Bot 2013-12-07 00:31:19 UTC
Change 99693 merged by jenkins-bot:
Override language_urls template data to avoid hook abuse

https://gerrit.wikimedia.org/r/99693
Comment 20 Andre Klapper 2014-02-14 13:21:47 UTC
All three patches mentioned in this ticket got merged. Is there more work left to do here (if yes: please reset the bug report status to NEW or ASSIGNED), or can you close this ticket as RESOLVED FIXED?
Comment 21 Andre Klapper 2014-04-25 06:48:14 UTC
No reply to comment 20 - assuming this bug is FIXED.
If that is not the case: Please reopen and elaborate what is left to do here to get this report fixed.
Comment 22 Andre Klapper 2014-04-25 06:49:02 UTC
Bleh. Guess I should keep this open due to comment 18...

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links