r80436 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r80435‎ | r80436 | r80437 >
Date:06:27, 17 January 2011
Author:bawolff
Status:ok (Comments)
Tags:
Comment:
Change the default collation from strtoupper to Language::uc, so that non-ascii characters get to play too.

I know the uppercase thing is just a standby until a real collation function is written. However in the
mean time, i think it'd be really weird for a wiki with $wgCapitalLinks = false to suddenly have
[[a]] and [[A]] sort under the same letter in a category page, but [[Ä]] and [[ä]] sort no where
near each other, even though on a capitalized wiki they would be the same page.

See discussion on r69816.

Also fix an issue with maintenance/updateCollation.php, where php thinks
that 'uppercase' == 0 (?!). I don't really know what the deal with that
is, but using a ! instead of == 0 seems to fix it. (Follow-up r69961)
Modified paths:
  • /trunk/phase3/includes/DefaultSettings.php (modified) (history)
  • /trunk/phase3/languages/Language.php (modified) (history)
  • /trunk/phase3/maintenance/updateCollation.php (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/updateCollation.php
——@@ -80,7 +80,7 @@
8181 $dbw->begin();
8282 foreach ( $res as $row ) {
8383 $title = Title::newFromRow( $row );
84 - if ( $row->cl_collation == 0 ) {
 84+ if ( !$row->cl_collation ) {
8585 # This is an old-style row, so the sortkey needs to be
8686 # converted.
8787 if ( $row->cl_sortkey == $title->getText()
Index: trunk/phase3/includes/DefaultSettings.php
——@@ -4616,10 +4616,10 @@
46174617 * for all rows where cl_collation != $wgCategoryCollation and regenerates
46184618 * cl_sortkey based on the page name and cl_sortkey_prefix.
46194619 *
4620 - * Currently only supports 'uppercase', which just uppercases the string. This
 4620+ * Currently only supports 'uppercase2', which just uppercases the string. This
46214621 * is a dummy collation, to be replaced later by real ones.
46224622 */
4623 -$wgCategoryCollation = 'uppercase';
 4623+$wgCategoryCollation = 'uppercase2';
46244624
46254625 /** @} */ # End categories }
46264626
Index: trunk/phase3/languages/Language.php
——@@ -3010,7 +3010,7 @@
30113011 */
30123012 public function convertToSortkey( $string ) {
30133013 # Fake function for now
3014 - return strtoupper( $string );
 3014+ return $this->uc( $string );
30153015 }
30163016
30173017 /**
——@@ -3040,6 +3040,6 @@
30413041 if ( $string[0] == "\0" ) {
30423042 $string = substr( $string, 1 );
30433043 }
3044 - return strtoupper( $this->firstChar( $string ) );
 3044+ return $this->uc( $this->firstChar( $string ) );
30453045 }
30463046 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r80443* Introduced a non-dummy collation for $wgCategoryCollation, namely UCA with ...tstarling14:02, 17 January 2011
r805501.17: MFT r80106, r80137, r80138, r80205, r80210, r80222, r80223, r80231, r80...catrope02:09, 19 January 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r69816Add non-identity collation, with migration script...simetrical20:58, 23 July 2010
r69961Reconcept cl_raw_sortkey as cl_sortkey_prefix...simetrical19:27, 26 July 2010

Comments

#Comment by Tim Starling (talk | contribs)   14:03, 17 January 2011

'uppercase' is zero, see Manual:Coding conventions#PHP pitfalls

#Comment by Simetrical (talk | contribs)   16:27, 17 January 2011

The "$row->cl_collation == 0" check is a holdover from when cl_collation was numeric. It should be "$row->cl_collation === " for maximum correctness -- the current code will incorrectly handle a collation named '0' (which is admittedly unlikely, so I'm not going to mark fixme). Never treat PHP strings as booleans, always explicitly compare to the empty string!

Otherwise this looks correct.

#Comment by Simetrical (talk | contribs)   16:28, 17 January 2011

I really want to find whoever designed the Code Review comment system IRL so I can whack them over the head repeatedly with a heavy object. I think you get what I meant, anyway (insert '' where the italics start).

#Comment by Catrope (talk | contribs)   18:03, 18 January 2011

That would be Brion Vibber ;)

#Comment by Brion VIBBER (talk | contribs)   18:05, 18 January 2011

Yeah it's... a bit flaky as I had to throw it together since LQT wasn't ready to integrate at the time. Alas, the double-single-quotes problem is just from using wikitext which has that silly italics syntax... :D

#Comment by Brion VIBBER (talk | contribs)   18:06, 18 January 2011

though i will point out that there is a preview button *ahem* :D

#Comment by Simetrical (talk | contribs)   00:46, 19 January 2011

Preview, but no edit. On a wiki . . .

Previewing is bad UI. You should be able to do something and then reverse it if it's bad, not be required to take an extra step beforehand. Like if the user takes some destructive action, you shouldn't prompt for confirmation, you should instead give them an undo button. Gmail's "undo send e-mail" feature is a nice implementation of this.

But yeah, I'm just whining, not volunteering to fix it.  ;)

Status & tagging log