r89925 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r89924‎ | r89925 | r89926 >
Date:12:02, 12 June 2011
Author:ariel
Status:reverted (Comments)
Tags:
Comment:
safer check if mhash is available (and I'm about to make it unavailable by disabling the fn in the php.ini for lucid builds)
Modified paths:
  • /trunk/phase3/includes/HistoryBlob.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/HistoryBlob.php
——@@ -516,7 +516,7 @@
517517 $header = unpack( 'Vofp/Vcsize', substr( $diff, 0, 8 ) );
518518
519519 # Check the checksum if mhash is available
520 - if ( extension_loaded( 'mhash' ) ) {
 520+ if ( function_exists( 'mhash' ) ) {
521521 $ofp = mhash( MHASH_ADLER32, $base );
522522 if ( $ofp !== substr( $diff, 0, 4 ) ) {
523523 wfDebug( __METHOD__. ": incorrect base checksum\n" );

Follow-up revisions

RevisionCommit summaryAuthorDate
r89927mft r89925 so we can disable mhash in php.ini instead of at build timeariel12:29, 12 June 2011
r90035Revert r89925, setting disable_functions in php.ini is definitely the wrong w...tstarling06:07, 14 June 2011

Comments

#Comment by ArielGlenn (talk | contribs)   12:23, 12 June 2011

I want this for wmf hosts so we can disable mhash right in the php.ini file, much more convenient than recompiling php. It's not enabled on the apaches currently; we want to keep that behavior. (The mhash check fails for a large number of revisions when I test it on lucid; enabling it would block OS upgrades of apaches and xml dump hosts.)

#Comment by Tim Starling (talk | contribs)   01:13, 14 June 2011

What is the reason for the hash mismatch?

#Comment by RobLa-WMF (talk | contribs)   16:30, 13 June 2011

Presumably not a tarball blocker. I'll leave the 1.17 tag on because this should get merged into the REL1_17 branch, but maybe not before the tarball is published.

#Comment by ArielGlenn (talk | contribs)   18:46, 13 June 2011

Not worried about the tarball at all, it just needs to not get dropped from the wmf live branch cause Bad Things Would Happen (TM).

#Comment by Tim Starling (talk | contribs)   02:00, 14 June 2011

Disabling the extension is definitely the wrong way to fix a hash mismatch. You should comment out the hash check, but leave mhash available for other code that wants to use it. Then you should work out why the hash check is failing.

#Comment by ArielGlenn (talk | contribs)   05:02, 14 June 2011

It's disabled on on all servers in hardy; who knows how long this is been broken. We're just keeping the situation the same for lucid. Yes, it would be nice to know why it fails; that can't hold up lucid rollouts. There is no other use of mhash in MW, I checked the extensions already before making this change.

#Comment by Tim Starling (talk | contribs)   06:07, 14 June 2011

OK, I guess I can fix it myself, if you're refusing.

Status & tagging log