Moodle
  1. Moodle
  2. MDL-27728

javascript form validation library

    Details

    • Database:
      Any
    • Testing Instructions:
      Hide

      Can't think of a case where this case is reproducible. But make sure all the forms with required field get submitted.
      1. Login as student
      2. Edit profile (Settings -> Edit profile)
      3. Try updating profile with empty required fields.
      4. Form should not be submitted and display error message on fields.
      5. Try add valid values and forms should be submitted.

      Try perform similar testing on other forms with required fields and make sure form is not submitted with invalid input.

      Show
      Can't think of a case where this case is reproducible. But make sure all the forms with required field get submitted. 1. Login as student 2. Edit profile (Settings -> Edit profile) 3. Try updating profile with empty required fields. 4. Form should not be submitted and display error message on fields. 5. Try add valid values and forms should be submitted. Try perform similar testing on other forms with required fields and make sure form is not submitted with invalid input.
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull Master Branch:
      wip-mdl-27728
    • Rank:
      17378

      Description

      Within lib/formslib.php on form submit, the code is intended to step through the form elements and validate the value of each element.

      Unfortunately, there are occasions when the element variable within the loop is undefined. This throws an exception. Since the form action has on exception return true, the form then gets submitted even though its values might not pass validation.

      This can be avoided as follows:

      Around line 1764 find

      function validate_' . $this->_formName . '_' . $escapedElementName . '(element) {
      

      Insert the following lines after it:

      if( undefined == element ){
           return false;
      }
      

      Then, around lines 1769 - 1774, find the range of lines beginning with

      var frm = element.parentNode;
      

      and ending with

      return qf_errorHandler(element, _qfMsg);
      

      Enclose this block in a condition like this:

          if( undefined != element.name ){
            var frm = element.parentNode;
            while (frm && frm.nodeName.toUpperCase() != "FORM") {
             frm = frm.parentNode;
            }
      ' . join("\n", $jsArr) . '
      
            return qf_errorHandler(element, _qfMsg);
          }
      

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for reporting this.

          I've put it on our backlog and we'll try to get to it as soon as we can.

          In the meantime adding more information, such as replication instructions, fix test instructions and a workaround, will help us and other users.

          Show
          Michael de Raadt added a comment - Thanks for reporting this. I've put it on our backlog and we'll try to get to it as soon as we can. In the meantime adding more information, such as replication instructions, fix test instructions and a workaround, will help us and other users.
          Hide
          Rajesh Taneja added a comment - - edited

          Hello James,

          Thanks for finding this issue and resolving it.
          Can you please provide steps to reproduce this issue, as this issue should not even exist at first place.

          Show
          Rajesh Taneja added a comment - - edited Hello James, Thanks for finding this issue and resolving it. Can you please provide steps to reproduce this issue, as this issue should not even exist at first place.
          Hide
          Rajesh Taneja added a comment - - edited

          This case will never happen in moodle core, as script gets generated for valid element's.
          Providing this patch as "sanity check", to make sure we don't miss this case

          Show
          Rajesh Taneja added a comment - - edited This case will never happen in moodle core, as script gets generated for valid element's. Providing this patch as "sanity check", to make sure we don't miss this case
          Hide
          Andrew Davis added a comment - - edited

          The testing instructions are not currently adequate. Please provide instructions on how to get to a specific form that you know contains a required field. Just pick one The point of testing isn't to reproduce the bug. It's to check you didn't break anything while fixing the bug.

          Explain what should happen when you omit the required field Vs when you supply it. Example testing instructions are below

          1) go to form X by going to course administration and clicking Y
          2) the field 'blah' should be marked as required. Don't enter it. click submit.
          3) check you get an error saying that "blah is required".
          4) enter blah. click submit.
          5) check form is accepted.

          This is just a hastily thought out example and may not be quite right for this issue but basically if you have modified form validation write out test steps that show that bad data is still rejected and that good data will be accepted.

          Show
          Andrew Davis added a comment - - edited The testing instructions are not currently adequate. Please provide instructions on how to get to a specific form that you know contains a required field. Just pick one The point of testing isn't to reproduce the bug. It's to check you didn't break anything while fixing the bug. Explain what should happen when you omit the required field Vs when you supply it. Example testing instructions are below 1) go to form X by going to course administration and clicking Y 2) the field 'blah' should be marked as required. Don't enter it. click submit. 3) check you get an error saying that "blah is required". 4) enter blah. click submit. 5) check form is accepted. This is just a hastily thought out example and may not be quite right for this issue but basically if you have modified form validation write out test steps that show that bad data is still rejected and that good data will be accepted.
          Hide
          Rajesh Taneja added a comment -

          Thanks Andrew
          I have updated the test instructions, as suggested by you.

          Show
          Rajesh Taneja added a comment - Thanks Andrew I have updated the test instructions, as suggested by you.
          Hide
          Rossiani Wijaya added a comment -

          Hi Raj,

          The patch is working great, except for validating textarea. Empty field in required textarea doesn't trigger any message to display.

          Another tiny issue is the spacing for the code.

          if ( undefined != element.name )
          

          And while you are there, you probably could also fix the tabbing spaces for that section (probably its a good idea to ask Sam for spacing issue).

          Regarding textarea issue, based on our conversation locally, there is no quick fix to address the issue. Therefore I created a separate issue to fix the textarea validation. Please refer to MDL-28674.

          Show
          Rossiani Wijaya added a comment - Hi Raj, The patch is working great, except for validating textarea. Empty field in required textarea doesn't trigger any message to display. Another tiny issue is the spacing for the code. if ( undefined != element.name ) And while you are there, you probably could also fix the tabbing spaces for that section (probably its a good idea to ask Sam for spacing issue). Regarding textarea issue, based on our conversation locally, there is no quick fix to address the issue. Therefore I created a separate issue to fix the textarea validation. Please refer to MDL-28674 .
          Hide
          Rajesh Taneja added a comment -

          Thanks for pointing that Rossie
          I just had a word with Sam about tabbing this section and at this point in time it's not important.

          I have modified the code to return true for undefined elements. As, in case of undefined elements JS should just let form to be submitted and let server handle validation.
          Checks will avoid browser to throw debug errors for undefined elements.

          Show
          Rajesh Taneja added a comment - Thanks for pointing that Rossie I just had a word with Sam about tabbing this section and at this point in time it's not important. I have modified the code to return true for undefined elements. As, in case of undefined elements JS should just let form to be submitted and let server handle validation. Checks will avoid browser to throw debug errors for undefined elements.
          Hide
          Rossiani Wijaya added a comment -

          Thanks for fixing this issue Raj.

          patches work great.

          Show
          Rossiani Wijaya added a comment - Thanks for fixing this issue Raj. patches work great.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated thanks!

          Please, test this into stable branch.

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated thanks! Please, test this into stable branch.
          Hide
          Michael de Raadt added a comment -

          Works nicely. I was able to rename some form elements using Firebug the JS validation skipped them. On the lesson page editing form it passed problems through to the server also.

          Show
          Michael de Raadt added a comment - Works nicely. I was able to rename some form elements using Firebug the JS validation skipped them. On the lesson page editing form it passed problems through to the server also.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Such an effort has get its prize. Universal Award to the very-best Moodle developer in the family. Thanks!

          Closing. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Such an effort has get its prize. Universal Award to the very-best Moodle developer in the family. Thanks! Closing. Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: