Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-25720

New question engine requires a PARAM_TRIM

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0.2
    • Component/s: Libraries
    • Labels:
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE

      Description

      This is needed by, for example, the short-answer and numerical question types. It is basically the same as PARAM_RAW, but it strips leading and trailing white-space.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              skodak Petr Skoda added a comment -

              Why not simply trim() the data you get?

              Show
              skodak Petr Skoda added a comment - Why not simply trim() the data you get?
              Hide
              timhunt Tim Hunt added a comment -

              The reason is to do with how the question engine processes submitted data.

              In Moodle 1.9 it basically did

              foreach ($_POST as $name => $value) {
                  if (preg_match('/^q(\d+)_(.+)$/), $name, $matches)) {
                      $responses[$matches[1]][$matches[2]] = $value;
                  }
              }
               
              foreach ($questions as $questionid => $question) {
                  process_responses($question, $responses[$questionid]);
              }

              That was bad, because it is not using optional/required_param. Also, if you used firebug to add hidden form fields to the HTML (or something) you could get Moodle to accept arbitrary data.

              In the new question engine the code is more like:

              foreach ($questions as $questionid => $q) {
                  $expected = $q->get_expected_vars();
               
                  foreach ($expected as $name => $type) {
                      $response[$name] = optional_param($q->prefix() . $name, null, $type);
                  }
               
                  $engine->save_response($q->id, $response);
               
                  $q->process_response($response);
              }

              The point is that we store the response in the database exactly as we get it from optional_param.

              That is why PARAM_TRIM makes sense. The only other options is to add trim calls in lots of different places that process the responses (grading, reports, display, ...).

              Also, it is a nice declarative way of saying what sort of input you expect. Note that a lot of other PARAM types effectively do a trim (PARAM_INT, PARAM_ALPHA, etc.) because they strip all whitespace. PARAM_TRIM would probably be useful in other places too.

              However, if you don't want this in core, there are ways I could work-around it. For example, I could add a $q->cleanup_input() method in the question types.

              Show
              timhunt Tim Hunt added a comment - The reason is to do with how the question engine processes submitted data. In Moodle 1.9 it basically did foreach ($_POST as $name => $value) { if (preg_match('/^q(\d+)_(.+)$/), $name, $matches)) { $responses[$matches[1]][$matches[2]] = $value; } }   foreach ($questions as $questionid => $question) { process_responses($question, $responses[$questionid]); } That was bad, because it is not using optional/required_param. Also, if you used firebug to add hidden form fields to the HTML (or something) you could get Moodle to accept arbitrary data. In the new question engine the code is more like: foreach ($questions as $questionid => $q) { $expected = $q->get_expected_vars();   foreach ($expected as $name => $type) { $response[$name] = optional_param($q->prefix() . $name, null, $type); }   $engine->save_response($q->id, $response);   $q->process_response($response); } The point is that we store the response in the database exactly as we get it from optional_param. That is why PARAM_TRIM makes sense. The only other options is to add trim calls in lots of different places that process the responses (grading, reports, display, ...). Also, it is a nice declarative way of saying what sort of input you expect. Note that a lot of other PARAM types effectively do a trim (PARAM_INT, PARAM_ALPHA, etc.) because they strip all whitespace. PARAM_TRIM would probably be useful in other places too. However, if you don't want this in core, there are ways I could work-around it. For example, I could add a $q->cleanup_input() method in the question types.
              Hide
              timhunt Tim Hunt added a comment -

              OK, After discussing with Andrew Davis, I think PARAM_RAW_TRIMMED is the right naming convention. That is clear, and easy to extend to other things later.

              I'm working on a new pull request.

              Show
              timhunt Tim Hunt added a comment - OK, After discussing with Andrew Davis, I think PARAM_RAW_TRIMMED is the right naming convention. That is clear, and easy to extend to other things later. I'm working on a new pull request.
              Hide
              timhunt Tim Hunt added a comment -

              PULL-41 submitted

              (For reference, the previous pull request was PULL-24).

              Related dev chat: http://moodle.org/mod/cvsadmin/view.php?conversationid=6367#c249150

              Show
              timhunt Tim Hunt added a comment - PULL-41 submitted (For reference, the previous pull request was PULL-24). Related dev chat: http://moodle.org/mod/cvsadmin/view.php?conversationid=6367#c249150
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              This has been finally integrated but with discussion/objections:

              1. This way of handling (manipulating) data is, in the long term unsustainable. We cannot end with PARAM_RAW_FIRSTUPPERCASE, PARAM_RAW_CHINESE, PARAM_RAW_WHATEVER. More yet, what if you want to trim one URL, will be also creating PARAM_URL_TRIM ?
              2. RAW stands for no cleaning so all the variables being validated that way aren't really being validated at all agains any known type (alpha, num, text, html, url...), so its use is clearly NOT recommended in general.
              3. Current status of PARAM_XXXX is one horrible mix of all sort of things (validation agains types, manipulating information and validation against DB). Horrible, horrible.

              So, we need to fix all those things, perhaps by:

              1. Separating real validators (know types of data) from manipulators.
              2. Allowing formslib stuff to know and apply manipulators.
              3. Reduce the use of any RAW handling to the minimum possible, always use real validators.

              Feel free to comment about this in HQ / propose anything / create new issue... but I don't think we can continue with this approach at all. Finally, Tim, use it with care (sure you will, lol) remember it isn't safe

              Thanks and ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - This has been finally integrated but with discussion/objections: This way of handling (manipulating) data is, in the long term unsustainable. We cannot end with PARAM_RAW_FIRSTUPPERCASE, PARAM_RAW_CHINESE, PARAM_RAW_WHATEVER. More yet, what if you want to trim one URL, will be also creating PARAM_URL_TRIM ? RAW stands for no cleaning so all the variables being validated that way aren't really being validated at all agains any known type (alpha, num, text, html, url...), so its use is clearly NOT recommended in general. Current status of PARAM_XXXX is one horrible mix of all sort of things (validation agains types, manipulating information and validation against DB). Horrible, horrible. So, we need to fix all those things, perhaps by: Separating real validators (know types of data) from manipulators. Allowing formslib stuff to know and apply manipulators. Reduce the use of any RAW handling to the minimum possible, always use real validators. Feel free to comment about this in HQ / propose anything / create new issue... but I don't think we can continue with this approach at all. Finally, Tim, use it with care (sure you will, lol) remember it isn't safe Thanks and ciao
              Hide
              timhunt Tim Hunt added a comment -

              Sure, one day the whole PARAM_... validation thing could be cleaned up a lot. Probably at the same time that we rewrite formslib. However, for now, I need this, so thank you for integrating it.

              Show
              timhunt Tim Hunt added a comment - Sure, one day the whole PARAM_... validation thing could be cleaned up a lot. Probably at the same time that we rewrite formslib. However, for now, I need this, so thank you for integrating it.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Closed. Will land to upstream in hours. Thanks and ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Closed. Will land to upstream in hours. Thanks and ciao

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    21/Feb/11