bad use of recordset return value for checking if empty.

looks to be around 30 instances of:
if ($rs = $DB->get_recordset
in core code, and 9
if (!$rs = $DB->get_recordset
calls plus any others that don't use $rs as the var or have an extra space somewhere.

Oh, oh. Agree all those instances must be fixed ASAP. Surely they haven't bad effects in a lot of places, but in others they can be leading to really wrong results.

Some comments:

1. get_recordset_xx() functions always return on proper moodle_recordset object.
2. moodle_recordset objects are subclasses of PHP's Iterator class (for easy use in loops and friends)
3. intended use of recordsets are to be looped and closed always, hence we always return the iterator object, no matter if it's empty or no.
4. to check if one iterator has anything to return, it can be done with the valid() public method.

So I think we need to iterate over all those cases...:

1. looking when we really need to use them into one "if" sentence.
2. changing them to use ->valid() when necessary
3. checking we are always closing the recordsets

I am happy to work on this, but want to get +1 from Petr before starting with it, just to be sure we are changing them in the best way.

Well, spotted, Dan!

Ciao

grep -r recordset * | grep if | wc -l

returns 100 (nice number but nasty one too)

->valid() is exactly what I needed - great to see it's already supported - thanks Eloy!

+1

I've updated the DB checker tool:

http://cvs.moodle.org/contrib/tools/check_db_syntax/check_db_syntax.php?view=markup

to detect various potentially wrong uses (not only "if" but also, "while|for|return" ones.

I'll start reviewing all them tomorrow (topdown). Ciao

Just for the records, I'm following this approach:

1) If all the code within the if condition is one foreach loop: No need to use recordset->valid(). Delete the condition completely, the loop won't be executed at all. Adjust close() if necessary. So, for example:

$sql = 'SELECT xxxx....';
if ($rs = $DB->get_recordset_sql($sql)) {
    foreach ($rs as $record) {
        // code goes here
    }
    $rs->close();
}

will be transformed to:

$sql = 'SELECT xxxx....';
$rs = $DB->get_recordset_sql($sql);
foreach ($rs as $record) {
    // code goes here
}
$rs->close();

2) If the condition is used to decide about the execution of any code not being a foreach loop, change the condition to use valid() and adjust close() if necessary. So, for example:

$sql = 'SELECT xxxx....';
if ($rs = $DB->get_recordset_sql($sql)) {
    // do something here conditionally and
    // also foreach if you want and
    // do here something else
    $rs->close();
}

will be transformed to:

$sql = 'SELECT xxxx....';
$rs = $DB->get_recordset_sql($sql);
if ($rs->valid()) {
    // do something here conditionally and
    // also foreach if you want and
    // do here something else
}
$rs->close();

You can follow the evolution of this @ https://github.com/stronk7/moodle/compare/master...MDL-25708
(see lib/accesslib.php and lib/datalib.php as the most tricky changes, and review them)

Ignoring this for sprint

This change fixes all the wrong uses of get_recordset_xxx() under 2.0, always returning one Iterator object (2.0) and not one Boolean false anymore.

The logic used to perform all the changes is explained here: http://tracker.moodle.org/browse/MDL-25708?focusedCommentId=100741&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-100741

I've reviewed all the changes twice before committing and also have tested successfully:

• Installing from scratch.
• Upgrade one 1.9 latest site to 2.0 latest
• Run all the DB tests
• Run all the unit tests

So, unless there is some typo, I think everything should continue working ok, saving some extra process here and there and also having all the wrong uses out from codebase.

Special attention, because of their complexity/importance are the ones performed @:

• lib/accesslib.php
• lib/datalib.php
• lib/completion/cron.php
• lib/moodle_database.php

Ciao

Note: the check_db_syntax.php (at contrib cvs) has been updated to cover bad uses and also some false positives discovered along the process

PULL-198 created for this issue.

Tested, thanks!