Details

    • Type: Bug
    • Status: Closed
    • Priority: 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

          Attachments

            Activity

            Hide
            marina 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 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_frenz Ankit Agarwal added a comment -

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

            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Looks good Marina, feel free to push forward after back porting and adding testing instructions. Thanks
            Hide
            marina 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 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
            poltawski 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
            poltawski 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 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 Marina Glancy added a comment - Thanks for comment Dan. Requesting peer review. I made param type PARAM_RAW_TRIMMED and simplified the validation.
            Hide
            poltawski Dan Poltawski added a comment -

            +1

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

            looks ok, +1

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

            Integrated (25 & master), thanks!

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

            Passing, thanks.

            Show
            fred Frédéric Massart added a comment - Passing, thanks.
            Hide
            poltawski 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
            poltawski 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:
                  Fix Release Date:
                  11/Nov/13