Moodle
  1. Moodle
  2. MDL-31175

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

    Details

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

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

        Activity

        Hide
        Sam Hemelryk added a comment -

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

        Show
        Sam Hemelryk added a comment - Hehe is my shotty code so probably fair that I look at this.
        Hide
        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
        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
        Petr Škoda added a comment -

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

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

        Putting up for peer-review

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

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

        Show
        Sam Hemelryk added a comment - Putting up for integration now so that this makes the next release.
        Hide
        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
        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
        Eloy Lafuente (stronk7) added a comment -

        Integrated, thanks! (21, 22 and master)

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

        Passing, unit tests completed under all branches.

        Show
        Eloy Lafuente (stronk7) added a comment - Passing, unit tests completed under all branches.
        Hide
        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
        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: