Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-16175

print_error and error should support throwing exceptions

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0
    • Component/s: General
    • Labels:
      None
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE

      Description

      Currently, they support throwing exceptions if unit tests are running. It would be great for any code to be able to say, "I want exceptions thrown" instead, either by defining a constant or setting something temporarily in $CFG, eg:

      1.
      define('CONVERTERRORSTOEXCEPTIONS', 1);
      // whatever code that might call error or print_error

      2.
      $CFG->converterrorstoexceptions = true;
      // whatever code that might call error or print_error
      $CFG->converterrorstoexcetions = false;

      3.
      $tmpsetting = $CFG->converterrorstoexceptions;
      $CFG->converterrorstoexceptions = true;
      // whatever code that might call error or print_error
      $CFG->converterrorstoexceptions = $tmpsetting;

      I prefer 3.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            mjollnir Penny Leach added a comment -

            adding people

            Show
            mjollnir Penny Leach added a comment - adding people
            Hide
            quen Sam Marshall added a comment -

            or how about

            convert_errors_to_exceptions();
            // ...
            convert_errors_to_exceptions(false);

            that way it can track nesting etc correctly

            Show
            quen Sam Marshall added a comment - or how about convert_errors_to_exceptions(); // ... convert_errors_to_exceptions(false); that way it can track nesting etc correctly
            Hide
            mjollnir Penny Leach added a comment -

            sam - good idea. what do you imagine that function doing internally? modifying $CFG or another global?

            Show
            mjollnir Penny Leach added a comment - sam - good idea. what do you imagine that function doing internally? modifying $CFG or another global?
            Hide
            nicolasconnault Nicolas Connault added a comment - - edited

            My idea:

            4.
            function set_converterrorstoexception($bool=null) {
            global $CFG;
            static $tmpsetting;
            static $nesting_level;

            if (empty($nesting_level))

            { $nesting_level = 1; }

            elseif (!is_null($bool))

            { $nesting_level++; return false; }

            if (is_null($bool))

            { $CFG->converterrorstoexception = $tmpsetting; $nesting_level--; }

            else

            { $tmpsetting = $CFG->converterrorstoexception; $CFG->converterrorstoexception = $bool; }

            }

            Show
            nicolasconnault Nicolas Connault added a comment - - edited My idea: 4. function set_converterrorstoexception($bool=null) { global $CFG; static $tmpsetting; static $nesting_level; if (empty($nesting_level)) { $nesting_level = 1; } elseif (!is_null($bool)) { $nesting_level++; return false; } if (is_null($bool)) { $CFG->converterrorstoexception = $tmpsetting; $nesting_level--; } else { $tmpsetting = $CFG->converterrorstoexception; $CFG->converterrorstoexception = $bool; } }
            Hide
            nicolasconnault Nicolas Connault added a comment - - edited

            Another idea from Sam:

            class convert_error_to_exceptions {
            public static $original_value = null;
            public $overridden = false;

            public function __construct() {
            global $CFG;
            if (is_null(convert_error_to_exceptions::$original_value))

            { convert_error_to_exceptions::$original_value = $CFG->converterrorstoexceptions; $CFG->converterrorstoexceptions = true; }

            else

            { $this->overridden = true; }

            }

            public function __destruct() {
            global $CFG;
            if (!$this->overridden)

            { $CFG->converterrorstoexceptions = convert_error_to_exceptions::$original_value; convert_error_to_exceptions::$original_value = null; }

            }
            }

            $err2exc = new convert_error_to_exceptions();
            // lots of code
            // Eventually $err2exc falls out of scope, is destroyed, and original $CFG value is restored

            Show
            nicolasconnault Nicolas Connault added a comment - - edited Another idea from Sam: class convert_error_to_exceptions { public static $original_value = null; public $overridden = false; public function __construct() { global $CFG; if (is_null(convert_error_to_exceptions::$original_value)) { convert_error_to_exceptions::$original_value = $CFG->converterrorstoexceptions; $CFG->converterrorstoexceptions = true; } else { $this->overridden = true; } } public function __destruct() { global $CFG; if (!$this->overridden) { $CFG->converterrorstoexceptions = convert_error_to_exceptions::$original_value; convert_error_to_exceptions::$original_value = null; } } } $err2exc = new convert_error_to_exceptions(); // lots of code // Eventually $err2exc falls out of scope, is destroyed, and original $CFG value is restored
            Hide
            skodak Petr Skoda added a comment -

            Wouldn't it be easier to throw only exception in library functions and stop using print_error()?
            I agreed with the proposal by Tim: do not use exceptions in normal code flow, use them only for exceptional stuff.

            There is one place where I would like to see optional exceptions - DML. At present we are returning false when SQL query errors out, but in upgrade it seems far more appropriate to throw a fatal exception if anything unexpected happens. We could use $DB->enable_exceptions(bool)


            back to 1/ please no defines for flags that are expected to change

            Show
            skodak Petr Skoda added a comment - Wouldn't it be easier to throw only exception in library functions and stop using print_error()? I agreed with the proposal by Tim: do not use exceptions in normal code flow, use them only for exceptional stuff. There is one place where I would like to see optional exceptions - DML. At present we are returning false when SQL query errors out, but in upgrade it seems far more appropriate to throw a fatal exception if anything unexpected happens. We could use $DB->enable_exceptions(bool) back to 1/ please no defines for flags that are expected to change
            Hide
            mjollnir Penny Leach added a comment -

            Yeah I think eventually everything should change to throw exceptions but that's not something we can do immediately. Allowing code to ask for exceptions in the interim seems reasonable.

            Show
            mjollnir Penny Leach added a comment - Yeah I think eventually everything should change to throw exceptions but that's not something we can do immediately. Allowing code to ask for exceptions in the interim seems reasonable.
            Hide
            skodak Petr Skoda added a comment -

            I would like to volunteer to do the conversion

            Show
            skodak Petr Skoda added a comment - I would like to volunteer to do the conversion
            Hide
            mjollnir Penny Leach added a comment -

            ok, timeframe?

            i think it would be good to have at least the workaround in head asap (mostly because i need it)

            Show
            mjollnir Penny Leach added a comment - ok, timeframe? i think it would be good to have at least the workaround in head asap (mostly because i need it)
            Hide
            skodak Petr Skoda added a comment -

            timeframe? asap - I will need this in DML, DDL, dbtransfer, upgrade code, completion lib just to mention a few of them

            Show
            skodak Petr Skoda added a comment - timeframe? asap - I will need this in DML, DDL, dbtransfer, upgrade code, completion lib just to mention a few of them
            Hide
            mjollnir Penny Leach added a comment -

            just found another example of why i need the workaround:

            • event queue runs
            • wakes up queued portfolio transfer of type mnet
            • something goes wrong
            • mnet calls print_error
            • script dies
            • event queue gets forever held up (do events expire?)

            if we could make the cron event processor ask for exceptions, it could catch this case and move on to the next event to be processed.

            Show
            mjollnir Penny Leach added a comment - just found another example of why i need the workaround: event queue runs wakes up queued portfolio transfer of type mnet something goes wrong mnet calls print_error script dies event queue gets forever held up (do events expire?) if we could make the cron event processor ask for exceptions, it could catch this case and move on to the next event to be processed.
            Hide
            mjollnir Penny Leach added a comment -

            (that is of course assuming you're not prepared to audit mnet and add exceptions everywhere in the next few days )

            Show
            mjollnir Penny Leach added a comment - (that is of course assuming you're not prepared to audit mnet and add exceptions everywhere in the next few days )
            Hide
            skodak Petr Skoda added a comment -

            I think that in this specific case MNET should throw exceptions and events should be able to expire somehow - I personally never thought about this when implementing events. In anycase it might die on PHP error too and catching would not help.

            I think this is a good topic for Monday discussion - I would like to get +1 from MD before we start spreading exceptions everywhere.

            Show
            skodak Petr Skoda added a comment - I think that in this specific case MNET should throw exceptions and events should be able to expire somehow - I personally never thought about this when implementing events. In anycase it might die on PHP error too and catching would not help. I think this is a good topic for Monday discussion - I would like to get +1 from MD before we start spreading exceptions everywhere.
            Hide
            mjollnir Penny Leach added a comment -

            17:24 < penny> petr - +1 for talking with md although you're right about the php error thing (events)
            17:25 < penny> and i agree mnet should throw exceptions, but i don't know if we can implement this in time
            17:26 < penny> the other option is to detect cron in print_error
            17:28 < skodak> lets solve this on Monday, temporary hack in print_error() is ok for me (in fact there is
            already one hack for unittests), for 2.0 we should imo aim for exceptions everywhere
            17:29 < penny> i agree
            17:29 < skodak> my only objects is for making this hack standard part of moodle
            17:29 < skodak> objection
            17:29 < skodak> grrr
            17:30 < penny> i think it's ok temporarily but we should aim to get it out by 2.0 and replaced with
            exceptions properly
            17:30 < skodak> yep, and the sooner the better
            17:30 < penny> yep

            agreement!

            Show
            mjollnir Penny Leach added a comment - 17:24 < penny> petr - +1 for talking with md although you're right about the php error thing (events) 17:25 < penny> and i agree mnet should throw exceptions, but i don't know if we can implement this in time 17:26 < penny> the other option is to detect cron in print_error 17:28 < skodak> lets solve this on Monday, temporary hack in print_error() is ok for me (in fact there is already one hack for unittests), for 2.0 we should imo aim for exceptions everywhere 17:29 < penny> i agree 17:29 < skodak> my only objects is for making this hack standard part of moodle 17:29 < skodak> objection 17:29 < skodak> grrr 17:30 < penny> i think it's ok temporarily but we should aim to get it out by 2.0 and replaced with exceptions properly 17:30 < skodak> yep, and the sooner the better 17:30 < penny> yep agreement!
            Hide
            dougiamas Martin Dougiamas added a comment -

            If we can have a consensus on a good final plan for implementing exceptions in Moodle among Petr, Penny and Tim then I'm all for it.

            Show
            dougiamas Martin Dougiamas added a comment - If we can have a consensus on a good final plan for implementing exceptions in Moodle among Petr, Penny and Tim then I'm all for it.
            Hide
            mjollnir Penny Leach added a comment -

            petr volunteered for this, reassigning.

            i still think this is urgent (at least the workaround) so some discussion towards a consensus would be good.

            Show
            mjollnir Penny Leach added a comment - petr volunteered for this, reassigning. i still think this is urgent (at least the workaround) so some discussion towards a consensus would be good.
            Hide
            timhunt Tim Hunt added a comment -

            I keep meaning to read this, think about it, and post some comments. I'll try to do it early next week.

            Show
            timhunt Tim Hunt added a comment - I keep meaning to read this, think about it, and post some comments. I'll try to do it early next week.
            Hide
            mjollnir Penny Leach added a comment -

            Hey Petr, what happened about this ?

            Show
            mjollnir Penny Leach added a comment - Hey Petr, what happened about this ?
            Hide
            skodak Petr Skoda added a comment -

            We rely on exceptions everywhere now - the print_error() and error() throw them for a long time.

            Thanks everybody!

            Show
            skodak Petr Skoda added a comment - We rely on exceptions everywhere now - the print_error() and error() throw them for a long time. Thanks everybody!

              People

              • Votes:
                1 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  24/Nov/10