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

          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