Page MenuHomePhabricator

onSkinTemplateOutputPageBeforeExec hook use causes language button to show up on pages in mobile
Closed, ResolvedPublic

Description

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...


Version: unspecified
Severity: major
See Also:
https://bugzilla.wikimedia.org/show_bug.cgi?id=57094

Details

Reference
bz57091

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 2:12 AM
bzimport set Reference to bz57091.
bzimport added a subscriber: Unknown Object (MLST).

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.

(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.

Change 95648 had a related patch set uploaded by Jdlrobson:
Hack around ULS bug 57091

https://gerrit.wikimedia.org/r/95648

Mobile now has a temporary workaround...

Ack, a non-hacky way to add the ULS gear icon is needed.

There is a similar issue in Wikidata: bug 57094.

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

Ahh this seems to be Wikibase client... I haved opened bug 57367 (however the original issue still needs to be addressed)

Jon, with https://gerrit.wikimedia.org/r/#/c/94899 for bug 56819 merged, are you now able to implement a future proof solution?

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 :)

(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 ;)

(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.

The current situation can be observed at http://en.wikipedia.beta.wmflabs.org/wiki/Main_Page. Can this issue be marked as resolved?

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?

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

(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 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.

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.

Change 99693 merged by jenkins-bot:
Override language_urls template data to avoid hook abuse

https://gerrit.wikimedia.org/r/99693

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?

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.

Bleh. Guess I should keep this open due to comment 18...

Yeh the hack is still in MobileFrontend a year later so I wouldn't call this resolved :)

Jdlrobson claimed this task.

SkinTemplateOutputPageBeforeExec was deprecated so I'm pretty sure this can be resolved.