r84258 MediaWiki - Code Review archive

Don't forget extensions too.

Extensions were deliberately not done, because it breaks compatibility between later versions of extensions and older versions of MediaWiki (same principle behind keeping wfLoadExtensionMessages()). Extensions should be updated in a version or so's time.

Only for extensions that encourage mismatching extension/core versions, which I really don't like. I don't think any WMF extensions do that, so they can be done at least. Since the old block functions are still there, it doesn't really matter for now I guess.

If you just toss the return value of wfMessage( 'foo' ); around, it will default to full parsing... I'd rather use ->text() in this case. Maybe the default should be changed?

+			$options[] = wfMsgExt(
+				'expiringblock',
+				$wgLang->date( $expiry ),
+				$wgLang->time( $expiry )
+			);

Second value missing.

Just grabbing random database handle in Language::formatExpiry feels ugly.

And lastly, I think $foo == (and equivalents) is just bullshit and should be avoided in new code. One should decide what is intended and write either !$foo or $foo === or strval($foo) === . In this case it's already in use, but something to keep in mind for future.

->text() is the equivalent of the old parsemag, which I never really liked. What's wrong with people linking "indefinite block" to a relevant policy or somesuch?

I've had a go at building a static method at the Database level for this, but it just seems more ugly still. The only other options would be a global function or a global define, both of which are equally unpleasant. Or continue to call $protectionFormExpiry = Block::infinite() and such incorrectnesses.

I agree on the $foo == ''; although as you say, the code was copied verbatim and I'd have to look carefully at what edgecase it's testing against there.

What if it appears, say in a drop down?

Does it?

I've had a go at building a static method at the Database level for this, but it just seems more ugly still.

I really don't see what the problem was here. The implementations are identical for all subclasses except DatabaseMssql anyway.

It's basically impossible to do without Late Static Binding, for which we'd require PHP5.3.

+ static $dbr, $msg; + if( !$dbr ){ + $dbr = wfGetDB( DB_SLAVE ); + $msg = wfMessage( 'infiniteblock' ); + }

It is better to cache the value of $dbr->getInfinity() instead of a db connection.

-		$this->mExpiry = self::decodeExpiry( $expiry );
+		$this->mExpiry = $expiry;

-		$this->mExpiry = self::decodeExpiry( $row->ipb_expiry );
+		$this->mExpiry = $row->ipb_expiry;

What about equals(), initFromRow(), doAutoblock(), isExpired()? The assume that it is either a MW timestamp or DB::getInfinity(). However, initFromRow() has the line:

$this->mExpiry = $row->ipb_expiry;

...and it never makes sure it is TS_MW as needed. That would break on non-MySQL.

It looks like *new* blocks come in with the expiry filtered via parseExpiryInput(), which is OK.

At any rate, I'm not sure I like mExpiry being DBMS dependent; ideally it would be independent and convert on save/load on the fly like most timestamp stuff already does.

To be clear, in that last sentence, I was talking not liking 'infinity' in mExpiry being DBMS dependent. Definitely non-infinite mExpiry timestamps should all be TS_MW.

Fixed in r92480.