On 1/8/08, vasilievvv@svn.wikimedia.org vasilievvv@svn.wikimedia.org wrote:
Log Message:
- Add exception hooks to output pretty messages
. . .
$wgOut->addHTML( $this->getHTML() );
if( $hookResult = $this->runHooks( get_class( $this ) ) ) {$wgOut->addHTML( $hookResult );} else {$wgOut->addHTML( $this->getHTML() );}
Is completely replacing all output the right way to go about generating pretty error messages? Your example more or less copy-pastes the HTML from MonoBook.php, but incompletely, which isn't good for anyone -- especially not those using non-Monobook skins. If we're going to say it should just be left ugly, okay, but if it's to be made pretty, it should use skins properly, IMO. It shouldn't just output prettier hardcoded HTML, which will look just as ugly in the context of a totally different skin and be more confusing.
People expect ugly error messages, but they don't expect error messages that are pretty but in a totally different way from the rest of the site. :)
Simetrical writes:
On 1/8/08, vasilievvv-Y8jq7F6rJ48dvk2hry9Ukdi2O/JbrIOy@public.gmane.org vasilievvv-Y8jq7F6rJ48dvk2hry9Ukdi2O/JbrIOy@public.gmane.org wrote:
Log Message:
- Add exception hooks to output pretty messages
. . .
$wgOut->addHTML( $this->getHTML() );
if( $hookResult = $this->runHooks( get_class( $this ) ) ) {$wgOut->addHTML( $hookResult );} else {$wgOut->addHTML( $this->getHTML() );}Is completely replacing all output the right way to go about generating pretty error messages? Your example more or less copy-pastes the HTML from MonoBook.php, but incompletely, which isn't good for anyone -- especially not those using non-Monobook skins. If we're going to say it should just be left ugly, okay, but if it's to be made pretty, it should use skins properly, IMO. It shouldn't just output prettier hardcoded HTML, which will look just as ugly in the context of a totally different skin and be more confusing.
People expect ugly error messages, but they don't expect error messages that are pretty but in a totally different way from the rest of the site. :)
These hooks are supposed to replace all error message. Example is just example, on the live site we should probably use the same skin as squid error message use.
On Jan 13, 2008 8:31 AM, VasilievVV vasilvv@gmail.com wrote:
These hooks are supposed to replace all error message.
They shouldn't, if they're meant to be pretty. Any error message meant to be pretty should use the current skin if available. The best way to do this would be to just adjust Exception.php so it uses $wgOut, if that's available and the exception wasn't thrown while processing another exception (to avoid loops). There's no need for a hook here at all, in my opinion. Also, if there is, why are you using an entirely new mechanism with its own global and function calls and registration convention instead of the existing hook mechanism?
Simetrical writes:
On Jan 13, 2008 8:31 AM, VasilievVV vasilvv-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
These hooks are supposed to replace all error message.
They shouldn't, if they're meant to be pretty. Any error message meant to be pretty should use the current skin if available. The best way to do this would be to just adjust Exception.php so it uses $wgOut, if that's available and the exception wasn't thrown while processing another exception (to avoid loops). There's no need for a hook here at all, in my opinion. Also, if there is, why are you using an entirely new mechanism with its own global and function calls and registration convention instead of the existing hook mechanism?
Note, that when exception is raised, $wgOut may not be initialized, so sometimes it's not possible to use it. Also, wfRunHooks may raise exceptions itself, so it's neccesary to use another mechanism instead of hooks. --VasilievVV
vasilvv@gmail.com wrote in message news:478A48C7.30007@gmail.com...
Simetrical writes:
On Jan 13, 2008 8:31 AM, VasilievVV
vasilvv-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
These hooks are supposed to replace all error message.
They shouldn't, if they're meant to be pretty. Any error message meant to be pretty should use the current skin if available. The best way to do this would be to just adjust Exception.php so it uses $wgOut, if that's available and the exception wasn't thrown while processing another exception (to avoid loops). There's no need for a hook here at all, in my opinion. Also, if there is, why are you using an entirely new mechanism with its own global and function calls and registration convention instead of the existing hook mechanism?
Note, that when exception is raised, $wgOut may not be initialized, so sometimes it's not possible to use it.
As mentioned ("so it uses $wgOut, if that's available")
Also, wfRunHooks may raise exceptions itself, so it's neccesary to use another mechanism instead of hooks.
As mentioned ("and the exception wasn't thrown while processing another exception (to avoid loops).")
- Mark Clements (HappyDog)
On 1/14/08, Mark Clements gmane@kennel17.co.uk wrote:
Note, that when exception is raised, $wgOut may not be initialized, so sometimes it's not possible to use it.
As mentioned ("so it uses $wgOut, if that's available")
Also, wfRunHooks may raise exceptions itself, so it's neccesary to use another mechanism instead of hooks.
As mentioned ("and the exception wasn't thrown while processing another exception (to avoid loops).")
I should point out that the exception handler already catches exceptions thrown from the outputHTML()/outputText() methods, and prints a minimalistic default error message in those cases (relying only on the __toString() method, which I guess is technically not safe either). Look at wfReportException().
Plus, if you're running any hooks at all, there's a risk that the hook will throw an exception; the added possibility that the default hook-handling code might throw one is unimportant. Who says it's more likely to than your duplicate code?
wikitech-l@lists.wikimedia.org