r70415 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r70414‎ | r70415 | r70416 >
Date:20:50, 3 August 2010
Author:simetrical
Status:ok (Comments)
Tags:
Comment:
Enable new category sort by default

Patch best viewed with whitespace changes ignored. This will doubtless
introduce a bunch of bugs. Please report any so I can fix them. If
they're big enough and the fix isn't obvious, please revert.
Modified paths:
  • /trunk/phase3/includes/CategoryPage.php (modified) (history)
  • /trunk/phase3/includes/DefaultSettings.php (modified) (history)
  • /trunk/phase3/includes/LinksUpdate.php (modified) (history)
  • /trunk/phase3/includes/installer/MysqlUpdater.php (modified) (history)
  • /trunk/phase3/maintenance/archives/patch-categorylinks-better-collation.sql (modified) (history)
  • /trunk/phase3/maintenance/updateCollation.php (modified) (history)
  • /trunk/phase3/maintenance/updaters.inc (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/archives/patch-categorylinks-better-collation.sql
@@ -1,12 +1,7 @@
22 --
33 -- patch-categorylinks-better-collation.sql
44 --
 5+-- Bugs 164, 1211, 23682.
56 ALTER TABLE categorylinks
67 ADD COLUMN cl_sortkey_prefix varchar(255) binary NOT NULL default '',
78 ADD COLUMN cl_collation tinyint NOT NULL default 0,
Index: trunk/phase3/maintenance/updateCollation.php
@@ -37,7 +37,7 @@
3838 __METHOD__
3939 );
4040
41 - $this->output( "Fixing around $count rows (estimate might be wrong).\n" );
 41+ $this->output( "Fixing collation for around $count rows (estimate might be wrong).\n" );
4242
4343 $count = 0;
4444 do {
Index: trunk/phase3/maintenance/updaters.inc
@@ -1086,6 +1086,21 @@
10871087 $task->execute();
10881088 }
10891089
 1090+function do_collation_update() {
 1091+ global $wgDatabase, $wgCollationVersion;
 1092+ if ( $wgDatabase->selectField(
 1093+ 'categorylinks',
 1094+ 'COUNT(*)',
 1095+ 'cl_collation != ' . $wgDatabase->addQuotes( $wgCollationVersion ),
 1096+ __FUNCTION__
 1097+ ) == 0 ) {
 1098+ wfOut( "...collations up-to-date.\n" );
 1099+ }
 1100+ require_once( 'updateCollation.php' );
 1101+ $task = new UpdateCollation();
 1102+ $task->execute();
 1103+}
 1104+
10901105 function sqlite_initial_indexes() {
10911106 global $wgDatabase;
10921107 // initial-indexes.sql fails if the indexes are already present, so we perform a quick check if our database is newer.
Index: trunk/phase3/includes/CategoryPage.php
@@ -51,17 +51,12 @@
5252 }
5353
5454 function closeShowCategory() {
55 - global $wgOut, $wgRequest, $wgExperimentalCategorySort;
 55+ global $wgOut, $wgRequest;
5656
57 - if ( $wgExperimentalCategorySort ) {
58 - $from = $until = array();
59 - foreach ( array( 'page', 'subcat', 'file' ) as $type ) {
60 - $from[$type] = $wgRequest->getVal( "{$type}from" );
61 - $until[$type] = $wgRequest->getVal( "{$type}until" );
62 - }
63 - } else {
64 - $from = $wgRequest->getVal( 'from' );
65 - $until = $wgRequest->getVal( 'until' );
 57+ $from = $until = array();
 58+ foreach ( array( 'page', 'subcat', 'file' ) as $type ) {
 59+ $from[$type] = $wgRequest->getVal( "{$type}from" );
 60+ $until[$type] = $wgRequest->getVal( "{$type}until" );
6661 }
6762
6863 $viewer = new CategoryViewer( $this->mTitle, $from, $until, $wgRequest->getValues() );
@@ -184,7 +179,7 @@
185180 * else use sortkey...
186181 */
187182 function getSubcategorySortChar( $title, $sortkey ) {
188 - global $wgContLang, $wgExperimentalCategorySort;
 183+ global $wgContLang;
189184
190185 if ( $title->getPrefixedText() == $sortkey ) {
191186 $word = $title->getDBkey();
@@ -192,11 +187,7 @@
193188 $word = $sortkey;
194189 }
195190
196 - if ( $wgExperimentalCategorySort ) {
197 - $firstChar = $wgContLang->firstLetterForLists( $word );
198 - } else {
199 - $firstChar = $wgContLang->firstChar( $word );
200 - }
 191+ $firstChar = $wgContLang->firstLetterForLists( $word );
201192
202193 return $wgContLang->convert( $firstChar );
203194 }
@@ -206,12 +197,7 @@
207198 */
208199 function addImage( Title $title, $sortkey, $pageLength, $isRedirect = false ) {
209200 if ( $this->showGallery ) {
210 - global $wgExperimentalCategorySort;
211 - if ( $wgExperimentalCategorySort ) {
212 - $flip = $this->flip['file'];
213 - } else {
214 - $flip = $this->flip;
215 - }
 201+ $flip = $this->flip['file'];
216202 if ( $flip ) {
217203 $this->gallery->insert( $title );
218204 } else {
@@ -226,7 +212,7 @@
227213 * Add a miscellaneous page
228214 */
229215 function addPage( $title, $sortkey, $pageLength, $isRedirect = false ) {
230 - global $wgContLang, $wgExperimentalCategorySort;
 216+ global $wgContLang;
231217 $this->articles[] = $isRedirect
232218 ? '<span class="redirect-in-category">' .
233219 $this->getSkin()->link(
@@ -238,29 +224,22 @@
239225 ) . '</span>'
240226 : $this->getSkin()->link( $title );
241227
242 - if ( $wgExperimentalCategorySort ) {
243 - $this->articles_start_char[] = $wgContLang->convert( $wgContLang->firstLetterForLists( $sortkey ) );
244 - } else {
245 - $this->articles_start_char[] = $wgContLang->convert( $wgContLang->firstChar( $sortkey ) );
246 - }
 228+ $this->articles_start_char[] = $wgContLang->convert( $wgContLang->firstLetterForLists( $sortkey ) );
247229 }
248230
249231 function finaliseCategoryState() {
250 - global $wgExperimentalCategorySort;
251 - if ( ( !$wgExperimentalCategorySort && $this->flip )
252 - || ( $wgExperimentalCategorySort && $this->flip['subcat'] ) ) {
 232+ if ( $this->flip['subcat'] ) {
253233 $this->children = array_reverse( $this->children );
254234 $this->children_start_char = array_reverse( $this->children_start_char );
255235 }
256 - if ( ( !$wgExperimentalCategorySort && $this->flip )
257 - || ( $wgExperimentalCategorySort && $this->flip['page'] ) ) {
 236+ if ( $this->flip['page'] ) {
258237 $this->articles = array_reverse( $this->articles );
259238 $this->articles_start_char = array_reverse( $this->articles_start_char );
260239 }
261240 }
262241
263242 function doCategoryQuery() {
264 - global $wgExperimentalCategorySort, $wgContLang;
 243+ global $wgContLang;
265244
266245 $dbr = wfGetDB( DB_SLAVE, 'category' );
267246
@@ -276,109 +255,58 @@
277256 $joins = array( 'categorylinks' => array( 'INNER JOIN', 'cl_from = page_id' ),
278257 'category' => array( 'LEFT JOIN', 'cat_title = page_title AND page_namespace = ' . NS_CATEGORY ) );
279258
280 - if ( $wgExperimentalCategorySort ) {
281 - # Copy-pasted from below, but that's okay, because the stuff below
282 - # will be deleted when this becomes the default.
283 - $this->nextPage = array(
284 - 'page' => null,
285 - 'subcat' => null,
286 - 'file' => null,
287 - );
288 - $this->flip = array( 'page' => false, 'subcat' => false, 'file' => false );
 259+ $this->nextPage = array(
 260+ 'page' => null,
 261+ 'subcat' => null,
 262+ 'file' => null,
 263+ );
 264+ $this->flip = array( 'page' => false, 'subcat' => false, 'file' => false );
289265
290 - foreach ( array( 'page', 'subcat', 'file' ) as $type ) {
291 - # Get the sortkeys for start/end, if applicable. Note that if
292 - # the collation in the database differs from the one
293 - # $wgContLang is using, pagination might go totally haywire.
294 - $extraConds = array( 'cl_type' => $type );
295 - if ( $this->from[$type] !== null ) {
296 - $extraConds[] = 'cl_sortkey >= '
297 - . $dbr->addQuotes( $wgContLang->convertToSortkey( $this->from[$type] ) );
298 - } elseif ( $this->until[$type] !== null ) {
299 - $extraConds[] = 'cl_sortkey < '
300 - . $dbr->addQuotes( $wgContLang->convertToSortkey( $this->until[$type] ) );
301 - $this->flip[$type] = true;
302 - }
 266+ foreach ( array( 'page', 'subcat', 'file' ) as $type ) {
 267+ # Get the sortkeys for start/end, if applicable. Note that if
 268+ # the collation in the database differs from the one
 269+ # $wgContLang is using, pagination might go totally haywire.
 270+ $extraConds = array( 'cl_type' => $type );
 271+ if ( $this->from[$type] !== null ) {
 272+ $extraConds[] = 'cl_sortkey >= '
 273+ . $dbr->addQuotes( $wgContLang->convertToSortkey( $this->from[$type] ) );
 274+ } elseif ( $this->until[$type] !== null ) {
 275+ $extraConds[] = 'cl_sortkey < '
 276+ . $dbr->addQuotes( $wgContLang->convertToSortkey( $this->until[$type] ) );
 277+ $this->flip[$type] = true;
 278+ }
303279
304 - $res = $dbr->select(
305 - $tables,
306 - array_merge( $fields, array( 'cl_sortkey_prefix' ) ),
307 - $conds + $extraConds,
308 - __METHOD__,
309 - $opts + array( 'ORDER BY' => $this->flip[$type] ? 'cl_sortkey DESC' : 'cl_sortkey' ),
310 - $joins
311 - );
 280+ $res = $dbr->select(
 281+ $tables,
 282+ array_merge( $fields, array( 'cl_sortkey_prefix' ) ),
 283+ $conds + $extraConds,
 284+ __METHOD__,
 285+ $opts + array( 'ORDER BY' => $this->flip[$type] ? 'cl_sortkey DESC' : 'cl_sortkey' ),
 286+ $joins
 287+ );
312288
313 - $count = 0;
314 - foreach ( $res as $row ) {
315 - $title = Title::newFromRow( $row );
316 - $rawSortkey = $title->getCategorySortkey( $row->cl_sortkey_prefix );
 289+ $count = 0;
 290+ foreach ( $res as $row ) {
 291+ $title = Title::newFromRow( $row );
 292+ $rawSortkey = $title->getCategorySortkey( $row->cl_sortkey_prefix );
317293
318 - if ( ++$count > $this->limit ) {
319 - # We've reached the one extra which shows that there
320 - # are additional pages to be had. Stop here...
321 - $this->nextPage[$type] = $rawSortkey;
322 - break;
323 - }
 294+ if ( ++$count > $this->limit ) {
 295+ # We've reached the one extra which shows that there
 296+ # are additional pages to be had. Stop here...
 297+ $this->nextPage[$type] = $rawSortkey;
 298+ break;
 299+ }
324300
325 - if ( $title->getNamespace() == NS_CATEGORY ) {
326 - $cat = Category::newFromRow( $row, $title );
327 - $this->addSubcategoryObject( $cat, $rawSortkey, $row->page_len );
328 - } elseif ( $this->showGallery && $title->getNamespace() == NS_FILE ) {
329 - $this->addImage( $title, $rawSortkey, $row->page_len, $row->page_is_redirect );
330 - } else {
331 - $this->addPage( $title, $rawSortkey, $row->page_len, $row->page_is_redirect );
332 - }
 301+ if ( $title->getNamespace() == NS_CATEGORY ) {
 302+ $cat = Category::newFromRow( $row, $title );
 303+ $this->addSubcategoryObject( $cat, $rawSortkey, $row->page_len );
 304+ } elseif ( $this->showGallery && $title->getNamespace() == NS_FILE ) {
 305+ $this->addImage( $title, $rawSortkey, $row->page_len, $row->page_is_redirect );
 306+ } else {
 307+ $this->addPage( $title, $rawSortkey, $row->page_len, $row->page_is_redirect );
333308 }
334309 }
335 -
336 - return;
337310 }
338 -
339 - # Non-$wgExperimentalCategorySort stuff
340 -
341 - if ( $this->from != '' ) {
342 - $pageCondition = 'cl_sortkey >= ' . $dbr->addQuotes( $this->from );
343 - $this->flip = false;
344 - } elseif ( $this->until != '' ) {
345 - $pageCondition = 'cl_sortkey < ' . $dbr->addQuotes( $this->until );
346 - $this->flip = true;
347 - } else {
348 - $pageCondition = '1 = 1';
349 - $this->flip = false;
350 - }
351 -
352 - $res = $dbr->select(
353 - $tables,
354 - $fields,
355 - $conds + array( $pageCondition ),
356 - __METHOD__,
357 - $opts + array( 'ORDER BY' => $this->flip ? 'cl_sortkey DESC' : 'cl_sortkey' ),
358 - $joins
359 - );
360 -
361 - $count = 0;
362 - $this->nextPage = null;
363 -
364 - foreach ( $res as $row ) {
365 - if ( ++$count > $this->limit ) {
366 - // We've reached the one extra which shows that there are
367 - // additional pages to be had. Stop here...
368 - $this->nextPage = $row->cl_sortkey;
369 - break;
370 - }
371 -
372 - $title = Title::newFromRow( $row );
373 -
374 - if ( $title->getNamespace() == NS_CATEGORY ) {
375 - $cat = Category::newFromRow( $row, $title );
376 - $this->addSubcategoryObject( $cat, $row->cl_sortkey, $row->page_len );
377 - } elseif ( $this->showGallery && $title->getNamespace() == NS_FILE ) {
378 - $this->addImage( $title, $row->cl_sortkey, $row->page_len, $row->page_is_redirect );
379 - } else {
380 - $this->addPage( $title, $row->cl_sortkey, $row->page_len, $row->page_is_redirect );
381 - }
382 - }
383311 }
384312
385313 function getCategoryTop() {
@@ -460,10 +388,6 @@
461389 * @return String: HTML output, possibly empty if there are no other pages
462390 */
463391 private function getSectionPagingLinks( $type ) {
464 - global $wgExperimentalCategorySort;
465 - if ( !$wgExperimentalCategorySort ) {
466 - return '';
467 - }
468392 if ( $this->until[$type] !== null ) {
469393 return $this->pagingLinks( $this->nextPage[$type], $this->until[$type], $type );
470394 } elseif ( $this->nextPage[$type] !== null || $this->from[$type] !== null ) {
@@ -474,18 +398,7 @@
475399 }
476400
477401 function getCategoryBottom() {
478 - global $wgExperimentalCategorySort;
479 - if ( $wgExperimentalCategorySort ) {
480 - # We have per-section paging links, no global ones.
481 - return '';
482 - }
483 - if ( $this->until != '' ) {
484 - return $this->pagingLinks( $this->nextPage, $this->until );
485 - } elseif ( $this->nextPage != '' || $this->from != '' ) {
486 - return $this->pagingLinks( $this->from, $this->nextPage );
487 - } else {
488 - return '';
489 - }
 402+ return '';
490403 }
491404
492405 /**
Index: trunk/phase3/includes/installer/MysqlUpdater.php
@@ -158,6 +158,8 @@
159159 array( 'add_field', 'interwiki', 'iw_api', 'patch-iw_api_and_wikiid.sql' ),
160160 array( 'drop_index_if_exists', 'iwlinks', 'iwl_prefix', 'patch-kill-iwl_prefix.sql' ),
161161 array( 'drop_index_if_exists', 'iwlinks', 'iwl_prefix_from_title', 'patch-kill-iwl_pft.sql' ),
 162+ array( 'add_field', 'categorylinks', 'cl_collation', 'patch-categorylinks-better-collation.sql' ),
 163+ array( 'do_collation_update' ),
162164 ),
163165 );
164166 }
Index: trunk/phase3/includes/LinksUpdate.php
@@ -426,57 +426,48 @@
427427 * @private
428428 */
429429 function getCategoryInsertions( $existing = array() ) {
430 - global $wgContLang, $wgExperimentalCategorySort, $wgCollationVersion;
 430+ global $wgContLang, $wgCollationVersion;
431431 $diffs = array_diff_assoc( $this->mCategories, $existing );
432432 $arr = array();
433433 foreach ( $diffs as $name => $sortkey ) {
434434 $nt = Title::makeTitleSafe( NS_CATEGORY, $name );
435435 $wgContLang->findVariantLink( $name, $nt, true );
436436
437 - if ( $wgExperimentalCategorySort ) {
438 - if ( $this->mTitle->getNamespace() == NS_CATEGORY ) {
439 - $type = 'subcat';
440 - } elseif ( $this->mTitle->getNamespace() == NS_FILE ) {
441 - $type = 'file';
442 - } else {
443 - $type = 'page';
444 - }
 437+ if ( $this->mTitle->getNamespace() == NS_CATEGORY ) {
 438+ $type = 'subcat';
 439+ } elseif ( $this->mTitle->getNamespace() == NS_FILE ) {
 440+ $type = 'file';
 441+ } else {
 442+ $type = 'page';
 443+ }
445444
446 - # TODO: This is kind of wrong, because someone might set a sort
447 - # key prefix that's the same as the default sortkey for the
448 - # title. This should be fixed by refactoring code to replace
449 - # $sortkey in this array by a prefix, but it's basically harmless
450 - # (Title::moveTo() has had the same issue for a long time).
451 - if ( $this->mTitle->getCategorySortkey() == $sortkey ) {
452 - $prefix = '';
453 - $sortkey = $wgContLang->convertToSortkey( $sortkey );
454 - } else {
455 - # Treat custom sortkeys as a prefix, so that if multiple
456 - # things are forced to sort as '*' or something, they'll
457 - # sort properly in the category rather than in page_id
458 - # order or such.
459 - $prefix = $sortkey;
460 - $sortkey = $wgContLang->convertToSortkey(
461 - $this->mTitle->getCategorySortkey( $prefix ) );
462 - }
463 -
464 - $arr[] = array(
465 - 'cl_from' => $this->mId,
466 - 'cl_to' => $name,
467 - 'cl_sortkey' => $sortkey,
468 - 'cl_timestamp' => $this->mDb->timestamp(),
469 - 'cl_sortkey_prefix' => $prefix,
470 - 'cl_collation' => $wgCollationVersion,
471 - 'cl_type' => $type,
472 - );
 445+ # TODO: This is kind of wrong, because someone might set a sort
 446+ # key prefix that's the same as the default sortkey for the
 447+ # title. This should be fixed by refactoring code to replace
 448+ # $sortkey in this array by a prefix, but it's basically harmless
 449+ # (Title::moveTo() has had the same issue for a long time).
 450+ if ( $this->mTitle->getCategorySortkey() == $sortkey ) {
 451+ $prefix = '';
 452+ $sortkey = $wgContLang->convertToSortkey( $sortkey );
473453 } else {
474 - $arr[] = array(
475 - 'cl_from' => $this->mId,
476 - 'cl_to' => $name,
477 - 'cl_sortkey' => $sortkey,
478 - 'cl_timestamp' => $this->mDb->timestamp()
479 - );
 454+ # Treat custom sortkeys as a prefix, so that if multiple
 455+ # things are forced to sort as '*' or something, they'll
 456+ # sort properly in the category rather than in page_id
 457+ # order or such.
 458+ $prefix = $sortkey;
 459+ $sortkey = $wgContLang->convertToSortkey(
 460+ $this->mTitle->getCategorySortkey( $prefix ) );
480461 }
 462+
 463+ $arr[] = array(
 464+ 'cl_from' => $this->mId,
 465+ 'cl_to' => $name,
 466+ 'cl_sortkey' => $sortkey,
 467+ 'cl_timestamp' => $this->mDb->timestamp(),
 468+ 'cl_sortkey_prefix' => $prefix,
 469+ 'cl_collation' => $wgCollationVersion,
 470+ 'cl_type' => $type,
 471+ );
481472 }
482473 return $arr;
483474 }
Index: trunk/phase3/includes/DefaultSettings.php
@@ -4473,16 +4473,6 @@
44744474 $wgCategoryPrefixedDefaultSortkey = true;
44754475
44764476 /**
4477 - * Enable experimental support for non-braindead collation on category pages.
4478 - * For this to work, you need to alter your categorylinks table by applying
4479 - * maintenance/archives/patch-categorylinks-better-collation.sql, then keep
4480 - * up-to-date with changes that are made to that file (they won't be
4481 - * automatically applied). You should also set $wgUseDumbLinkUpdate = true and
4482 - * run maintenance/refreshLinks.php.
4483 - */
4484 -$wgExperimentalCategorySort = false;
4485 -
4486 -/**
44874477 * A version indicator for collations that will be stored in cl_collation for
44884478 * all new rows. Used when the collation algorithm changes: a script checks
44894479 * for all rows where cl_collation != $wgCollationVersion and regenerates

Follow-up revisions

RevisionCommit summaryAuthorDate
r79706(Bug 25254) cl_timestamp gets updated on null edit where it shouldn't....bawolff02:42, 6 January 2011
r80432(bug 26737; follow-up r70415) Make new category stuff play nice with __NOGALL...bawolff01:16, 17 January 2011
r80433(follow-up r70415) Fixes the function that determines if category counts are ...bawolff02:27, 17 January 2011

Comments

#Comment by Simetrical (talk | contribs)   23:07, 5 January 2011

Bug 25254 needs to block deployment of this. It should be fairly simple to fix, but I'm not sure I'm going to get around to it in the immediate future, so if someone else wants to take a shot, please feel free.

#Comment by Catrope (talk | contribs)   23:12, 5 January 2011

Either that, or it needs to be disableable.

#Comment by Simetrical (talk | contribs)   23:57, 5 January 2011

Possibly not practical, given that it involves schema changes. I can't remember if they'd be reverse-compatible, offhand.

#Comment by Bawolff (talk | contribs)   02:45, 6 January 2011

fixed issue in r79706. Resetting this revision to new

#Comment by Tim Starling (talk | contribs)   06:05, 20 January 2011

The ops team has asked that we deploy this feature separately from the resource loader, i.e. at a later time. This is to allow us to downgrade to 1.16 should the resource loader cause serious ops problems. This means either re-adding a global variable to switch it off, or temporarily patching the feature out of 1.17wmf1. I'm leaning towards patching it out.

#Comment by Simetrical (talk | contribs)   12:02, 21 January 2011

I removed the global variable because there were just way too many places where code had to duplicated wholesale, as you can see from the diff. But either way it will be messy, so I don't know which way is better. Take your pick.

Status & tagging log