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

I encountered this problem too.

I think the problem begins here:

        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:

 public function get_in_or_equal($items, $type=SQL_PARAMS_QM, $prefix='param', $equal=true, $onemptyitems=false)

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.

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

    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.

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);
    }

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?

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?

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).

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.

Looks perfect, thanks! Integrated.

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

Ciao

Thanks Tim - tests passed

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

Closing as fixed, ciao

Thank You Much!

Issue seems to be resolved.