Moodle
  1. Moodle
  2. MDL-25708

bad use of recordset return value for checking if empty.

    Details

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

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

        Issue Links

          Activity

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

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

          returns 100 (nice number but nasty one too)

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

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

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

          +1

          Show
          Petr Škoda added a comment - +1
          Hide
          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
          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
          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
          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
          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
          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
          Martin Dougiamas added a comment -

          Ignoring this for sprint

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

          PULL-198 created for this issue.

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

          Tested, thanks!

          Show
          Dongsheng Cai added a comment - Tested, thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: