r79706 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r79705‎ | r79706 | r79707 >
Date:02:42, 6 January 2011
Author:bawolff
Status:resolved (Comments)
Tags:
Comment:
(Bug 25254) cl_timestamp gets updated on null edit where it shouldn't.
Caused by two things:
*after the new category collation stuff, the if this category has changed
code was comparing the user supplied sortkey, with the collation sortable
sortkey, which didn't work. (See also CR for r70415) This is new issue in 1.17
*If the sortkey was too long (long enough to get truncated by DB), then the
did this sortkey change code also failed since it was comparing the
non-truncated sortkey to the truncated sortkey. This issue is present
in older (all previous?) versions of mediawiki. This issue especially affects
non-latin wikis that use 3-byte characters.

This bug primarily is an issue for people using DPL type extension.

I wasn't sure if i should add RELEASE-NOTES, since i'm tagging this 1.17
Modified paths:
  • /trunk/phase3/includes/LinksUpdate.php (modified) (history)
  • /trunk/phase3/maintenance/archives/patch-categorylinks-better-collation.sql (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/archives/patch-categorylinks-better-collation.sql
——@@ -4,6 +4,10 @@
55 -- Bugs 164, 1211, 23682. This is the second version of this patch; the
66 -- changes are also incorporated into patch-categorylinks-better-collation2.sql,
77 -- for the benefit of trunk users who applied the original.
 8+--
 9+-- Due to bug 25254, the length limit of 255 bytes for cl_sortkey_prefix
 10+-- is also enforced in php. If you change the length of that field, make
 11+-- sure to also change the check in LinksUpdate.php.
812 ALTER TABLE /*$wgDBprefix*/categorylinks
913 CHANGE COLUMN cl_sortkey cl_sortkey varbinary(230) NOT NULL default '',
1014 ADD COLUMN cl_sortkey_prefix varchar(255) binary NOT NULL default '',
Index: trunk/phase3/includes/LinksUpdate.php
——@@ -67,6 +67,14 @@
6868 $this->mInterlangs[$key] = $title;
6969 }
7070
 71+ foreach ( $this->mCategories as $cat => &$sortkey ) {
 72+ # If the sortkey is longer then 255 bytes,
 73+ # it truncated by DB, and then doesn't get
 74+ # matched when comparing existing vs current
 75+ # categories, causing bug 25254.
 76+ $sortkey = substr( $sortkey, 0, 255 );
 77+ }
 78+
7179 $this->mRecursive = $recursive;
7280
7381 wfRunHooks( 'LinksUpdateConstructed', array( &$this ) );
——@@ -687,11 +695,15 @@
688696 * @private
689697 */
690698 function getExistingCategories() {
691 - $res = $this->mDb->select( 'categorylinks', array( 'cl_to', 'cl_sortkey' ),
 699+ $res = $this->mDb->select( 'categorylinks', array( 'cl_to', 'cl_sortkey_prefix' ),
692700 array( 'cl_from' => $this->mId ), __METHOD__, $this->mOptions );
693701 $arr = array();
694702 foreach ( $res as $row ) {
695 - $arr[$row->cl_to] = $row->cl_sortkey;
 703+ if ( $row->cl_sortkey_prefix !== '' ) {
 704+ $arr[$row->cl_to] = $row->cl_sortkey_prefix;
 705+ } else {
 706+ $arr[$row->cl_to] = $this->mTitle->getCategorySortkey();
 707+ }
696708 }
697709 return $arr;
698710 }

Sign-offs

UserFlagDate
Hasharinspected09:10, 5 June 2011

Follow-up revisions

RevisionCommit summaryAuthorDate
r797201.17: MFT r79578, r79591, r79593, r79595, r79649, r79650, r79651, r79653, r79...catrope14:48, 6 January 2011
r79763follow-up r79706 (the cl_timestamp updating on null edit bug) add comment to...bawolff22:09, 6 January 2011
r81554(follow up r79706 to address CR comments) Simplify some of the logic in Links...bawolff02:16, 5 February 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r70415Enable new category sort by default...simetrical20:50, 3 August 2010

Comments

#Comment by Nikerabbit (talk | contribs)   08:32, 6 January 2011

Surely that patch file is never going to be changed, but instead the main table definition file is and a new patch is created if needed. Thus the comment seems a bit misplaced.

#Comment by Simetrical (talk | contribs)   20:39, 6 January 2011

Thanks for the fix.

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

Why does it put $this->mTitle->getCategorySortkey() in the existing array when cl_sortkey_prefix is empty? Why can't you just do 

Index: LinksUpdate.php
===================================================================
--- LinksUpdate.php	(revision 80613)
+++ LinksUpdate.php	(working copy)
@@ -452,7 +452,7 @@
 			# title.  This should be fixed by refactoring code to replace
 			# $sortkey in this array by a prefix, but it's basically harmless
 			# (Title::moveTo() has had the same issue for a long time).
-			if ( $this->mTitle->getCategorySortkey() == $sortkey ) {
+			if ( '' === $sortkey ) {
 				$prefix = '';
 				$sortkey = Collation::singleton()->getSortKey( $sortkey );
 			} else {
@@ -699,11 +699,7 @@
 			array( 'cl_from' => $this->mId ), __METHOD__, $this->mOptions );
 		$arr = array();
 		foreach ( $res as $row ) {
-			if ( $row->cl_sortkey_prefix !== '' ) {
-				$arr[$row->cl_to] = $row->cl_sortkey_prefix;
-			} else {
-				$arr[$row->cl_to] = $this->mTitle->getCategorySortkey();
-			}
+			$arr[$row->cl_to] = $row->cl_sortkey_prefix;
 		}
 		return $arr;
 	}

?

#Comment by Bawolff (talk | contribs)   00:09, 23 January 2011

Because the array output by ParserOutput::getCategories has the pagename as the value of the sortkey for categories that don't specify one, so just doing the above changes would mess up the getCategoryInsertions method of LinksUpdate.

However, I suppose it would make sense to change how the parser determines sortkeys, so that is not an issue.

#Comment by Bawolff (talk | contribs)   02:16, 5 February 2011

Done in r81554

resetting revision status to new.

Status & tagging log