Moodle

Yeah, confirmed.

Under 2.0 it calls:

$OUTPUT->fatal_error()

that has one explicit:

if (!debugging('', DEBUG_DEVELOPER)) {
return false;
}

IMO we are mixing things here, user-level errors and developers info. This needs to be clarified asap IMO.

Ciao

That is a bug.

The DEBUG_DEVELOPER bit should just be deciding whether to print a stack-trace.

So something like this is correct:

--- outputrenderers.php	3 Oct 2009 08:55:01 -0000	1.32
+++ outputrenderers.php	23 Oct 2009 14:18:56 -0000
@@ -1920,10 +1920,6 @@
 
         $output = '';
 
-        if (!debugging('', DEBUG_DEVELOPER)) {
-            return false;
-        }
-
         if ($this->has_started()) {
             $output .= $this->opencontainers->pop_all_but_last();
         } else {
@@ -1942,10 +1938,10 @@
             $output .= $this->notification('error() is a deprecated function. ' .
                     'Please call print_error() instead of error()', 'notifytiny');
         }
-        if (!empty($debuginfo)) {
+        if (!empty($debuginfo) && debugging('', DEBUG_DEVELOPER)) {
             $output .= $this->notification($debuginfo, 'notifytiny');
         }
-        if (!empty($backtrace)) {
+        if (!empty($backtrace) && debugging('', DEBUG_DEVELOPER)) {
             $output .= $this->notification('Stack trace: ' .
                     format_backtrace($backtrace), 'notifytiny');
         }

Note the patch makes DEBUG_DEVELOPER to control the printing of both $debuginfo and $backtrace. Looks ok?

Note early_error() - the alternative for non-started pages does exactly that: Use DEBUG_DEVELOPER to control the printing of both so I think it's ok to follow the same approach in fatal_error().

Ciao

That looks good. Stick it in to CVS.

I have – I think – a more comprehensive patch series over here MDL-20645

Patchseries at http://dev.laptop.org/git/users/martin/moodle.git/log/?h=mdl20-errorhandling

fixed

Petr, that's rude. You didn't review my (more comprehensive) patch and you fixed this narrower issue instead. I'll now rework my patches on top of this, but this isn't a fun way to work with you.

I am sorry, but I reviewed them and spent more them two days working on related issues

http://dev.laptop.org/git/users/martin/moodle.git/commit/?h=mdl20-errorhandling&id=c0fbe5c2681675694e36b5ed471d678e00e4ddfb
is wrong - the rendering code needs to handle debug info, not the exception handler, it would also cause excessive duplication of code in handlers

http://dev.laptop.org/git/users/martin/moodle.git/commit/?h=mdl20-errorhandling&id=d31554e1179e1f5ccb115e685d4823153c074237
rejected - 503 in used for fatal moodle server errors (early errors in setup.php), 404 is used mostly for cases where user follows obsoleted link or result of changed permissions

http://dev.laptop.org/git/users/martin/moodle.git/commit/?h=mdl20-errorhandling&id=f32c007daf55d752393325a0d074da6a71c44002
sort of included in my patches + several other fixes and improvements

http://dev.laptop.org/git/users/martin/moodle.git/commit/?h=mdl20-errorhandling&id=9997328db1823c5c1d61fd62d4b0d7fb5688f035
used

What did I do wrong? I was maintaining this part of Moodle for years now. Before every release I go through the install/upgrade/setup.php and similar code and fix regression; I am watching tracker for any related problems and try to fix them as soon as possible. After each branching I try to improve setup.php, I remove all the useless legacy stuff from there, I am trying to keep things consistent and secure.

I am sorry if I hurt your feelings.

It is not about feelings. Thanks for giving me some notes on my patches.

About:

http://dev.laptop.org/git/users/martin/moodle.git/commit/?h=mdl20-errorhandling&id=c0fbe5c2681675694e36b5ed471d678e00e4ddfb
is wrong - the rendering code needs to handle debug info, not the exception handler, it would also cause excessive duplication of code in handlers

I hope that you have preserved somehow the "write it to the log before preparing the pretty print message". A good part of the rpoblem I found is that we fail during the preparations of pretty messages. Relying on the renderer stuff is fragile.

And about:

http://dev.laptop.org/git/users/martin/moodle.git/commit/?h=mdl20-errorhandling&id=d31554e1179e1f5ccb115e685d4823153c074237
rejected - 503 in used for fatal moodle server errors (early errors in setup.php), 404 is used mostly for cases where user follows obsoleted link or result of changed permissions

This is wrong. We error out in many situations, and many of them are worthy of a 500 or 503.

We may need a 404 error handler, and a 403 error handler – but that is a separate issue.

The problem is that print_error is an API that is use everywhere, often without using all the optional arguments, so inserting an new param at the end is a pain.

But actually, in Moodle 2.0, with its increasing use of exceptions, we can start to handle this better without changing the API.

• For example, coding_exception, dml_exception, etc. -> 503.
• Require capability failure (may need to make this throw a new lacking_capability_exception or something) -> 403
• dml_missing_record_exception -> 404

But I think leaving print_error as 404 is the least bad option for now.

Unfortunately the print_error() was not a good idea since the very beginning especially for translators - tons of useless and duplicated error strings I was objecting but was ignored - in the end I spent many hours cleaning up the lang files and removing useless print_error()s all over the place.

Tim, I suppose you overlooked my new parameter makes the signature of moodle_exception() and print_error() the same, if there is anything wrong with that we should change both imho.

I do like the idea of sending different code for each exception!

hmm, I think that many more print_error()s will be removed, because in many cases it ends up with DML exception much earlier.