MediaWiki r85918 - Code Review

Appears to have caused bug 28834 with the change to Exception.php's wfReportException() removing an nl2br() call and thus removing line breaks from the exception stack trace.

Fixed in r87628.

Appears to break HipHop support. Please do not define multiple functions with the same name, even if they are in different files. There are lots of other ways to do this.

I had already noticed this, I just hadn't worked on a fix for it yet. To be honest, I don't really like this approach at all. wfDie() means very different things, depending on the context.

• Dying in maintenance scripts - New maintenance scripts can just use error() with the second parameter. Older ones should be updated to subclass Maintenance anyway.
• Catastrophic errors like PHP version too low, API disabled, or missing LocalSettings.
• Some old junk in Exception.php

Since wfDie() isn't used outside of core anywhere, I'd suggest we just remove it entirely or at least severely limit the use-cases.

In most places where you might want to use this new variant of wfDie(), you can throw an exception instead. Exceptions already have the infrastructure for formatting error messages in various ways depending on the entry point. If there's some problem with that infrastructure, then it should be fixed rather than duplicated.

The old uses of wfDie() are just a shortcut for "echo $msg;die(-1);", they reflect Brion's disgust at PHP when he found out that die($msg) doesn't do that. See r12660. I removed most instances from the core in r14631 in favour of exceptions.

Certainly pretty much all errors should be triggered by raising an exception. The use of wfDie() is to define what the exception should do with it. Before, IIRC, exceptions triggered in load.php outputted HTML into what should be a CSS/JS environment, for instance, because Exception.php didn't know what context it was echoing into (still doesn't, for that matter). The uses of wfDie() in Exception.php are not old junk, they're brand new in this revision.

How does wfDie() have "different meanings depending on the context"? wfDie() means "I can't handle this any more, get me out of here"; in a fashion which is entry-point-dependent, doesn't require access to the autoloader, and preferably which works on as many environments as possible, such as PHP4.

I suggest restoring wfDie() back to how it was before, and thinking of a new name for what you want to do.

It would be helpful if this, like the other issues affecting HipHop support, could be documented at HipHop or somesuch. What is the way to achieve this? Would a structure like

// load.php
define( 'MW_ENTRY_POINT', 'LOAD' );
 
...
 
if( MW_ENTRY_POINT == 'LOAD' ) {
    function wfDie( $msg ){ ... }
}

Work?

I thought I documented it already, at HipHop#Declarations. Let me know if there's something unclear about that page.

No, it won't work. One thing that would work would be to have a set of exception formatting classes in an autoloaded class file, then set the class name to use via a constant, global variable or static member accessor function like Exception::setFormattingClass(). That won't work for PHP 4, but I'm pretty sure I broke PHP 4 parsing in r88959. So maybe some more robust solution is needed for PHP 4, like say moving the bulk of the code out of the entry points and restoring PHP4.php.

PHP4.php was better than what you have here anyway, since it was able to detect the situation where a .php5 extension will work. Or at least it did until the entry points were broken again.

The other option is forgetting about PHP4.

Well yeah, specifically, documenting the fact that parse errors mean you have to upgrade to PHP 5 and leaving it at that. The reason I did some work on PHP 4 error messages in r41713 is because we were having support requests from people who didn't know what the parse errors meant.

> I'm pretty sure I broke PHP 4 parsing in r88959.

You did r1=88958&r2=88959. PHP 4 doesn't like try/catch.

I'd say let's either drop the PHP 4 checks entirely, or move the actual code out of the entry points and require() through to the 'real' code, so we don't all keep accidentally checking things in that break the PHP 4 parser. :)

I had already looked at wfIndexMain(), I think it should be pieced and moved into MediaWiki class. Also, it is broken for $wgArticle currently.

Sounds good. Can you also rename the MediaWiki class to MainEntry or something while you're at it? It's always been a helper for index.php, and has very little to do with the rest of MediaWiki.

That's exactly what I did! But as Tim's pointed out elsewhere, most uses of require() breaks HipHop unless it's run through MWInit::compiledPath(), and you can't get that without loading WebStart, which is not PHP4-compatible... :( So you can't really move very much of what's in index.php deeper into the callstack, because you still have to leave enough infrastructure to be able to correctly find the deeper file.

Poop. My vote then goes towards just killing all the PHP 4 stuff and documenting 'if you get parse errors, it's cause you're running year-2000 tech still, upgrade silly' :D