Last modified: 2010-09-30 00:49:00 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 T27354, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 25354 - CentralNotice $sitename variable not working with ParserFunctions
CentralNotice $sitename variable not working with ParserFunctions
Status: RESOLVED FIXED
Product: MediaWiki extensions
Classification: Unclassified
CentralNotice (Other open bugs)
unspecified
All All
: Normal enhancement (vote)
: ---
Assigned To: Ryan Kaldari
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-28 19:33 UTC by Casey Brown
Modified: 2010-09-30 00:49 UTC (History)
6 users (show)

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


Attachments

Description Casey Brown 2010-09-28 19:33:12 UTC
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à
 |Viquillibres|Viquitexts=Els {{SITENAME}} estan
 |Viquinotícies|Viquidites=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:

* http://ca.wikipedia.org/wiki/?banner=VectorNewLook_phase3
* http://ca.wiktionary.org/wiki/?banner=VectorNewLook_phase3

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
Comment 1 Ryan Kaldari 2010-09-28 20:16:03 UTC
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 :(
Comment 2 Ryan Kaldari 2010-09-28 20:19:06 UTC
Something like...
<script type="text/javascript">
switch ($sitename) {
   case "Viccionari":
   ...
}
</script><noscript>$sitename està</noscript>

should work in Javascript.
Comment 3 MZMcBride 2010-09-28 20:20:52 UTC
(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.
Comment 4 Ryan Kaldari 2010-09-28 20:22:27 UTC
Would you care to elaborate on the snarky comment?
Comment 5 Casey Brown 2010-09-28 20:24:04 UTC
(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?
Comment 6 Ryan Kaldari 2010-09-28 20:28:03 UTC
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.
Comment 7 MZMcBride 2010-09-28 20:29:00 UTC
(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.
Comment 8 Ryan Kaldari 2010-09-28 20:40:43 UTC
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
Comment 9 MZMcBride 2010-09-28 21:10:29 UTC
(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.
Comment 10 Ryan Kaldari 2010-09-28 21:31:03 UTC
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 :)
Comment 11 p858snake 2010-09-28 23:00:09 UTC
(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.
Comment 12 Ryan Kaldari 2010-09-28 23:23:41 UTC
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.
Comment 13 Ryan Kaldari 2010-09-29 00:15:04 UTC
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.
Comment 14 Ryan Kaldari 2010-09-29 00:25:20 UTC
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...
Comment 15 Leinad 2010-09-29 02:07:39 UTC
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 :(
Comment 16 Roan Kattouw 2010-09-29 15:58:28 UTC
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.
Comment 17 Leinad 2010-09-29 16:40:51 UTC
{{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:}}.
Comment 18 Ryan Kaldari 2010-09-29 19:47:44 UTC
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.
Comment 19 Ryan Kaldari 2010-09-29 19:48:25 UTC
actually, hold that thought...
Comment 20 Ryan Kaldari 2010-09-29 19:55:48 UTC
Sorry, svn conflict. Should actually be fixed on tesla now:
http://donation.tesla.usability.wikimedia.org/index.php/Special:CentralNotice

Please help test.
Comment 21 Ryan Kaldari 2010-09-29 20:00:08 UTC
I've thrown out the new $sitename and $amount scheme. You should be able to use any combination of {{SITENAME}}, {{#switch}}, {{#ifeq}}, {{GRAMMAR}}, etc.
Comment 22 Leinad 2010-09-29 20:02:57 UTC
(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.
Comment 23 Ryan Kaldari 2010-09-30 00:49:00 UTC
This is working as expected on Tesla according to Casey. Should get deployed to live next week. Marking fixed.

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


Navigation
Links