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

undefined $dependentsetting in register_dependency() in base_settings.class.php

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.1.6, 2.2.3
    • Component/s: Backup
    • Labels:

      Description

      public function register_dependency(setting_dependency $dependency) {
          if ($this->is_circular_reference($dependency->get_dependent_setting())) {
              $a = new stdclass();
              $a->alreadydependent = $this->name;
              $a->main = $dependentsetting->get_name();
              throw new base_setting_exception('setting_circular_reference', $a);
          }
          $this->dependencies[$dependency->get_dependent_setting()->get_name()] = $dependency;
          $dependency->get_dependent_setting()->register_dependent_dependency($dependency);
      }

      should be probably $dependency->get_name();

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hehe is my shotty code so probably fair that I look at this.

            Show
            samhemelryk Sam Hemelryk added a comment - Hehe is my shotty code so probably fair that I look at this.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi Petr, sending this back.
            setting_dependency doesnt have a get_name method.
            Pretty sure that is meant to be:

            $a->main = $dependency->get_dependent_setting()->get_name();

            I've just been really looking at this and am thinking the following patch is the right way to go.
            It fixes this issue, fixes the backup settings unit tests and adds a register dependency test.
            I'm thinking there are bigger problems there as the backup setting tests are missing includes and I haven't seen the unit tests failing... who knows. Either way let me know what you think, if you want feel free to take over again or I'll add it to my queue.

            My solution:

            diff --git a/backup/util/settings/base_setting.class.php b/backup/util/settings/base_setting.class.php
            index 82e6eb1..c969f5e 100644
            --- a/backup/util/settings/base_setting.class.php
            +++ b/backup/util/settings/base_setting.class.php
            @@ -294,7 +294,7 @@ abstract class base_setting {
                     if ($this->is_circular_reference($dependency->get_dependent_setting())) {
                         $a = new stdclass();
                         $a->alreadydependent = $this->name;
            -            $a->main = $dependentsetting->get_name();
            +            $a->main = $dependency->get_dependent_setting()->get_name();
                         throw new base_setting_exception('setting_circular_reference', $a);
                     }
                     $this->dependencies[$dependency->get_dependent_setting()->get_name()] = $dependency;
            diff --git a/backup/util/settings/simpletest/testsettings.php b/backup/util/settings/simpletest/testsettings.php
            index fd079cc..7d0d8a7 100644
            --- a/backup/util/settings/simpletest/testsettings.php
            +++ b/backup/util/settings/simpletest/testsettings.php
            @@ -32,10 +32,12 @@ require_once($CFG->dirroot . '/backup/util/interfaces/checksumable.class.php');
             require_once($CFG->dirroot . '/backup/backup.class.php');
             require_once($CFG->dirroot . '/backup/util/settings/base_setting.class.php');
             require_once($CFG->dirroot . '/backup/util/settings/backup_setting.class.php');
            +require_once($CFG->dirroot . '/backup/util/settings/setting_dependency.class.php');
             require_once($CFG->dirroot . '/backup/util/settings/root/root_backup_setting.class.php');
             require_once($CFG->dirroot . '/backup/util/settings/activity/activity_backup_setting.class.php');
             require_once($CFG->dirroot . '/backup/util/settings/section/section_backup_setting.class.php');
             require_once($CFG->dirroot . '/backup/util/settings/course/course_backup_setting.class.php');
            +require_once($CFG->dirroot . '/backup/util/ui/backup_ui_setting.class.php');
             
             /*
              * setting tests (all)
            @@ -209,6 +211,21 @@ class setting_test extends UnitTestCase {
                         $this->assertEqual($e->a->alreadydependent, 'test4');
                     }
             
            +        $bs1 = new mock_base_setting('test1', base_setting::IS_INTEGER, null);
            +        $bs2 = new mock_base_setting('test2', base_setting::IS_INTEGER, null);
            +        $bs1->register_dependency(new setting_dependency_disabledif_empty($bs1, $bs2));
            +        try {
            +            // $bs1 is already dependent on $bs2 so this should fail.
            +            $bs2->register_dependency(new setting_dependency_disabledif_empty($bs2, $bs1));
            +            $this->assertTrue(false, 'base_setting_exception expected');
            +        } catch (exception $e) {
            +            $this->assertTrue($e instanceof base_setting_exception);
            +            $this->assertEqual($e->errorcode, 'setting_circular_reference');
            +            $this->assertTrue($e->a instanceof stdclass);
            +            $this->assertEqual($e->a->main, 'test1');
            +            $this->assertEqual($e->a->alreadydependent, 'test2');
            +        }
            +
                     // Create 3 settings and observe between them, last one must
                     // automatically inherit all the settings defined in the main one
                     $bs1 = new mock_base_setting('test1', base_setting::IS_INTEGER, null);

            Show
            samhemelryk Sam Hemelryk added a comment - Hi Petr, sending this back. setting_dependency doesnt have a get_name method. Pretty sure that is meant to be: $a->main = $dependency->get_dependent_setting()->get_name(); I've just been really looking at this and am thinking the following patch is the right way to go. It fixes this issue, fixes the backup settings unit tests and adds a register dependency test. I'm thinking there are bigger problems there as the backup setting tests are missing includes and I haven't seen the unit tests failing... who knows. Either way let me know what you think, if you want feel free to take over again or I'll add it to my queue. My solution: diff --git a/backup/util/settings/base_setting.class.php b/backup/util/settings/base_setting.class.php index 82e6eb1..c969f5e 100644 --- a/backup/util/settings/base_setting.class.php +++ b/backup/util/settings/base_setting.class.php @@ -294,7 +294,7 @@ abstract class base_setting { if ($this->is_circular_reference($dependency->get_dependent_setting())) { $a = new stdclass(); $a->alreadydependent = $this->name; - $a->main = $dependentsetting->get_name(); + $a->main = $dependency->get_dependent_setting()->get_name(); throw new base_setting_exception('setting_circular_reference', $a); } $this->dependencies[$dependency->get_dependent_setting()->get_name()] = $dependency; diff --git a/backup/util/settings/simpletest/testsettings.php b/backup/util/settings/simpletest/testsettings.php index fd079cc..7d0d8a7 100644 --- a/backup/util/settings/simpletest/testsettings.php +++ b/backup/util/settings/simpletest/testsettings.php @@ -32,10 +32,12 @@ require_once($CFG->dirroot . '/backup/util/interfaces/checksumable.class.php'); require_once($CFG->dirroot . '/backup/backup.class.php'); require_once($CFG->dirroot . '/backup/util/settings/base_setting.class.php'); require_once($CFG->dirroot . '/backup/util/settings/backup_setting.class.php'); +require_once($CFG->dirroot . '/backup/util/settings/setting_dependency.class.php'); require_once($CFG->dirroot . '/backup/util/settings/root/root_backup_setting.class.php'); require_once($CFG->dirroot . '/backup/util/settings/activity/activity_backup_setting.class.php'); require_once($CFG->dirroot . '/backup/util/settings/section/section_backup_setting.class.php'); require_once($CFG->dirroot . '/backup/util/settings/course/course_backup_setting.class.php'); +require_once($CFG->dirroot . '/backup/util/ui/backup_ui_setting.class.php'); /* * setting tests (all) @@ -209,6 +211,21 @@ class setting_test extends UnitTestCase { $this->assertEqual($e->a->alreadydependent, 'test4'); } + $bs1 = new mock_base_setting('test1', base_setting::IS_INTEGER, null); + $bs2 = new mock_base_setting('test2', base_setting::IS_INTEGER, null); + $bs1->register_dependency(new setting_dependency_disabledif_empty($bs1, $bs2)); + try { + // $bs1 is already dependent on $bs2 so this should fail. + $bs2->register_dependency(new setting_dependency_disabledif_empty($bs2, $bs1)); + $this->assertTrue(false, 'base_setting_exception expected'); + } catch (exception $e) { + $this->assertTrue($e instanceof base_setting_exception); + $this->assertEqual($e->errorcode, 'setting_circular_reference'); + $this->assertTrue($e->a instanceof stdclass); + $this->assertEqual($e->a->main, 'test1'); + $this->assertEqual($e->a->alreadydependent, 'test2'); + } + // Create 3 settings and observe between them, last one must // automatically inherit all the settings defined in the main one $bs1 = new mock_base_setting('test1', base_setting::IS_INTEGER, null);
            Hide
            skodak Petr Skoda added a comment -

            Reassigning to you, it seems you understand it a lot more than I do, thanks a lot for the review.

            Show
            skodak Petr Skoda added a comment - Reassigning to you, it seems you understand it a lot more than I do, thanks a lot for the review.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Putting up for peer-review

            Show
            samhemelryk Sam Hemelryk added a comment - Putting up for peer-review
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Putting up for integration now so that this makes the next release.

            Show
            samhemelryk Sam Hemelryk added a comment - Putting up for integration now so that this makes the next release.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Some hours ago...

            the main moodle.git repository has been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Some hours ago... the main moodle.git repository has been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated, thanks! (21, 22 and master)

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated, thanks! (21, 22 and master)
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Passing, unit tests completed under all branches.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Passing, unit tests completed under all branches.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            FCT (fixed, closing, thanks). Ciao

            "I feel a very unusual sensation - if it is not indigestion, I think it must be gratitude!"
            ~ Benjamin Disraeli

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - FCT (fixed, closing, thanks). Ciao "I feel a very unusual sensation - if it is not indigestion, I think it must be gratitude!" ~ Benjamin Disraeli

              People

              • Votes:
                1 Vote for this issue
                Watchers:
                1 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/May/12