Sam Hemelryk added a comment - 23/Jan/12 3:40 PM Hehe is my shotty code so probably fair that I look at this.

Sam Hemelryk added a comment - 23/Jan/12 3:59 PM 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 );

Petr Škoda added a comment - 27/Feb/12 3:02 PM Reassigning to you, it seems you understand it a lot more than I do, thanks a lot for the review.

Sam Hemelryk added a comment - 01/Mar/12 11:51 AM Putting up for peer-review

Sam Hemelryk added a comment - 07/Mar/12 5:20 AM Putting up for integration now so that this makes the next release.

Eloy Lafuente (stronk7) added a comment - 10/Mar/12 5:30 AM 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

Eloy Lafuente (stronk7) added a comment - 13/Mar/12 11:18 PM Integrated, thanks! (21, 22 and master)

Eloy Lafuente (stronk7) added a comment - 13/Mar/12 11:18 PM Passing, unit tests completed under all branches.

Eloy Lafuente (stronk7) added a comment - 15/Mar/12 8:23 PM FCT (fixed, closing, thanks). Ciao "I feel a very unusual sensation - if it is not indigestion, I think it must be gratitude!" ~ Benjamin Disraeli