r112453 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r112452‎ | r112453 | r112454 >
Date:23:03, 26 February 2012
Author:tstarling
Status:ok (Comments)
Tags:
Comment:
Re-add the exception log removed in r88392: unlike Chrome, the current Firebug displays exceptions dumped in this way with full details, including a backtrace
Modified paths:
  • /trunk/phase3/resources/mediawiki/mediawiki.js (modified) (history)

Diff [purge]

Index: trunk/phase3/resources/mediawiki/mediawiki.js
@@ -784,6 +784,7 @@
785785 // and not in debug mode, such as when a symbol that should be global isn't exported
786786 if ( window.console && typeof window.console.log === 'function' ) {
787787 console.log( 'mw.loader::execute> Exception thrown by ' + module + ': ' + e.message );
 788+ console.log( e );
788789 }
789790 registry[module].state = 'error';
790791 }

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r88392Changing console.log(e) to a throw. console.log makes it go toString() which ...krinkle20:31, 18 May 2011

Comments

#Comment by Krinkle (talk | contribs)   23:29, 26 February 2012

r88392 removed the console.log(e) and added a throw. Which made it trigger debug workflows for exceptions in all debuggers (instead of just Firebug), but also made it catchable from unit tests such as QUnit. r112451 removed the throw leaving no debug hook for exceptions and this revision re-adds the console log line.

I'll check out if it's a problem to throw exceptions from execute() (which r88392 intended to do and was reviewed), if it's ok I'll swap it for throw again, okay ? (effectively re-applying r88392)

#Comment by Tim Starling (talk | contribs)   01:18, 27 February 2012

Yes, I think rethrowing the exception is the best option, at least in the short term. In theory, it's possible to catch exceptions to allow unrelated modules to continue to work, but the existing code is not sufficient to make that happen. The whole loop in handlePending() is aborted when an exception is thrown, but the module responsible is does not have its state set to "error", only the innocent module which called execute()

#Comment by Tim Starling (talk | contribs)   01:26, 27 February 2012

...is set to "error". Support for requests for modules with dependencies that have errors does not seem to be fully implemented. At least rethrowing the exception will cleanly stop processing.

Status & tagging log