Moodle
  1. Moodle
  2. MDL-16175

print_error and error should support throwing exceptions

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor 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
    • Rank:
      31182

      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.

        Issue Links

          Activity

          Hide
          Penny Leach added a comment -

          adding people

          Show
          Penny Leach added a comment - adding people
          Hide
          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
          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
          Penny Leach added a comment -

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

          Show
          Penny Leach added a comment - sam - good idea. what do you imagine that function doing internally? modifying $CFG or another global?
          Hide
          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
          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
          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
          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
          Petr Škoda 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
          Petr Škoda 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
          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
          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
          Petr Škoda added a comment -

          I would like to volunteer to do the conversion

          Show
          Petr Škoda added a comment - I would like to volunteer to do the conversion
          Hide
          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
          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
          Petr Škoda 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
          Petr Škoda 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
          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
          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
          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
          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
          Petr Škoda 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
          Petr Škoda 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
          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
          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
          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
          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
          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
          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
          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
          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
          Penny Leach added a comment -

          Hey Petr, what happened about this ?

          Show
          Penny Leach added a comment - Hey Petr, what happened about this ?
          Hide
          Petr Škoda added a comment -

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

          Thanks everybody!

          Show
          Petr Škoda 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: