r86100 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r86099‎ | r86100 | r86101 >
Date:23:28, 14 April 2011
Author:bawolff
Status:resolved (Comments)
Tags:
Comment:
(bug 28540) revert r83544 - It really breaks things when using uca-default collation (As noticed on tw)

Various functions (particuarly the getFirstCharacter ones) all expect to have the human readable sortkey.
Passing them the binary sortkey results in the getFirstChar headers to be totally wrong. It also
causes paging not to work since the sortkey gets double encoded.

This issue probably wasn't noticed, since it wouldn't be visible on the uppercase collation
as uppercasing the first letter won't affect the first character headings, nor does double uppercasing
something matter.

Also changed one of the variable names from $rawSortkey to $humanSortkey as it confused me
(raw as in before turning to binary, or raw as in the real non-human readable sortkey)
Modified paths:
  • /trunk/phase3/includes/CategoryPage.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/CategoryPage.php
——@@ -244,7 +244,7 @@
245245 * else use sortkey...
246246 *
247247 * @param Title $title
248 - * @param string $sortkey
 248+ * @param string $sortkey The human-readable sortkey (before transforming to icu or whatever).
249249 */
250250 function getSubcategorySortChar( $title, $sortkey ) {
251251 global $wgContLang;
——@@ -364,22 +364,22 @@
365365 $count = 0;
366366 foreach ( $res as $row ) {
367367 $title = Title::newFromRow( $row );
368 - $rawSortkey = $row->cl_sortkey;
 368+ $humanSortkey = $title->getCategorySortkey( $row->cl_sortkey_prefix );
369369
370370 if ( ++$count > $this->limit ) {
371371 # We've reached the one extra which shows that there
372372 # are additional pages to be had. Stop here...
373 - $this->nextPage[$type] = $rawSortkey;
 373+ $this->nextPage[$type] = $humanSortkey;
374374 break;
375375 }
376376
377377 if ( $title->getNamespace() == NS_CATEGORY ) {
378378 $cat = Category::newFromRow( $row, $title );
379 - $this->addSubcategoryObject( $cat, $rawSortkey, $row->page_len );
 379+ $this->addSubcategoryObject( $cat, $humanSortkey, $row->page_len );
380380 } elseif ( $title->getNamespace() == NS_FILE ) {
381 - $this->addImage( $title, $rawSortkey, $row->page_len, $row->page_is_redirect );
 381+ $this->addImage( $title, $humanSortkey, $row->page_len, $row->page_is_redirect );
382382 } else {
383 - $this->addPage( $title, $rawSortkey, $row->page_len, $row->page_is_redirect );
 383+ $this->addPage( $title, $humanSortkey, $row->page_len, $row->page_is_redirect );
384384 }
385385 }
386386 }

Sign-offs

UserFlagDate
Simetricalinspected21:48, 15 April 2011

Follow-up revisions

RevisionCommit summaryAuthorDate
r86121(follow-up r86100) Make categorypage not die as much while transitioning betw...bawolff17:23, 15 April 2011
r864641.17wmf1: MFT r85377, r85555, r85583, r86100, r86121, r86130, r86142, r86146,...catrope11:27, 20 April 2011
r864741.17: MFT r81731, r85377, r85547, r85555, r85583, r85803, r85881, r86100, r86...catrope13:22, 20 April 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r83544Don't regenerate raw sortkeys in CategoryPage.php...simetrical20:49, 8 March 2011

Comments

#Comment by Bawolff (talk | contribs)   23:32, 14 April 2011

tagging 1.17wmf1 - I presume at some point wikimedia will switch to uca-default, in which case it will need this.

#Comment by Simetrical (talk | contribs)   16:16, 15 April 2011

This is wrong for the reason I gave in the r83544 commit summary. I'd suggest that you use cl_sortkey if cl_collation is the empty string, otherwise use getCategorySortkey() as at present. Otherwise it will break if the categorylinks migration script hasn't yet run, or hasn't yet finished running.

You're right that my commit was wrong, though. Apparently I misunderstood "raw sortkey" myself, although I wrote the code. I thought it meant "possibly binary" rather than "unprocessed". I'd suggest that someone look through all the sortkey stuff and make sure it's clear in all cases whether you're dealing with 1) cl_sortkey_prefix, 2) the output of Title::getCategorySortkey(), or 3) cl_sortkey. Otherwise you'll probably see more confusion and bugs like this.

#Comment by Bawolff (talk | contribs)   17:24, 15 April 2011

Sounds like a good plan. Did that in r86121.

resetting revision to new

Status & tagging log