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:
    • Rank:
      48791

      Description

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

        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: