Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.5, 2.6
    • Fix Version/s: 2.5.3
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide

      Create/edit URL resource and try to enter URLs that are valid or not valid, with or without extra spaces in the beginning/end.
      Try to leave field empty or containing just spaces.

      Make sure that empty (or ' ') URL is not accepted, extra spaces are trimmed.

      Please note that URL validation is very light here.

      Show
      Create/edit URL resource and try to enter URLs that are valid or not valid, with or without extra spaces in the beginning/end. Try to leave field empty or containing just spaces. Make sure that empty (or ' ') URL is not accepted, extra spaces are trimmed. Please note that URL validation is very light here.
    • Workaround:
      Hide

      The cause of the problem seems to be, that in mod_url_mod_form (/mod/url/mod_form.php line 54) the 'externalurl' field type is PARAM_URL and when inserting URL with space in end, then it will not be valid URL and the value gets empty.

      When changing the described line as the following:

      -        $mform->setType('externalurl', PARAM_URL);
      +        $mform->setType('externalurl', PARAM_TEXT);
      

      then the form starts to work as expected.
      The URL gets validated anyway in function "validation" in the same file.

      Show
      The cause of the problem seems to be, that in mod_url_mod_form (/mod/url/mod_form.php line 54) the 'externalurl' field type is PARAM_URL and when inserting URL with space in end, then it will not be valid URL and the value gets empty. When changing the described line as the following: - $mform->setType('externalurl', PARAM_URL); + $mform->setType('externalurl', PARAM_TEXT); then the form starts to work as expected. The URL gets validated anyway in function "validation" in the same file.
    • Affected Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      wip-MDL-41921-master

      Description

      1. Add URL resource to course
      2. Inserting URL that needs trimming to "External URL" field (for example "http://example.com ")
      3. Click "Save and display" or "Save and return..."

      Expected result: trim the "External URL" value and save the resource.

      Actual result: resource not saved, "External URL" field empty and with error "Required".

        Gliffy Diagrams

          Activity

          Hide
          Marina Glancy added a comment -

          I suggest this approach, this will fix all forms (for example "Webpage" in user edit form) but preserve validation that would be lost in case of substituting with PARAM_TEXT.

          Submitting for peer review

          Show
          Marina Glancy added a comment - I suggest this approach, this will fix all forms (for example "Webpage" in user edit form) but preserve validation that would be lost in case of substituting with PARAM_TEXT. Submitting for peer review
          Hide
          Ankit Agarwal added a comment -

          Looks good Marina, feel free to push forward after back porting and adding testing instructions.

          Thanks

          Show
          Ankit Agarwal added a comment - Looks good Marina, feel free to push forward after back porting and adding testing instructions. Thanks
          Hide
          Marina Glancy added a comment -

          Hm, looking closer at the issue it feels like regression from MDL-34311
          https://github.com/moodle/moodle/commit/b716cd2b702da070003cd19c808eff3309e32c6f

          one-line change made lots of url-validation code redundant.

          There are two options for solutions here:

          1. accept code submitted by Mart in "workaround".
          2. remove all mod_url url validation and replace it with clean_param(..., PARAM_URL) - see link to my branch above

          Asking second opinion from Dan Poltawski (author of MDL-34311) and Petr Skoda (author of mod_url code)

          PS: BTW those all fail being valid URLs:

          clean_param('http://правительство.рф/', PARAM_URL);
          clean_param('http://www.example.com/žlutý koníček/lala.txt', PARAM_URL);
          clean_param('ftp://user:password@www.example.com/', PARAM_URL);

          Show
          Marina Glancy added a comment - Hm, looking closer at the issue it feels like regression from MDL-34311 https://github.com/moodle/moodle/commit/b716cd2b702da070003cd19c808eff3309e32c6f one-line change made lots of url-validation code redundant. There are two options for solutions here: 1. accept code submitted by Mart in "workaround". 2. remove all mod_url url validation and replace it with clean_param(..., PARAM_URL) - see link to my branch above Asking second opinion from Dan Poltawski (author of MDL-34311 ) and Petr Skoda (author of mod_url code) PS: BTW those all fail being valid URLs: clean_param('http://правительство.рф/', PARAM_URL); clean_param('http://www.example.com/žlutý koníček/lala.txt', PARAM_URL); clean_param('ftp://user:password@www.example.com/', PARAM_URL);
          Hide
          Dan Poltawski added a comment - - edited

          Doh. I don't think we should rely on the PARAM_URL for 'validation' if its already being validated correctly by the form. +1 to switch it to PARAM_RAW and do the validation in the form validation.

          This is a problem we have other places too (like using PARAM_EMAIL)

          Show
          Dan Poltawski added a comment - - edited Doh. I don't think we should rely on the PARAM_URL for 'validation' if its already being validated correctly by the form. +1 to switch it to PARAM_RAW and do the validation in the form validation. This is a problem we have other places too (like using PARAM_EMAIL)
          Hide
          Marina Glancy added a comment -

          Thanks for comment Dan.
          Requesting peer review.

          I made param type PARAM_RAW_TRIMMED and simplified the validation.

          Show
          Marina Glancy added a comment - Thanks for comment Dan. Requesting peer review. I made param type PARAM_RAW_TRIMMED and simplified the validation.
          Hide
          Dan Poltawski added a comment -

          +1

          Show
          Dan Poltawski added a comment - +1
          Hide
          Petr Skoda added a comment -

          looks ok, +1

          Show
          Petr Skoda added a comment - looks ok, +1
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (25 & master), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (25 & master), thanks!
          Hide
          Frédéric Massart added a comment -

          Passing, thanks.

          Show
          Frédéric Massart added a comment - Passing, thanks.
          Hide
          Dan Poltawski added a comment -

          You did it!

          Thanks for your contribution, this change has been merged upstream and is now available on your local git mirror and on download sites shortly.

          Show
          Dan Poltawski added a comment - You did it! Thanks for your contribution, this change has been merged upstream and is now available on your local git mirror and on download sites shortly.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: