Moodle
  1. Moodle
  2. MDL-29339

Oracle error "ORA-01008: not all variables bound" when passing object with __toString in $params

    Details

    • Database:
      Oracle
    • Testing Instructions:
      Hide

      1. Run all the unit tests in question and mod/quiz.

      2. Test attempting a quiz.

      3. Test previewing a question from the question bank.

      4. Test all the quiz reports, on a quiz with several student attempts. In particular test manual grading through the grading report.

      Show
      1. Run all the unit tests in question and mod/quiz. 2. Test attempting a quiz. 3. Test previewing a question from the question bank. 4. Test all the quiz reports, on a quiz with several student attempts. In particular test manual grading through the grading report.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      19002

      Description

      You can trigger this in the quiz reports (which work on other DBs):

      When Instructor clicks on homework quiz Report, Gets Error

      Error reading from database

      More information about this error

      Debug info: ORA-01008: not all variables bound

      etc - see below

        Issue Links

          Activity

          Hide
          Yanfei Lu added a comment -

          I encountered this problem too.

          Show
          Yanfei Lu added a comment - I encountered this problem too.
          Hide
          Yanfei Lu added a comment -

          I think the problem begins here:

          moodle/question/engine/datalib.php line507-515
                  list($statetest, $stateparams) = $this->db->get_in_or_equal(array(
                          question_state::$gaveup,
                          question_state::$gradedwrong,
                          question_state::$gradedpartial,
                          question_state::$gradedright,
                          question_state::$mangaveup,
                          question_state::$mangrwrong,
                          question_state::$mangrpartial,
                          question_state::$mangrright), SQL_PARAMS_NAMED, 'st');
          

          these question_state objects is still objects when bind with SQL statement variables, this can explain the "not all variables bound" error. For now, I use a very tricky solution:

                  list($statetest, $stateparams) = $this->db->get_in_or_equal(array(
                  	question_state::$gaveup . '',
                          question_state::$gradedwrong . '',
                          question_state::$gradedpartial . '',
                          question_state::$gradedright . '',
                          question_state::$mangaveup . '',
                          question_state::$mangrwrong . '',
                          question_state::$mangrpartial . '',
                          question_state::$mangrright . ''), SQL_PARAMS_NAMED, 'st');
          

          provide string context for these objects, then these objects will convert to strings, which conforms to state column in database.
          This is ugly of course. I think this problem should be solved in the function below:

          moodle/lib/dml/moodle_database.php line584
           public function get_in_or_equal($items, $type=SQL_PARAMS_QM, $prefix='param', $equal=true, $onemptyitems=false)
          
          Show
          Yanfei Lu added a comment - I think the problem begins here: moodle/question/engine/datalib.php line507-515 list($statetest, $stateparams) = $ this ->db->get_in_or_equal(array( question_state::$gaveup, question_state::$gradedwrong, question_state::$gradedpartial, question_state::$gradedright, question_state::$mangaveup, question_state::$mangrwrong, question_state::$mangrpartial, question_state::$mangrright), SQL_PARAMS_NAMED, 'st'); these question_state objects is still objects when bind with SQL statement variables, this can explain the "not all variables bound" error. For now, I use a very tricky solution: list($statetest, $stateparams) = $ this ->db->get_in_or_equal(array( question_state::$gaveup . '', question_state::$gradedwrong . '', question_state::$gradedpartial . '', question_state::$gradedright . '', question_state::$mangaveup . '', question_state::$mangrwrong . '', question_state::$mangrpartial . '', question_state::$mangrright . ''), SQL_PARAMS_NAMED, 'st'); provide string context for these objects, then these objects will convert to strings, which conforms to state column in database. This is ugly of course. I think this problem should be solved in the function below: moodle/lib/dml/moodle_database.php line584 public function get_in_or_equal($items, $type=SQL_PARAMS_QM, $prefix='param', $equal= true , $onemptyitems= false )
          Hide
          Tim Hunt added a comment -

          The point is that those objects have a perfectly good __toString method, which means that PHP should have no problem using them as strings when necessary.

          The problem is not get_in_or_equal. That works fine on other DBs. The problem must be where we bind the parameter values to the placeholders in the SQL. That is, bind_params or fix_sql_params in lib/dml/oci_native_moodle_database.php. There is already code there to convert bools to ints. I think we should add similar code to convert objects to their __toString representation, if possible.

          Eloy, does that sound right to you? And if so, where would you put that code? I am happy to make the patch, but I need some guidance about exactly where to do it.

          Show
          Tim Hunt added a comment - The point is that those objects have a perfectly good __toString method, which means that PHP should have no problem using them as strings when necessary. The problem is not get_in_or_equal. That works fine on other DBs. The problem must be where we bind the parameter values to the placeholders in the SQL. That is, bind_params or fix_sql_params in lib/dml/oci_native_moodle_database.php. There is already code there to convert bools to ints. I think we should add similar code to convert objects to their __toString representation, if possible. Eloy, does that sound right to you? And if so, where would you put that code? I am happy to make the patch, but I need some guidance about exactly where to do it.
          Hide
          Yanfei Lu added a comment -

          Tim, I found another place that will result in the same problem.

          question/engine/datalib.php line775-778
              public function in_summary_state_test($summarystate, $equal = true, $prefix = 'summarystates') {
                  $states = question_state::get_all_for_summary_state($summarystate);
                  return $this->db->get_in_or_equal($states, SQL_PARAMS_NAMED, $prefix, $equal);
              }
          

          These states objects still cann't be binded correctlly.

          Show
          Yanfei Lu added a comment - Tim, I found another place that will result in the same problem. question/engine/datalib.php line775-778 public function in_summary_state_test($summarystate, $equal = true , $prefix = 'summarystates') { $states = question_state::get_all_for_summary_state($summarystate); return $ this ->db->get_in_or_equal($states, SQL_PARAMS_NAMED, $prefix, $equal); } These states objects still cann't be binded correctlly.
          Hide
          Yanfei Lu added a comment -

          Yet again, I use ugly solution for convenient, and waiting for the change to come. lib/dml/oci_native_moodle_database.php is too difficult for me now.

              public function in_summary_state_test($summarystate, $equal = true, $prefix = 'summarystates') {
                  $states = question_state::get_all_for_summary_state($summarystate);
                  $str_states = array();
                  foreach($states as $s)
                  	$str_states[] = $s->__toString();
                  return $this->db->get_in_or_equal($str_states, SQL_PARAMS_NAMED, $prefix, $equal);
              }
          
          Show
          Yanfei Lu added a comment - Yet again, I use ugly solution for convenient, and waiting for the change to come. lib/dml/oci_native_moodle_database.php is too difficult for me now. public function in_summary_state_test($summarystate, $equal = true , $prefix = 'summarystates') { $states = question_state::get_all_for_summary_state($summarystate); $str_states = array(); foreach($states as $s) $str_states[] = $s->__toString(); return $ this ->db->get_in_or_equal($str_states, SQL_PARAMS_NAMED, $prefix, $equal); }
          Hide
          Tim Hunt added a comment -

          This is a completely un-tested, but plausible-looking fix.

          Would a cast like $params[$key] = (string) $value; be nicer that $params[$key] = '' . $value; ?

          Also, does this work? I guess we really ought to add some DB tests if we want to guarantee this is supported.

          Eloy, comments?

          Show
          Tim Hunt added a comment - This is a completely un-tested, but plausible-looking fix. Would a cast like $params [$key] = (string) $value; be nicer that $params [$key] = '' . $value; ? Also, does this work? I guess we really ought to add some DB tests if we want to guarantee this is supported. Eloy, comments?
          Hide
          Tim Hunt added a comment -

          OK, some basic tests written (new commit on the same branch). They pass on Postgres. Note that there are no tests for the bool special case in the Oracle driver, as far as I can see.

          Can anyone run these tests on all other DBs?

          Show
          Tim Hunt added a comment - OK, some basic tests written (new commit on the same branch). They pass on Postgres. Note that there are no tests for the bool special case in the Oracle driver, as far as I can see. Can anyone run these tests on all other DBs?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          After some discussion it has been decided to prevent completely passing any object to moodle_database (see MDL-29894), so it will be caller responsibility to pass proper params (surely cast states in this exact case).

          Show
          Eloy Lafuente (stronk7) added a comment - After some discussion it has been decided to prevent completely passing any object to moodle_database (see MDL-29894 ), so it will be caller responsibility to pass proper params (surely cast states in this exact case).
          Hide
          Tim Hunt added a comment -

          OK, more primitive fix applied (manually cast the objects to string in the question engine code, rather than relying on magic.

          That matches what happens when we load the data, there is an explicit cast in the question engine code $step->state = question_state::get($record->state);

          Also, there was already an explicit cast to string in one of the QE DB queries, so this just makes all the rest consistent.

          Please test this change very carefully.

          Show
          Tim Hunt added a comment - OK, more primitive fix applied (manually cast the objects to string in the question engine code, rather than relying on magic. That matches what happens when we load the data, there is an explicit cast in the question engine code $step->state = question_state::get($record->state); Also, there was already an explicit cast to string in one of the QE DB queries, so this just makes all the rest consistent. Please test this change very carefully.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Looks perfect, thanks! Integrated.

          I hope next week MDL-29894 will be able to drastic-prevent any other object use.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Looks perfect, thanks! Integrated. I hope next week MDL-29894 will be able to drastic-prevent any other object use. Ciao
          Hide
          Sam Hemelryk added a comment -

          Thanks Tim - tests passed

          Show
          Sam Hemelryk added a comment - Thanks Tim - tests passed
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Done, your delicious hacks have been sent upstream, many thanks!

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Done, your delicious hacks have been sent upstream, many thanks! Closing as fixed, ciao
          Hide
          Cindy Duggan added a comment -

          Thank You Much!

          Issue seems to be resolved.

          Show
          Cindy Duggan added a comment - Thank You Much! Issue seems to be resolved.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: