Page MenuHomePhabricator

CentralNotice $sitename variable not working with ParserFunctions
Closed, ResolvedPublic

Description

Author: bugs

Description:
The new variable for {{SITENAME}} ($sitename) doesn't seem to be working with ParserFunctions, which we need for some languages to work properly.

For example, see the Catalan version of the VectorNewLook_phase3 banner.[0] We use this code:

{{#switch:{{SITENAME}}

Viquipèdia=La {{SITENAME}} està
Viccionari=El {{SITENAME}} està
ViquillibresViquitexts=Els {{SITENAME}} estan
ViquinotíciesViquidites=Les {{SITENAME}} estan
#default={{SITENAME}} està }} canviant d'aspecte.

so that the banner is grammatically correct on all of the projects.

I replaced {{SITENAME}} with $sitename to see if this still works with your new variable, but it looks like it doesn't:

Both of those are showing the default phrasing, "SITENAME està", while they should say "La Viquipèdia està" and "El Viccionari està" respectively... so something's not working and the ParserFunction isn't being processed correctly.

[0]http://meta.wikimedia.org/wiki/Special:NoticeTemplate/view?template=VectorNewLook_phase3&wpUserLanguage=ca


Version: unspecified
Severity: enhancement

Details

Reference
bz25354

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 11:20 PM
bzimport set Reference to bz25354.

Most wikitext functions will not work in banners. The only reason some of them worked before is because the banner pre-generation process allowed us to implement some ugly hacks to use some of them. We should probably not be using any wikitext within banners. We should be using HTML and Javascript instead. If there is a requirement that banners use wikitext, we'll need to reimplement the banner loader in a different way. When I asked Alex and Philippe about what wiki functions were needed, I was only told about message translation and {{SITENAME}}. No one mentioned the use of parser functions :(

Something like...
<script type="text/javascript">
switch ($sitename) {

case "Viccionari":
...

}
</script><noscript>$sitename està</noscript>

should work in Javascript.

(In reply to comment #2)

Something like...
<script type="text/javascript">
switch ($sitename) {

case "Viccionari":
...

}
</script><noscript>$sitename està</noscript>

should work in Javascript.

That's kind of disgusting.

Would you care to elaborate on the snarky comment?

bugs wrote:

(In reply to comment #1)

When I asked Alex and Philippe about what
wiki functions were needed, I was only told about message translation and
{{SITENAME}}. No one mentioned the use of parser functions :(

I think the only other wiki-text that we use is ParserFunctions (#switch, #ifeq), which is used to support {{SITENAME}}. (In a lot of languages, the sentence changes depending on the gender of the noun or case or other weird things.)

(In reply to comment #2)

Something like...
<script type="text/javascript">
switch ($sitename) {

case "Viccionari":
...

}
</script><noscript>$sitename està</noscript>

should work in Javascript.

I have no idea how JavaScript works, so I'll just have to take your word for it. :-)

Is the <noscript></noscript> text the default (i.e. if the sitename isn't one of those ones, use this)? Or is it just shown if JavaScript isn't working?

The noscript is output if Javascript is turned off or isn't working. You'll still need to have a default option within the Javascript itself. If I can't figure out a good way to re-hack this functionality, I'll write up some complete Javascript for you to use for this situation.

(In reply to comment #4)

Would you care to elaborate on the snarky comment?

It's not snarky. I don't think it's particularly reasonable to require people implementing these banners to have a working knowledge of JavaScript. I think that requiring people to learn and use JavaScript is going to needlessly complicate these banners. Worse than requiring people to learn how to operate <script> tags, you're suggesting that they understand the semantics of <noscript> as well. This is ugly and hackish.

It might be possible to include some sort of wrapper function, either in PHP or JavaScript, that makes this less painful. I'm not really sure how feasible that is, though if you were going to do that, you may as well just implement wikitext, because it's already at least understood by the people who work with these banners.

As someone who has worked with these templates/banners/etc. in the past, even relatively simple code quickly becomes mangled and broken as these templates are duplicated and modified.

So it's not reasonable to expect banner writers to know basic Javascript, but it is reasonable that they understand using Wikitext parser functions? Regardless, any way that this is implemented will be a hack since the banners don't live on the local wikis, they live on meta. So they can't just be parsed and pulled into the page like a template. And {{SITENAME}} is especially problematic for this reason. Obviously if I had known that this was needed, I would have built a better way to do it in CentralNotice. In the meantime, I'm offering a workaround. But next time, I'll just keep my "disgusting" ideas to myself :P

(In reply to comment #8)

So it's not reasonable to expect banner writers to know basic Javascript, but
it is reasonable that they understand using Wikitext parser functions?
Regardless, any way that this is implemented will be a hack since the banners
don't live on the local wikis, they live on meta. So they can't just be parsed
and pulled into the page like a template. And {{SITENAME}} is especially
problematic for this reason. Obviously if I had known that this was needed, I
would have built a better way to do it in CentralNotice. In the meantime, I'm
offering a workaround. But next time, I'll just keep my "disgusting" ideas to
myself :P

Sometimes, people look back at what was previously implemented and say "this was a horrible hack, why did we do it this way?" I realize that some of these problems aren't easy to solve and I realize the importance of this project. I imagine that's why the Wikimedia Foundation has hired someone to work on this rather than relying on volunteers. :-)

The problem with workarounds is that they have a tendency to become long-lasting. This is true of a lot of areas of MediaWiki/Wikimedia development.

In my view, there should be as little coding required by the users of this extension as possible. While JavaScript is obviously more common on the Web, for most wiki users, ParserFunctions/wikitext are more common and easier to understand. This brings the benefit of people being available to help if the ParserFunctions/wikitext get complicated. The same really isn't as true for JavaScript, surprising as that may seem.

That said, I don't think supporting wikitext/ParserFunctions is necessarily a must-have for this extension, but it does provide some somewhat sane features (like {{PLURAL:}}) that could be handy in banners.

When non-coders (or even coders sometimes) are forced to do coding, especially in an environment that doesn't have input sanity checks like Tidy, we end up with all sorts of nastiness from things like unclosed tags, for example. Your suggested code seems frail and hackish. I imagine you knew this when proposing it. I didn't mean to judge you, just your suggestion, as being "disgusting." I'm also a bit tired today; that's probably not helping matters either. Apologies if I came across as snarky.

The original banner parsing code was prefixed with a warning declaring it "A god-damned dirty hack" by whoever wrote it, so it wasn't actually my subjective opinion :)

I agree that using parser functions for all the uses you describe probably makes more sense than Javascript. I just wasn't aware those functions were needed in CentralNotice banners. Going on the information that I had (that we just needed a way to output the site name), I thought it made sense to throw out the parser hack (that was also causing problems with message caching) and just support $sitename as a sort of custom variable. Oh well, hindsight's 20/20. At least with the new banner loading system we can now do geotargeting, get real banner impressions, test banners within local wikis, and it doesn't break on secure servers. Trading 4 bugs for 1 isn't so bad is it?

And thanks for the apology. It's frustrating to get criticism without elaboration or explanation. Constructive criticism is always welcome, however :)

(In reply to comment #10)

The original banner parsing code was prefixed with a warning declaring it "A
god-damned dirty hack" by whoever wrote it, so it wasn't actually my subjective
opinion :)

I agree that using parser functions for all the uses you describe probably
makes more sense than Javascript. I just wasn't aware those functions were
needed in CentralNotice banners.

Not to be snarky, but if something is written in as a hack, it's generally needed (and wanted) even if people don't remember to tell you.

Yes, obviously it was needed, but it didn't say what it was needed for. I was told it was needed for {{SITENAME}}. When I rewrote all of the banner loading code (which was necessary in order to be able to support the new features listed above), rather than reimplementing some kind of hack to allow partial wiki parsing, I just wrote something very straightforward to support the output of site names. I wasn't just throwing out old code for the fun of it.

It looks like the old hack isn't easily implementable in the new system. Now that we are doing banner message localization based on user language rather than site language (which was another bug fixed by the new system) we can't use the site language for guessing the database name (an essential part of the hack) without further fragmenting our caching scheme. The hack also required doing $wgConf->loadFullData() on every banner request, which didn't matter when they were all pre-generated, but now that they are generated dynamically, that could be a performance hit. I'm still investigating solutions, however.

I think I have a solution that doesn't require guessing the database name or loading a remote wiki's configuration on every banner request. Stay tuned...

danny.leinad wrote:

Hello!

I would like to report that Polish language also needs {{#switch: or some substitute.

In Polish we have various grammar forms which depend on the name of the project. If you do not fix this issue, we will have create not one, but six banners for each project :(

Don't we have {{GRAMMAR}} for these kinds of things? I'm not totally sure how {{GRAMMAR}} works, but if it can be used for this it should result in nicer-looking code than that {{#switch:}}.

Of course {{GRAMMAR}} and {{#switch:}} suffer from the exact same problems, so trading one for the other doesn't actually help you to fix this bug.

danny.leinad wrote:

{{GRAMMAR:}} provides (http://svn.wikimedia.org/viewvc/mediawiki/trunk/extensions/WikimediaMessages/WikimediaGrammarForms.php) only declension of the names of the
projects.

Polish (and other Slavic languages) also have inflection of other words, so we need {{#switch:}}.

OK, this should be fixed on the tesla test server now. Please help me test it and make sure that everything is working as expected.

actually, hold that thought...

I've thrown out the new $sitename and $amount scheme. You should be able to use any combination of {{SITENAME}}, {{#switch}}, {{#ifeq}}, {{GRAMMAR}}, etc.

danny.leinad wrote:

(In reply to comment #20)

Sorry, svn conflict. Should actually be fixed on tesla now:
http://donation.tesla.usability.wikimedia.org/index.php/Special:CentralNotice

Please help test.

I can't create an account.

This is working as expected on Tesla according to Casey. Should get deployed to live next week. Marking fixed.