r80443 MediaWiki - Code Review archive

What provides the Collator class?

intl. That's why it's mentioned in DefaultSettings.php and the commit message, and an exception is thrown if it isn't present.

Can we maybe backport this to 1.17 but keep using the uppercase collation for now?

Will do once reviewed.

I'm not sure of what this code does, but is the IcuCollation class capable of sorting e.g. "Aritmética" and "Álgebra" under "A" in Portuguese (Bug 24142)? Or would we need to create new classes for specific languages?

PS: Should that bug be marked as depending on bug 164?

Yes, IcuCollation can do that. For some information about other things it can do, my mailing list post is probably a good starting point: http://lists.wikimedia.org/pipermail/wikitech-l/2011-January/051228.html

Great!

Are there plans of changing wgCategoryCollation to 'uca-default' on Wikimedia wikis?

Also, can bug 24142 be marked as being fixed in this revision?

$wgCategoryCollation is currently set to 'uppercase' on WMF. I couldn't set it to 'uca-default' because the intl extension for PHP isn't installed on our servers. I hear this is not very easy to do until we upgrade the Apaches to lucid, but maybe Tim knows a way around that. I'm also not entirely sure Tim wanted to deploy uca-default WMF-wide; he seemed to indicate that but didn't go into detail. I'll try to get hold of him and ask him about both of those things.

I duped bug 24142 to bug 164

+				/*
+				printf( "Out of order: U+%05X > U+%05X\n",
+					utf8ToCodepoint( $prevChar ),
+					utf8ToCodepoint( $char ) );
+				 */

Debugging code?

+	 * corresponds to a logical sort of the corresponding strings.  Current
+	 * code expects that a null character should sort before all others, but
+	 * has no other particular expectations (and that one can be changed if
+	 * necessary).

You're now using newlines instead of nulls, so this is probably wrong, right?

+		if ( $string[0] == "\0" ) {
+			$string = substr( $string, 1 );
+		}

Is this needed after the null->newline change?

+		$key = $this->mainCollator->getSortKey( $string ) . '';

Why are you appending an empty string? To force the result to a string or something?

Looks OK otherwise, marking as such. If the null vs. newline thing turns out to be an issue, that can be fixed later.

The out of order character list is a particularly useful kind of debug output, which I thought might be useful for future developers, which is why I left it in (but commented-out).

Yes the comment is wrong, like the one that you noted on r80614.

I think stripping the null or newline from the start of the string in getFirstLetter() is unnecessary, but I have asked Aryeh on CR r69977 to make sure. I am waiting for his response.

When a PHP string is constructed by an extension, the memory allocated for the string should have one extra byte in it beyond the string length, which should be set to null. This prevents naive extensions interpreting PHP strings as C strings from running off the end of the string into security-relevant or invalid memory. When PHP is compiled with --enable-debug, every access to a string checks whether this null character is present, and raises a warning if it is not. The intl extension (and xdiff, see HistoryBlob.php) incorrectly generates strings without a trailing null character. Appending an empty string causes the null character to be added, thus preventing any other accesses of the same string from raising a warning.

Alright, that answers all my questions.

This is fine to go in 1.17, but in the longer term the inaccurate comments should be corrected. Also, the apppending-empty-string-to-ensure-null-termination thing is so obscure and pointless-looking to someone unfamiliar with PHP's internals that a quick comment explaining what's going on would be very useful.

I do not know enough about php, but in my opinion it is better to assign Collation::singleton() in updateCollation.php to a local var and reuse that, instead of getting it over and over in a nested loop. Or is there no difference in php? Thanks for explanation.

That's a micro-optimization that's hardly relevant in a loop that executes two DB queries and waits for slaves to catch up to the master. For N slaves that means doing N+3 queries, and waiting for a DB server to reply to a query is much slower (probably more than one order of magnitude) than function call overhead in Collation::singleton().

I just happened to be looking at the Collation code today, I was wondering: Is there a reason to use an EN language object, instead of whatever the content language is for the Uppercase collation? This would make a difference on (for example) turkish wikis due to the whole i <-> İ thing.