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

bad use of recordset return value for checking if empty.

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0.2
    • Component/s: Database SQL/XMLDB
    • Labels:
    • Difficulty:
      Moderate
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE

      Description

      with the changes in 2.0 the following code won't work correctly as recordset calls will always return some data and aren't empty.

      here are just a couple of recordset_sql calls I've found but there are a lot more that need fixing.

      this one looks really bad in lib/dml/moodle_database.php:
      if ($mrs = $this->get_recordset_sql($sql, $params, 0, 1))

      admin/multilangupgrade.php:
      if ($rs = $DB->get_recordset_sql($sql)) {

      course/user.php:
      if (!$rs = $DB->get_recordset_sql($sql)) {

      enrol/authorize/localfuncs.php:
      if (!$rs = $DB->get_recordset_sql($sql)) {

      group/overview.php:
      if ($rs = $DB->get_recordset_sql($sql, $params)) {

      lots like this in lib/accesslib.php:
      if ($rs = $DB->get_recordset_sql($sql, $params)) {

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            danmarsden Dan Marsden added a comment -

            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.

            Show
            danmarsden Dan Marsden added a comment - 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.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            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

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - 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: get_recordset_xx() functions always return on proper moodle_recordset object. moodle_recordset objects are subclasses of PHP's Iterator class (for easy use in loops and friends) 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. 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...: looking when we really need to use them into one "if" sentence. changing them to use ->valid() when necessary 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
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

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

            returns 100 (nice number but nasty one too)

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - grep -r recordset * | grep if | wc -l returns 100 (nice number but nasty one too)
            Hide
            danmarsden Dan Marsden added a comment -

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

            Show
            danmarsden Dan Marsden added a comment - ->valid() is exactly what I needed - great to see it's already supported - thanks Eloy!
            Hide
            skodak Petr Skoda added a comment -

            +1

            Show
            skodak Petr Skoda added a comment - +1
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            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

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - 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
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

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

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - 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();
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            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)

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - 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)
            Hide
            dougiamas Martin Dougiamas added a comment -

            Ignoring this for sprint

            Show
            dougiamas Martin Dougiamas added a comment - Ignoring this for sprint
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            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

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - 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
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            PULL-198 created for this issue.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - PULL-198 created for this issue.
            Hide
            dongsheng Dongsheng Cai added a comment -

            Tested, thanks!

            Show
            dongsheng Dongsheng Cai added a comment - Tested, thanks!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  21/Feb/11