Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5
    • Component/s: Backup
    • Labels:
    • Testing Instructions:
      Hide

      With debug set to developer mode and showing errors (or alternatively looking to logs), perform these operations, verifying that messages like this are not shown anymore:

      Did you remember to call setType() for ...
      

      Operations:

      • Backup a course.
      • Restore a course.
      • Import activities into a course.
      • Backup an activity.
      • Restore an activity.
      • Duplicate an activity.

      And optionally:

      • Publish a course to some hub (if this fails, plz file another issue).
      Show
      With debug set to developer mode and showing errors (or alternatively looking to logs), perform these operations, verifying that messages like this are not shown anymore: Did you remember to call setType() for ... Operations: Backup a course. Restore a course. Import activities into a course. Backup an activity. Restore an activity. Duplicate an activity. And optionally: Publish a course to some hub (if this fails, plz file another issue).
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      With MDL-34311 landed, the backup and restore forms are reporting thousands of missing types. They should be reviewed and fixed.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Dan Poltawski added a comment -

            Jeesh.. call_user_func_array(array($this->_form, 'addElement'), $setting->get_ui()->get_element_properties($task, $OUTPUT));

            Show
            Dan Poltawski added a comment - Jeesh.. call_user_func_array(array($this->_form, 'addElement'), $setting->get_ui()->get_element_properties($task, $OUTPUT));
            Hide
            Dan Poltawski added a comment -

            So backup_setting_ui_text is going to be problematic.

            Show
            Dan Poltawski added a comment - So backup_setting_ui_text is going to be problematic.
            Hide
            Dan Poltawski added a comment -

            This is as far as I got, but this doesn't look fun.

            diff --git a/backup/restorefile_form.php b/backup/restorefile_form.php
            index f83ee22..f903415 100644
            --- a/backup/restorefile_form.php
            +++ b/backup/restorefile_form.php
            @@ -28,6 +28,7 @@ class course_restore_form extends moodleform {
                     $mform =& $this->_form;
                     $contextid = $this->_customdata['contextid'];
                     $mform->addElement('hidden', 'contextid', $contextid);
            +        $mform->setType('contextid', PARAM_INT);
                     $mform->addElement('filepicker', 'backupfile', get_string('files'));
                     $submit_string = get_string('restore');
                     $this->add_action_buttons(false, $submit_string);
            diff --git a/backup/util/ui/base_moodleform.class.php b/backup/util/ui/base_moodleform.class.php
            index 7a733ee..609ad6d 100644
            --- a/backup/util/ui/base_moodleform.class.php
            +++ b/backup/util/ui/base_moodleform.class.php
            @@ -83,11 +83,15 @@ abstract class base_moodleform extends moodleform {
                     $mform = $this->_form;
                     $mform->setDisableShortforms();
                     $stage = $mform->addElement('hidden', 'stage', $this->uistage->get_stage());
            +        $mform->setType('stage', PARAM_INT);
                     $stage = $mform->addElement('hidden', $ui->get_name(), $ui->get_uniqueid());
            +        $mform->setType($ui->get_name(), PARAM_ALPHANUMEXT);
                     $params = $this->uistage->get_params();
                     if (is_array($params) && count($params) > 0) {
                         foreach ($params as $name=>$value) {
                             $stage = $mform->addElement('hidden', $name, $value);
            +                // TODO, fix this hack!!
            +                $mform->setType($name, PARAM_INT);
                         }
                     }
                 }
            @@ -262,6 +266,7 @@ abstract class base_moodleform extends moodleform {
                         $this->_form->addElement('html', html_writer::end_tag('div'));
                     }
                     $this->_form->addElement('hidden', $settingui->get_name(), $settingui->get_value());
            +        $this->_form->setType($settingui->get_name(), PARAM_BOOL);
                 }
                 /**
                  * Adds dependencies to the form recursively
            

            Show
            Dan Poltawski added a comment - This is as far as I got, but this doesn't look fun. diff --git a/backup/restorefile_form.php b/backup/restorefile_form.php index f83ee22..f903415 100644 --- a/backup/restorefile_form.php +++ b/backup/restorefile_form.php @@ -28,6 +28,7 @@ class course_restore_form extends moodleform { $mform =& $this->_form; $contextid = $this->_customdata['contextid']; $mform->addElement('hidden', 'contextid', $contextid); + $mform->setType('contextid', PARAM_INT); $mform->addElement('filepicker', 'backupfile', get_string('files')); $submit_string = get_string('restore'); $this->add_action_buttons(false, $submit_string); diff --git a/backup/util/ui/base_moodleform.class.php b/backup/util/ui/base_moodleform.class.php index 7a733ee..609ad6d 100644 --- a/backup/util/ui/base_moodleform.class.php +++ b/backup/util/ui/base_moodleform.class.php @@ -83,11 +83,15 @@ abstract class base_moodleform extends moodleform { $mform = $this->_form; $mform->setDisableShortforms(); $stage = $mform->addElement('hidden', 'stage', $this->uistage->get_stage()); + $mform->setType('stage', PARAM_INT); $stage = $mform->addElement('hidden', $ui->get_name(), $ui->get_uniqueid()); + $mform->setType($ui->get_name(), PARAM_ALPHANUMEXT); $params = $this->uistage->get_params(); if (is_array($params) && count($params) > 0) { foreach ($params as $name=>$value) { $stage = $mform->addElement('hidden', $name, $value); + // TODO, fix this hack!! + $mform->setType($name, PARAM_INT); } } } @@ -262,6 +266,7 @@ abstract class base_moodleform extends moodleform { $this->_form->addElement('html', html_writer::end_tag('div')); } $this->_form->addElement('hidden', $settingui->get_name(), $settingui->get_value()); + $this->_form->setType($settingui->get_name(), PARAM_BOOL); } /** * Adds dependencies to the form recursively
            Hide
            Dan Poltawski added a comment -

            Not taking this one. I don't understand it enough.

            Show
            Dan Poltawski added a comment - Not taking this one. I don't understand it enough.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Sending to integration, note I've not considered this like something to document as a change in APIs coz the new methods, although "public" are for internal use of the backup ui stuff.

            Also note that this shouldn't be any security problem (to be backported), because backup/restore has its own validation of all settings, mainly for non-interactive executions (having not ui). See base_setting->validate_value() that is performed both on construction of the setting and when set_value() is called.

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Sending to integration, note I've not considered this like something to document as a change in APIs coz the new methods, although "public" are for internal use of the backup ui stuff. Also note that this shouldn't be any security problem (to be backported), because backup/restore has its own validation of all settings, mainly for non-interactive executions (having not ui). See base_setting->validate_value() that is performed both on construction of the setting and when set_value() is called. Ciao
            Hide
            Dan Poltawski added a comment -

            Integrated, thanks Eloy

            Show
            Dan Poltawski added a comment - Integrated, thanks Eloy
            Hide
            Eloy Lafuente (stronk7) added a comment -

            (thanks Jerome, for getting on this->testing)

            Show
            Eloy Lafuente (stronk7) added a comment - (thanks Jerome, for getting on this->testing)
            Hide
            Jérôme Mouneyrac added a comment - - edited

            Tested with basics of Alpine Mountaineering (school demo course) and a fresh course.

            • restore a course (I processed the restore as a merge) OK
            • backup a course OK
            • Import all activities from a course OK
            • Duplicate a forum OK
            • publish: Failed but OK from backup perspective (Publish > Advertise - the all process + Publish > Share - including some backup part but it's in fact related to the publish code that propagate parameter till the end of the backup process). MDL-38888

            I didn't understand the backup/restore activities tests, is there a specific function for that? I thought it was just a backup/restore course where you only select activities? I'll pass it as OK as I'm pretty sure my tests covered it. Let me know if I'm wrong.

            Not OK but not covered by testing:

            • search on community finder (add community finder block and do a search) MDL-38889
            • admin: register on a specific hub (Server > Hubs + all the registration process) MDL-38890

            Passing. Thanks.

            Show
            Jérôme Mouneyrac added a comment - - edited Tested with basics of Alpine Mountaineering (school demo course) and a fresh course. restore a course (I processed the restore as a merge) OK backup a course OK Import all activities from a course OK Duplicate a forum OK publish: Failed but OK from backup perspective (Publish > Advertise - the all process + Publish > Share - including some backup part but it's in fact related to the publish code that propagate parameter till the end of the backup process). MDL-38888 I didn't understand the backup/restore activities tests, is there a specific function for that? I thought it was just a backup/restore course where you only select activities? I'll pass it as OK as I'm pretty sure my tests covered it. Let me know if I'm wrong. Not OK but not covered by testing: search on community finder (add community finder block and do a search) MDL-38889 admin: register on a specific hub (Server > Hubs + all the registration process) MDL-38890 Passing. Thanks.
            Hide
            Dan Poltawski added a comment -

            Did you remember to call thankDevelopers() for 'this_weeks_work'? Defaulting to PARAM_SHODDY thanking.

            line 1289 of \lib\changes.php: call to debugging()
            line 281 of \lib\are.php: call to moodleform->detectMissingThanks()
            line 202 of \lib\now.php: call to moodleform->_is_poor_form()
            line 73 of \course\upstream.php: call to moodleform->forgetingToThank()

            Show
            Dan Poltawski added a comment - Did you remember to call thankDevelopers() for 'this_weeks_work'? Defaulting to PARAM_SHODDY thanking. line 1289 of \lib\changes.php: call to debugging() line 281 of \lib\are.php: call to moodleform->detectMissingThanks() line 202 of \lib\now.php: call to moodleform->_is_poor_form() line 73 of \course\upstream.php: call to moodleform->forgetingToThank()

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: