Page MenuHomePhabricator

ResourceLoader: Exceptions from lessphp cause problems (Exception object is not MWException)
Closed, ResolvedPublic

Description

On multiple locations, ResourceLoader.php has a call for $e->logException() where $e is of type Exception.

The logException() function is *NOT* a method for Exception class. It is a method for MWException class. Whenever an exception happens, the user will get the following error message:

Call to undefined method Exception::logException() in /var/www/wiki/includes/resourceloader/ResourceLoader.php on line 835

All these references have to be fixed.


Version: 1.22.0
Severity: major

Details

Reference
bz55442

Event Timeline

bzimport raised the priority of this task from to High.Nov 22 2014, 2:14 AM
bzimport set Reference to bz55442.
bzimport added a subscriber: Unknown Object (MLST).
Huji created this task.Oct 7 2013, 8:20 PM

Indeed, the type hint is incorrect.

What kind of exception are you getting though that isn't an instance of MWException? What is the error message?

Huji added a comment.Oct 7 2013, 10:06 PM

I got an exception from the lessc.inc.php when it tried to parse Bootstrap 3. The PHP less included in MW is apparently unable to parse Bootstrap 3 (can parse Boostrap 2.x though), for reasons I couldn't figure out yet.

Regardless, it led me to identify bug 55442.

As for the error parsing Bootstrap 3, see https://github.com/leafo/lessphp/issues/481.

As those errors are not of type MWException, fixing the type hint isn't going to do it. We should either abstract the less parsing in a try/catch and re-throw as MWException or generalise this code to not depend on MWException.

Adding Ori to CC.

Blocking 1.22 release. Like other exceptions, ResourceLoader should properly catch them all and output as comment and log it, as to not break load.php responses.

ori added a comment.Oct 7 2013, 11:51 PM

I got an exception from the lessc.inc.php when it tried to parse Bootstrap 3.

What was the exception?

Huji added a comment.Oct 8 2013, 2:44 AM

The message is what I posted above (see Description). I can try to reproduce the bug and dig more, but I'm sure it is due to lessphp not being able to parse Bootstrap 3.

ori added a comment.Oct 8 2013, 3:44 AM

(In reply to comment #6)

The message is what I posted above (see Description). I can try to reproduce
the bug and dig more, but I'm sure it is due to lessphp not being able to
parse
Bootstrap 3.

More details and possibly steps to reproduce would be helpful.

Maybe you can replace line 835 with

if ( !method_exists( $e, 'logException' ) ) debug_print_backtrace();

Change 88645 had a related patch set uploaded by Krinkle:
resourceloader: Rethrow LESS error and let makeModuleResponse handle it

https://gerrit.wikimedia.org/r/88645

Huji added a comment.Oct 8 2013, 11:58 PM

Ori, I can't reproduce the bug. I did a git pull before trying to reproduce the bug (not so smart, huh?) and because I was using the HEAD revision of MW, lessphp and Bootstrap at the time, going back in time is cumbersome.

If you have any advice about reproducing the bug, please reach me directly by email (so as not to clutter this bug report).

Fixed.

Change-Id: Iab98e3a7a9b78d8602e69e0571b35cf107a96b72

Change 88645 merged by jenkins-bot:
resourceloader: Don't catch LESS error in ResourceLoaderFileModule

https://gerrit.wikimedia.org/r/88645