Moodle
  1. Moodle
  2. MDL-34311

Improve debugging when using default cleaning

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3, 2.5
    • Fix Version/s: 2.5
    • Component/s: Forms Library
    • Testing Instructions:
      Hide
      1. Ensure that the course form displays without any warnings and that course format options can be changed and saved
      2. Edit the course/edit_form.php, comment out this line:
        $mform->setType('idnumber', PARAM_RAW);
      3. VERIFY: that a DEVELOPER debug warning is thrown
      4. Change your debugging level lower.
      5. VERIFY: that a debugging warning is not thrown.
      6. Uncomment the line
      7. Try different forms and ensure they still work

      PLEASE NOTE: there may be more debugging warnings thrown around core with this. These are new bugs to address in new issues.

      Show
      Ensure that the course form displays without any warnings and that course format options can be changed and saved Edit the course/edit_form.php, comment out this line: $mform->setType('idnumber', PARAM_RAW); VERIFY: that a DEVELOPER debug warning is thrown Change your debugging level lower. VERIFY: that a debugging warning is not thrown. Uncomment the line Try different forms and ensure they still work PLEASE NOTE: there may be more debugging warnings thrown around core with this. These are new bugs to address in new issues.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-34311-master
    • Rank:
      42679

      Description

      This is just an idea I wanted to explore...

      Right now people use mforms and it occasionally happens that devs forget to specify a PARAM type and we fall back to PARAM_RAW. This brings us relatively regular security issues. This seems preventable, if devs want PARAM_RAW the they can specify it manually and in other cases, we can throw a developer warning to catch it early.

      I attach a proof of concept patch below. It has the problem that we are throwing warnings in selects etc where not necessary, so we'd need to resolve that. Is there another reason we haven't considered this before?

      diff --git a/lib/formslib.php b/lib/formslib.php
      index 306c4e5..11b4292 100644
      --- a/lib/formslib.php
      +++ b/lib/formslib.php
      @@ -1466,6 +1466,7 @@ class MoodleQuickForm extends HTML_QuickForm_DHTMLRulesTableless {
                           $type = $this->_types[$key];
                       } else {
                           $type = PARAM_RAW;
      +                    debugging("Reverting to defaulting to DEFAULT cleaning of param $key", DEBUG_DEVELOPER);
                       }
                       if (is_array($s)) {
                           $submission[$key] = clean_param_array($s, $type, true);
      

        Issue Links

        Progress
        Resolved Sub-Tasks

        Sub-Tasks

        1.
        Fix missing calls to setType in assign Sub-task Closed Dan Poltawski
         
        2.
        Error reported on question editing form Sub-task Closed Dan Poltawski
         
        3.
        PHP warnings on course restore page Sub-task Closed Dan Poltawski
         
        4.
        Review all admin pages looking for missing setType() uses Sub-task Closed Rajesh Taneja
         
        5.
        Numerous warnings during course restore. Sub-task Closed moodle.com
         
        6.
        Lesson edit page not calling setType for hidden and text inputs Sub-task Closed Rossiani Wijaya
         
        7.
        Fix missing calls to setType in repositories Sub-task Closed Dan Poltawski
         
        8.
        Take rid of missing setType() when uploading users Sub-task Closed Eloy Lafuente (stronk7)
         
        9.
        Add the correct types to all the backup/restore forms Sub-task Closed Eloy Lafuente (stronk7)
         
        10.
        Portfolio element missing setType Sub-task Closed Dan Poltawski
         
        11.
        Fix missing calls to setType in enrolment methods Sub-task Closed David Monllaó
         
        12.
        Multianswer question form not calling setType on hidden input "confirm" Sub-task Closed Dan Poltawski
         
        13.
        Glossary not calling setType for text input entbypage Sub-task Closed Dan Poltawski
         
        14.
        Fix missing setType calls when editing blocks Sub-task Closed David Monllaó
         
        15.
        Missing call to setType in "My private files" form Sub-task Closed Matteo Scaramuccia
         
        16.
        user/files.php sets $data->returnurl without defining in form Sub-task Closed Dan Poltawski
         
        17.
        Missing call to setType in signup form Sub-task Closed Dan Poltawski
         
        18.
        Apply proper cleaning to phpunit involved forms to let it pass Sub-task Closed Eloy Lafuente (stronk7)
         
        19.
        Course completion form not calling setType for criteria_grade_value and locale aware floats Sub-task Closed Dan Poltawski
         
        20.
        Error when modifying student grade Sub-task Closed Andrew Davis
         
        21.
        Fix missing calls to setType in scorm mod_form. Sub-task Closed Dan Marsden
         
        22.
        Many PARAM_RAW warnings when editing a grade forms Sub-task Closed Dan Poltawski
         
        23.
        Missing setType calls in mod_choice Sub-task Closed Dan Poltawski
         
        24.
        setType debugging should not be displayed on hardFreeze() elements Sub-task Closed Dan Poltawski
         
        25.
        Fix missing calls to setType in calendar Sub-task Closed Dan Poltawski
         
        26.
        Database pages show setType() warnings Sub-task Closed Rossiani Wijaya
         
        27.
        setType errors when adding a new category Sub-task Closed Dan Poltawski
         
        28.
        Add unit tests for the debugging change (and don't call detectMissingSetType twice) Sub-task Closed Dan Poltawski
         
        29.
        Completion settings page shows setType() warnings Sub-task Closed Dan Poltawski
         
        30.
        Fix missing setType calls in advanced grading Sub-task Closed Dan Poltawski
         
        31.
        Fix missing setType calls in acceptance testing tool Sub-task Closed David Monllaó
         
        32.
        Fix missing setType in workshop random allocator Sub-task Closed David Mudrak
         
        33.
        Fix missing calls to setType in blog Sub-task Closed Ankit Agarwal
         
        34.
        Workshop submission setType error Sub-task Closed Rossiani Wijaya
         
        35.
        Missing call to SetType in mod_folder Sub-task Closed Stephen Bourget
         
        36.
        quiz/startattempt.php show setType warning. Sub-task Closed Tim Hunt
         
        37.
        Groups import missing setType Sub-task Closed Dan Poltawski
         
        38.
        assignment upgrade tool setType warnings Sub-task Closed Damyon Wiese
         
        39.
        Missing setType() in portfolio format select form Sub-task Closed Dan Poltawski
         
        40.
        Community finder search display setType() form warnings Sub-task Closed Jérôme Mouneyrac
         
        41.
        Fix missing calls to setType in networking Sub-task Closed Rossiani Wijaya
         
        42.
        Fix missing calls to setType() for externalURL in assignment Sub-task Closed Rossiani Wijaya
         
        43.
        setType() warnings in Box.net and Flickr portfolio export forms Sub-task Closed Rossiani Wijaya
         
        44.
        Missing setType call in Feedback forms Sub-task Closed Frédéric Massart
         
        45.
        Missing setType in course legacy files Sub-task Closed Marina Glancy
         
        46.
        Missing setType call for advanced grading. Sub-task Closed Damyon Wiese
         
        47.
        setType() message when editing a tag Sub-task Closed Dan Poltawski
         
        48.
        Fix missing calls to setType in course criteria settings Sub-task Closed Yuliya Bozhko
         
        49.
        Fix missing setType calls when exporting gradebook Sub-task Closed Rajesh Taneja
         

          Activity

          Hide
          Dan Poltawski added a comment -

          Adding some watchers who might know why this is a bad idea.

          Show
          Dan Poltawski added a comment - Adding some watchers who might know why this is a bad idea.
          Hide
          Petr Škoda added a comment -

          +1

          Show
          Petr Škoda added a comment - +1
          Hide
          Tim Hunt added a comment -

          +1

          Show
          Tim Hunt added a comment - +1
          Hide
          Sam Hemelryk added a comment -

          Definitely +1

          Show
          Sam Hemelryk added a comment - Definitely +1
          Hide
          Dan Poltawski added a comment -

          OK, sending for peer review.

          1. I think that text and hidden fields are the only elements that we should enforce this in. But I'd appreciate the input of other experts! (for a long time I was setting a PARAM type on selects, and told it was not necessary.
          2. I've done one fix for the course form and the dynamic adding of options which is one of those formslib hacks.. I didn't look deeply at this other than suppressing the warnings. I think its better we do the warning at display time
          Show
          Dan Poltawski added a comment - OK, sending for peer review. I think that text and hidden fields are the only elements that we should enforce this in. But I'd appreciate the input of other experts! (for a long time I was setting a PARAM type on selects, and told it was not necessary. I've done one fix for the course form and the dynamic adding of options which is one of those formslib hacks.. I didn't look deeply at this other than suppressing the warnings. I think its better we do the warning at display time
          Hide
          Petr Škoda added a comment -

          I think this should explain the weird PARAM_TEXT parameter type and the fact that we clean html before output, not after input.

          Show
          Petr Škoda added a comment - I think this should explain the weird PARAM_TEXT parameter type and the fact that we clean html before output, not after input.
          Hide
          Frédéric Massart added a comment -

          Hi Dan,

          great idea, here are some minor comments:

          1. I'm not really a fan of having 200 blank spaces in a debug message, I guess this one should never be displayed outside of an HTML page, but my personal preference goes for a concatenated string. (Very trivial )
          2. I think you should set a default value on the URL elements, or edit the element to default itself to PARAM_URL.
          3. The element cmidnumber added in moodleform_mod@standard_coursemodule_elements, uses a type text, and could use a setType.
          4. I'm not convinced that the debugging messages should be placed in display(). My style would go for a new method which encapsulates the logic you've added, and call it from the places where it's needed the most. I was looking for the place where we're sure the definition has been set, but that's in display() and validate(), so I'm not sure if it should be part of both or none.

          Anyway, that's looking good.

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Hi Dan, great idea, here are some minor comments: I'm not really a fan of having 200 blank spaces in a debug message, I guess this one should never be displayed outside of an HTML page, but my personal preference goes for a concatenated string. (Very trivial ) I think you should set a default value on the URL elements, or edit the element to default itself to PARAM_URL. The element cmidnumber added in moodleform_mod@standard_coursemodule_elements, uses a type text, and could use a setType. I'm not convinced that the debugging messages should be placed in display(). My style would go for a new method which encapsulates the logic you've added, and call it from the places where it's needed the most. I was looking for the place where we're sure the definition has been set, but that's in display() and validate(), so I'm not sure if it should be part of both or none. Anyway, that's looking good. Cheers, Fred
          Hide
          Dan Poltawski added a comment -

          Hi Fred,

          Thanks for the review, i've updated it based on your comments:

          1. Fixed, thanks.
          2. Good spot, i've added this. I've noticed this is missing in various places, which is pretty bad since the url seems to do very little. I'm not confident enough to set a default for it though. Best leave that up to developers. (Could be some people want PARAM_LOCALURL for e.g.).
          3. Added a commit for it.
          4. I did consider adding as a function this when I was creating the patch, but I was slightly put off by adding a new function which sole purpose is for a developer debugging. But you're request has convinced me its better and so i've done that.

          I've made it be called in display() and updateSubmission(). I think it needs to be called in display() because otherwise we'll hardly ever see the debugging message, better make it easier to spot and updateSubmission() because thats actually where we process the data coming through.

          Show
          Dan Poltawski added a comment - Hi Fred, Thanks for the review, i've updated it based on your comments: 1. Fixed, thanks. 2. Good spot, i've added this. I've noticed this is missing in various places, which is pretty bad since the url seems to do very little. I'm not confident enough to set a default for it though. Best leave that up to developers. (Could be some people want PARAM_LOCALURL for e.g.). 3. Added a commit for it. 4. I did consider adding as a function this when I was creating the patch, but I was slightly put off by adding a new function which sole purpose is for a developer debugging. But you're request has convinced me its better and so i've done that. I've made it be called in display() and updateSubmission(). I think it needs to be called in display() because otherwise we'll hardly ever see the debugging message, better make it easier to spot and updateSubmission() because thats actually where we process the data coming through.
          Hide
          Frédéric Massart added a comment - - edited

          Cheers Dan.

          1. I'm not sure about placing the check in _process_submission, but if the idea is to display something not only when the form is displayed, but also when it gets data and there is nowhere else to place it, then fine with me.
          2. I noticed this morning that elements part of a group are not in the _elements array, so they won't throw any debugging message if they don't have a rule set. Though perhaps it's not even possible to set a rule on these elements in the first place.
          3. Do you think some developers would extend display()? If yes, your warning could be lost.
          Show
          Frédéric Massart added a comment - - edited Cheers Dan. I'm not sure about placing the check in _process_submission, but if the idea is to display something not only when the form is displayed, but also when it gets data and there is nowhere else to place it, then fine with me. I noticed this morning that elements part of a group are not in the _elements array, so they won't throw any debugging message if they don't have a rule set. Though perhaps it's not even possible to set a rule on these elements in the first place. Do you think some developers would extend display() ? If yes, your warning could be lost.
          Hide
          Dan Poltawski added a comment -

          1. Yep, and its also at this point we actually do the cleaning.
          2. Seems beyond the scope of this issue. Worth creating a bug for though.
          3. I've never seen in that happen since mforms was introduced. But in any case thats just as good as now (and they'll get it on submission)

          (sending for integration)

          Show
          Dan Poltawski added a comment - 1. Yep, and its also at this point we actually do the cleaning. 2. Seems beyond the scope of this issue. Worth creating a bug for though. 3. I've never seen in that happen since mforms was introduced. But in any case thats just as good as now (and they'll get it on submission) (sending for integration)
          Hide
          Damyon Wiese added a comment -

          Thanks for this Dan,

          I did some testing and it does trigger alot of debugging messages. I'll new create bugs for the ones I spotted - if the testers could do the same it would be helpful. I didn't find any false positives so this is generally a helpful change.

          Integrated to master.

          Show
          Damyon Wiese added a comment - Thanks for this Dan, I did some testing and it does trigger alot of debugging messages. I'll new create bugs for the ones I spotted - if the testers could do the same it would be helpful. I didn't find any false positives so this is generally a helpful change. Integrated to master.
          Hide
          Dan Poltawski added a comment -

          Hmm, this one in quiz doesn't look good actually:

          Did you remember to call setType() for 'feedbackboundaries[3]'? Defaulting to PARAM_RAW cleaning.
          line 1266 of /lib/formslib.php: call to debugging()
          line 281 of /lib/formslib.php: call to moodleform->detectMissingSetType()
          line 202 of /lib/formslib.php: call to moodleform->_process_submission()
          line 71 of /course/moodleform_mod.php: call to moodleform->moodleform()
          line 53 of /mod/quiz/mod_form.php: call to moodleform_mod->moodleform_mod()
          line 248 of /course/modedit.php: call to mod_quiz_mod_form->__construct()

          Show
          Dan Poltawski added a comment - Hmm, this one in quiz doesn't look good actually: Did you remember to call setType() for 'feedbackboundaries [3] '? Defaulting to PARAM_RAW cleaning. line 1266 of /lib/formslib.php: call to debugging() line 281 of /lib/formslib.php: call to moodleform->detectMissingSetType() line 202 of /lib/formslib.php: call to moodleform->_process_submission() line 71 of /course/moodleform_mod.php: call to moodleform->moodleform() line 53 of /mod/quiz/mod_form.php: call to moodleform_mod->moodleform_mod() line 248 of /course/modedit.php: call to mod_quiz_mod_form->__construct()
          Hide
          Eloy Lafuente (stronk7) added a comment -

          grrr,

          phpunit tests failing:

          http://stronk7.doesntexist.com/job/07.%20Run%20phpunit%20UnitTests%20(master)/2184/console

          plus... also... is this covered by own tests?

          Show
          Eloy Lafuente (stronk7) added a comment - grrr, phpunit tests failing: http://stronk7.doesntexist.com/job/07.%20Run%20phpunit%20UnitTests%20(master)/2184/console plus... also... is this covered by own tests?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I've created MDL-38719 about to fix the occurrences related with phpunit execution. Maybe some will overlap with some of the other related issues, but we need unit tests passing asap, so it gets precedence. Working on it now.

          Show
          Eloy Lafuente (stronk7) added a comment - I've created MDL-38719 about to fix the occurrences related with phpunit execution. Maybe some will overlap with some of the other related issues, but we need unit tests passing asap, so it gets precedence. Working on it now.
          Hide
          Dan Poltawski added a comment -

          Wow, I am shocked that we've managed to unit test formslib output.

          Show
          Dan Poltawski added a comment - Wow, I am shocked that we've managed to unit test formslib output.
          Hide
          Tim Hunt added a comment -

          Dan, those quiz boundaries are either '17' or '80%' so PARAM_RAW is right. The values are carefully validated, parsed and cleaned later.

          (And previously, PARAM_RAW was a default, and no-one was objecting to code that relied in the default until your change.)

          Show
          Tim Hunt added a comment - Dan, those quiz boundaries are either '17' or '80%' so PARAM_RAW is right. The values are carefully validated, parsed and cleaned later. (And previously, PARAM_RAW was a default, and no-one was objecting to code that relied in the default until your change.)
          Hide
          Dan Poltawski added a comment -

          Tim: I meant that it was not good that it isn't being detected. It looks like you've set it as PARAM_RAW in the repeated options, but my code isn't detecting that explicit definition.

          Show
          Dan Poltawski added a comment - Tim: I meant that it was not good that it isn't being detected. It looks like you've set it as PARAM_RAW in the repeated options, but my code isn't detecting that explicit definition.
          Hide
          Petr Škoda added a comment -

          Please stop calling PARAM_TEXT a "plaintext", it is HTML fragment without tags, html entities work fine there. Arbitrary characters can be "plaintext", what matters is how you print it to page output - echo() x p(). It is sad how many people are confused by PARAM_TEXT, previously everybody used PARAM_CLEAN without thinking, now PARAM_TEXT is abused the same way...

          Show
          Petr Škoda added a comment - Please stop calling PARAM_TEXT a "plaintext", it is HTML fragment without tags, html entities work fine there. Arbitrary characters can be "plaintext", what matters is how you print it to page output - echo() x p(). It is sad how many people are confused by PARAM_TEXT, previously everybody used PARAM_CLEAN without thinking, now PARAM_TEXT is abused the same way...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Dan, repeated element treat type in an special way, and it's set for an "unexisting" element instead of foreach repetition. It got all the morning here to inspect how nasty is that integration/wrapping.

          I think that, with the code added @ MDL-38719, now your detector is looking properly inside of repeated elements.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Dan, repeated element treat type in an special way, and it's set for an "unexisting" element instead of foreach repetition. It got all the morning here to inspect how nasty is that integration/wrapping. I think that, with the code added @ MDL-38719 , now your detector is looking properly inside of repeated elements. Ciao
          Hide
          Dan Poltawski added a comment -

          Thanks for fixing that Eloy (sorry I didn't do it).

          Petr: not sure who you are addressing that to!

          Show
          Dan Poltawski added a comment - Thanks for fixing that Eloy (sorry I didn't do it). Petr: not sure who you are addressing that to!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Ah, I think Petr commented here because, while I was investigating formslib, I called him about this piece of docs:

          http://docs.moodle.org/dev/lib/formslib.php_Form_Definition#Most_Commonly_Used_PARAM_.2A_Types

          (again mixing PARAM_TEXT with plain-text, so surely that's the cause for his comment above).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Ah, I think Petr commented here because, while I was investigating formslib, I called him about this piece of docs: http://docs.moodle.org/dev/lib/formslib.php_Form_Definition#Most_Commonly_Used_PARAM_.2A_Types (again mixing PARAM_TEXT with plain-text, so surely that's the cause for his comment above). Ciao
          Hide
          David Monllaó added a comment -

          Hi,

          Sorry, this sounds a bit off-topic but is related with this issue, the new debugging("Did you remember to call setType()...") is causing a few behat tests to fail due to the prevented redirection when using redirect(). Behat test site runs with debug to max level to detect more issues, the redirection is not really the problem as we can avoid it hacking the debugging() or children functions code, but we are supposed to fail tests when debugging messages are thrown (related work done in MDL-38041 but stopped https://tracker.moodle.org/browse/MDL-38041?focusedCommentId=204503&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-204503) but if master branch can contain debugging messages we can not wait until all the related issues are fixed to have a 100% pass again.

          I propose to add a $CFG->behat_debug = false|true and run behat in stable branches (after 2.5 release) with debug to max, and master with $CFG->behat_debug off in the CI suite runs, but also running master with $CFG->behat_debug = true in the CI server once per week to detect all this issues, but without affecting the integration tests results.

          Waiting for feedback before creating the related issue.

          Pasting the missing setType() behat tests detected:

          When adding mod_choice instances

          Did you remember to call setType() for 'limit'? Defaulting to PARAM_RAW cleaning.
          
              line 1275 of /lib/formslib.php: call to debugging()
              line 281 of /lib/formslib.php: call to moodleform->detectMissingSetType()
              line 202 of /lib/formslib.php: call to moodleform->_process_submission()
              line 71 of /course/moodleform_mod.php: call to moodleform->moodleform()
              line 248 of /course/modedit.php: call to moodleform_mod->moodleform_mod()
          
          Did you remember to call setType() for 'limit'? Defaulting to PARAM_RAW cleaning.
          
              line 1275 of /lib/formslib.php: call to debugging()
              line 281 of /lib/formslib.php: call to moodleform->detectMissingSetType()
              line 202 of /lib/formslib.php: call to moodleform->_process_submission()
              line 71 of /course/moodleform_mod.php: call to moodleform->moodleform()
              line 248 of /course/modedit.php: call to moodleform_mod->moodleform_mod()
          
          Did you remember to call setType() for 'limit'? Defaulting to PARAM_RAW cleaning.
          
              line 1275 of /lib/formslib.php: call to debugging()
              line 281 of /lib/formslib.php: call to moodleform->detectMissingSetType()
              line 202 of /lib/formslib.php: call to moodleform->_process_submission()
              line 71 of /course/moodleform_mod.php: call to moodleform->moodleform()
              line 248 of /course/modedit.php: call to moodleform_mod->moodleform_mod()
          
          Did you remember to call setType() for 'limit'? Defaulting to PARAM_RAW cleaning.
          
              line 1275 of /lib/formslib.php: call to debugging()
              line 281 of /lib/formslib.php: call to moodleform->detectMissingSetType()
              line 202 of /lib/formslib.php: call to moodleform->_process_submission()
              line 71 of /course/moodleform_mod.php: call to moodleform->moodleform()
              line 248 of /course/modedit.php: call to moodleform_mod->moodleform_mod()
          
          Did you remember to call setType() for 'limit'? Defaulting to PARAM_RAW cleaning.
          
              line 1275 of /lib/formslib.php: call to debugging()
              line 281 of /lib/formslib.php: call to moodleform->detectMissingSetType()
              line 202 of /lib/formslib.php: call to moodleform->_process_submission()
              line 71 of /course/moodleform_mod.php: call to moodleform->moodleform()
              line 248 of /course/modedit.php: call to moodleform_mod->moodleform_mod()
          

          Editing the user profile

          Did you remember to call setType() for 'email'? Defaulting to PARAM_RAW cleaning.
          
              line 1275 of /lib/formslib.php: call to debugging()
              line 281 of /lib/formslib.php: call to moodleform->detectMissingSetType()
              line 202 of /lib/formslib.php: call to moodleform->_process_submission()
              line 150 of /user/editadvanced.php: call to moodleform->moodleform()
          
          Show
          David Monllaó added a comment - Hi, Sorry, this sounds a bit off-topic but is related with this issue, the new debugging("Did you remember to call setType()...") is causing a few behat tests to fail due to the prevented redirection when using redirect(). Behat test site runs with debug to max level to detect more issues, the redirection is not really the problem as we can avoid it hacking the debugging() or children functions code, but we are supposed to fail tests when debugging messages are thrown (related work done in MDL-38041 but stopped https://tracker.moodle.org/browse/MDL-38041?focusedCommentId=204503&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-204503 ) but if master branch can contain debugging messages we can not wait until all the related issues are fixed to have a 100% pass again. I propose to add a $CFG->behat_debug = false|true and run behat in stable branches (after 2.5 release) with debug to max, and master with $CFG->behat_debug off in the CI suite runs, but also running master with $CFG->behat_debug = true in the CI server once per week to detect all this issues, but without affecting the integration tests results. Waiting for feedback before creating the related issue. Pasting the missing setType() behat tests detected: When adding mod_choice instances Did you remember to call setType() for 'limit'? Defaulting to PARAM_RAW cleaning. line 1275 of /lib/formslib.php: call to debugging() line 281 of /lib/formslib.php: call to moodleform->detectMissingSetType() line 202 of /lib/formslib.php: call to moodleform->_process_submission() line 71 of /course/moodleform_mod.php: call to moodleform->moodleform() line 248 of /course/modedit.php: call to moodleform_mod->moodleform_mod() Did you remember to call setType() for 'limit'? Defaulting to PARAM_RAW cleaning. line 1275 of /lib/formslib.php: call to debugging() line 281 of /lib/formslib.php: call to moodleform->detectMissingSetType() line 202 of /lib/formslib.php: call to moodleform->_process_submission() line 71 of /course/moodleform_mod.php: call to moodleform->moodleform() line 248 of /course/modedit.php: call to moodleform_mod->moodleform_mod() Did you remember to call setType() for 'limit'? Defaulting to PARAM_RAW cleaning. line 1275 of /lib/formslib.php: call to debugging() line 281 of /lib/formslib.php: call to moodleform->detectMissingSetType() line 202 of /lib/formslib.php: call to moodleform->_process_submission() line 71 of /course/moodleform_mod.php: call to moodleform->moodleform() line 248 of /course/modedit.php: call to moodleform_mod->moodleform_mod() Did you remember to call setType() for 'limit'? Defaulting to PARAM_RAW cleaning. line 1275 of /lib/formslib.php: call to debugging() line 281 of /lib/formslib.php: call to moodleform->detectMissingSetType() line 202 of /lib/formslib.php: call to moodleform->_process_submission() line 71 of /course/moodleform_mod.php: call to moodleform->moodleform() line 248 of /course/modedit.php: call to moodleform_mod->moodleform_mod() Did you remember to call setType() for 'limit'? Defaulting to PARAM_RAW cleaning. line 1275 of /lib/formslib.php: call to debugging() line 281 of /lib/formslib.php: call to moodleform->detectMissingSetType() line 202 of /lib/formslib.php: call to moodleform->_process_submission() line 71 of /course/moodleform_mod.php: call to moodleform->moodleform() line 248 of /course/modedit.php: call to moodleform_mod->moodleform_mod() Editing the user profile Did you remember to call setType() for 'email'? Defaulting to PARAM_RAW cleaning. line 1275 of /lib/formslib.php: call to debugging() line 281 of /lib/formslib.php: call to moodleform->detectMissingSetType() line 202 of /lib/formslib.php: call to moodleform->_process_submission() line 150 of /user/editadvanced.php: call to moodleform->moodleform()
          Hide
          Dan Poltawski added a comment -

          David: I don't think you should do any tests without debugging set to max, that is the wrong direction. We should try and fix them all this week. (It's my fault for introducing such a destructive change without all of the fixes, I will do all the ones breaking automated tests).

          Show
          Dan Poltawski added a comment - David: I don't think you should do any tests without debugging set to max, that is the wrong direction. We should try and fix them all this week. (It's my fault for introducing such a destructive change without all of the fixes, I will do all the ones breaking automated tests).
          Hide
          David Monllaó added a comment -

          Ok if master is not supposed to have debugging() messages, there is no need for new setting.

          As far as I've seen is only

          • edit user profile -> email
          • mod_choice add/edit instance -> limit
          Show
          David Monllaó added a comment - Ok if master is not supposed to have debugging() messages, there is no need for new setting. As far as I've seen is only edit user profile -> email mod_choice add/edit instance -> limit
          Hide
          Eloy Lafuente (stronk7) added a comment -

          (agree, always debugging to max, and any debugging/notice/warning output should lead to failed test)

          Show
          Eloy Lafuente (stronk7) added a comment - (agree, always debugging to max, and any debugging/notice/warning output should lead to failed test)
          Hide
          Dan Poltawski added a comment -

          David, the fixes are in MDL-38717 and MDL-38736 and i'll be bribing Damyon to integrate them.

          Show
          Dan Poltawski added a comment - David, the fixes are in MDL-38717 and MDL-38736 and i'll be bribing Damyon to integrate them.
          Hide
          David Monllaó added a comment -

          All as expected. I've added a related issue MDL-38744

          With both mod_choice and user profile forms patches integrated the tests suite is passing again.

          Show
          David Monllaó added a comment - All as expected. I've added a related issue MDL-38744 With both mod_choice and user profile forms patches integrated the tests suite is passing again.
          Hide
          Michael de Raadt added a comment -

          It's good that this change is revealing where forms can be improved.

          Hopefully we'll uncover more during the upcoming QA period.

          Show
          Michael de Raadt added a comment - It's good that this change is revealing where forms can be improved. Hopefully we'll uncover more during the upcoming QA period.
          Hide
          Damyon Wiese added a comment -

          Thanks for your hard work. This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads).

          Show
          Damyon Wiese added a comment - Thanks for your hard work. This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads).
          Hide
          Dan Poltawski added a comment -

          Added unit tests for this in MDL-38897

          Show
          Dan Poltawski added a comment - Added unit tests for this in MDL-38897

            People

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

              Dates

              • Created:
                Updated:
                Resolved: