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

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

        Gliffy Diagrams

          Issue Links

          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 Skoda added a comment -

            +1

            Show
            Petr Skoda 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 Skoda 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 Skoda 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 Skoda 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 Skoda 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: