r77896 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r77895‎ | r77896 | r77897 >
Date:18:31, 6 December 2010
Author:pdhanda
Status:ok (Comments)
Tags:
Comment:
Part of fix/workaround to bug 26163. API calls to get language links and categories as html
Modified paths:
  • /trunk/phase3/includes/api/ApiParse.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/api/ApiParse.php
@@ -219,9 +219,19 @@
220220 if ( isset( $prop['langlinks'] ) ) {
221221 $result_array['langlinks'] = $this->formatLangLinks( $p_result->getLanguageLinks() );
222222 }
 223+ if ( isset( $prop['languageshtml'] ) ) {
 224+ $languagesHtml = $this->languagesHtml( $p_result->getLanguageLinks() );
 225+ $result_array['languageshtml'] = array();
 226+ $result->setContent( $result_array['languageshtml'], $languagesHtml );
 227+ }
223228 if ( isset( $prop['categories'] ) ) {
224229 $result_array['categories'] = $this->formatCategoryLinks( $p_result->getCategories() );
225230 }
 231+ if ( isset( $prop['categorieshtml'] ) ) {
 232+ $categoriesHtml = $this->categoriesHtml( $p_result->getCategories() );
 233+ $result_array['categorieshtml'] = array();
 234+ $result->setContent( $result_array['categorieshtml'], $categoriesHtml );
 235+ }
226236 if ( isset( $prop['links'] ) ) {
227237 $result_array['links'] = $this->formatLinks( $p_result->getLinks() );
228238 }
@@ -326,6 +336,20 @@
327337 return $result;
328338 }
329339
 340+ private function categoriesHtml( $categories ) {
 341+ global $wgOut, $wgUser;
 342+ $wgOut->addCategoryLinks( $categories );
 343+ $sk = $wgUser->getSkin();
 344+ return $sk->getCategories();
 345+ }
 346+
 347+ private function languagesHtml( $languages ) {
 348+ global $wgOut, $wgUser;
 349+ $wgOut->setLanguageLinks( $languages );
 350+ $sk = $wgUser->getSkin();
 351+ return $sk->otherLanguages();
 352+ }
 353+
330354 private function formatLinks( $links ) {
331355 $result = array();
332356 foreach ( $links as $ns => $nslinks ) {
@@ -408,7 +432,9 @@
409433 ApiBase::PARAM_TYPE => array(
410434 'text',
411435 'langlinks',
 436+ 'languageshtml',
412437 'categories',
 438+ 'categorieshtml',
413439 'links',
414440 'templates',
415441 'images',
@@ -444,6 +470,8 @@
445471 ' text - Gives the parsed text of the wikitext',
446472 ' langlinks - Gives the langlinks the parsed wikitext',
447473 ' categories - Gives the categories of the parsed wikitext',
 474+ ' categorieshtml - Gives the html version of the categories',
 475+ ' languageshtml - Gives the html version of the languagelinks',
448476 ' links - Gives the internal links in the parsed wikitext',
449477 ' templates - Gives the templates in the parsed wikitext',
450478 ' images - Gives the images in the parsed wikitext',

Follow-up revisions

RevisionCommit summaryAuthorDate
r77897Part of fix/workaround to bug 26163. Get the categories and languagelinks as...pdhanda19:47, 6 December 2010
r77943Merged r7789 from trunk - Part of fix/workaround to bug 26163. API calls to g...pdhanda01:17, 7 December 2010
r77944Merged r77897 and r77930 from trunk - Part of fix/workaround to bug 26163 ...pdhanda01:21, 7 December 2010

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r77516Fixes bug 26163 - Missing categories and iw links on diff pages.pdhanda00:12, 1 December 2010

Comments

#Comment by Catrope (talk | contribs)   18:43, 8 December 2010
+		global $wgOut, $wgUser;
+		$wgOut->addCategoryLinks( $categories );
+		$sk = $wgUser->getSkin();
+		return $sk->getCategories();

The fact that this is how you render category links is one of the most depressing examples of evil globals in MediaWiki :(

#Comment by Dantman (talk | contribs)   00:16, 6 February 2011

I don't like this addition to the api:

  • We already output category and langlink information in the api, any api caller should be using THAT data and generating their own html. NOT asking for skin-specific html to be magically generated for them with no explicit understanding of what skin they are in.
  • Skins should be free to style categories however they want, even free to completely omit that standard category block, or render it in ways that hacks like this may be unable to reproduce properly
  • Skin::otherLanguages is NOT used by any of our modern skins to generate language links. It's a holdover from the old skin system that we only keep around because we haven't removed standard/Classic, Nostalgia, and CologneBlue. It's the wrong thing to use when rendering a set of language links in modern skins like vector.
    • The use of Skin::otherLanguages here is resulting in an utter hack in 1.18 where we're trying to improve the skin system and modernize/exterminate the outdated skins.
    • bawolff also comments on how ugly this is.

I recommend we back this out of 1.17 before it goes stable and we have to permanently resort to hacks in core to maintain this undesirable feature.

Recommendations on how to deal with the FlaggedRevs workaround:

  • It's already a ridiculous hack hardcoding guesses like where a skin places the category box, hardcoding the actual markup and building it client side from the list of language links and categories that you get from the api isn't going to make much of a difference.
  • The use of ajax like this is introducing a usability barrier locking out users without js from viewing diff pages. Whatever limitation in core is making it necessary for FlaggedRevs to replace the diff page with an ajax diff page should be fixed and it should start rendering diff pages normally, where the skin system can actually render the category and language links the way they are expected to.

If we really absolutely need a system to let client side js update the list of categories and language links, it should be done by building an abstract api in MW's client side js to control parts of the ui like these, done in a way that recognizes that skins are free to render things in any way they want (ie: include fallbacks to render things simply below the content or whatnot if a skin-specific way can't be found), and support a simple api that custom skins can implement to allow that client side api to understand how it is supposed to update parts of the ui.

#Comment by RobLa-WMF (talk | contribs)   05:40, 6 February 2011

We're not reverting this for 1.17, because 1) we're only 2 days from the scheduled 1.17wmf1 deployment, and reverting this means we delay the 1.17, and 2) this was already deployed in the 1.16wmf4 branch and pushed in December, so this ship has already sailed. I'm adding the 1.18 tag to remove it from consideration for blocking the 1.17wmf1 deployment.

I'm going to leave the "fixme" tag on here for now, since I think it's reasonable to have a non-urgent discussion about this. However, I believe that this was an acceptable compromise, per my comments on r77516 and bug 26163.

#Comment by RobLa-WMF (talk | contribs)   04:09, 17 June 2011

This has been running in production long enough now that it's silly to treat this as a "fixme". Marking "ok".

#Comment by Duplicatebug (talk | contribs)   09:20, 6 February 2011

related: bug 26514

Status & tagging log