Moodle
  1. Moodle
  2. MDL-25720

New question engine requires a PARAM_TRIM

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor 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
    • Rank:
      15344

      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.

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment -

          Why not simply trim() the data you get?

          Show
          Petr Škoda added a comment - Why not simply trim() the data you get?
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Eloy Lafuente (stronk7) added a comment -

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

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