Moodle
  1. Moodle
  2. MDL-28483

adding URL resource without an url leads to error

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0.3, 2.1, 2.2
    • Fix Version/s: 2.0.6, 2.1.3
    • Component/s: Resource
    • Labels:
    • Testing Instructions:
      Hide

      This needs to be tested in all three branches.
      In Stables
      1. Add a URL resource to a page
      2. Make sure URL field is required.
      3. Try entering invalid urls (for example:- hxxp://something) and make sure that generates an error and nothing is saved.
      4. Enter a valid url and Save and return to course
      5. Click on this url resource
      6. Make sure everything works as expected.

      In Master
      1. This patch contains module upgrade code, so make sure the upgrade goes smoothly.
      2. Make sure the field "externalurl" in table "url" is "not-null" after the upgrade.
      3. Repeat 1-6 from above

      Show
      This needs to be tested in all three branches. In Stables 1. Add a URL resource to a page 2. Make sure URL field is required. 3. Try entering invalid urls (for example:- hxxp://something) and make sure that generates an error and nothing is saved. 4. Enter a valid url and Save and return to course 5. Click on this url resource 6. Make sure everything works as expected. In Master 1. This patch contains module upgrade code, so make sure the upgrade goes smoothly. 2. Make sure the field "externalurl" in table "url" is "not-null" after the upgrade. 3. Repeat 1-6 from above
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull Master Branch:
      MDL-28483-master

      Description

      Try adding URL resource into a course (see Testing instructions).

      In 2.0+ the field "External URL" is not required. If user does not enter the value there and tries to click the link, an error occurs with the text "A required parameter (id) was missing", which is very confusing.

      In 1.9 the field "Location" is required and pre-populated with "http://" by default (which does not result in an error if user forgets to fill it). In this case if user clicks on the link from a course page, nothing happens at all.

      IMHO, the link should be required in 2.0+ (and the description field not) and value "http://" should not pass validation in 1.9. But probably there was a good reason why link is no longer required in 2.0+.

      Another thing I have noticed is that help for "Display" lists more options than there are in the dropdown.

        Gliffy Diagrams

        1. MDL-28483_git_diff1.jpg
          284 kB
        2. MDL-28483_git_diff2.jpg
          316 kB
        3. MDL-28483_git_diff3.jpg
          391 kB
        4. Setting up a URL.png
          114 kB
        5. URL error.png
          95 kB

          Issue Links

            Activity

            Hide
            Martin Dougiamas added a comment -

            You are spot on, Marina. The field should be required in 2.0+ (the description field should not be).

            Show
            Martin Dougiamas added a comment - You are spot on, Marina. The field should be required in 2.0+ (the description field should not be).
            Hide
            Adrian Kirk-Burnnand added a comment -

            Attaching screenshots of diffs to illustrate code fixes required.

            • MDL-28483_git_diff1.jpg - validation changes
            • MDL-28483_git_diff2.jpg - default setting changes
            • MDL-28483_git_diff1.jpg - optional database definition change, may require other changes to upgrade scripts.
            Show
            Adrian Kirk-Burnnand added a comment - Attaching screenshots of diffs to illustrate code fixes required. MDL-28483 _git_diff1.jpg - validation changes MDL-28483 _git_diff2.jpg - default setting changes MDL-28483 _git_diff1.jpg - optional database definition change, may require other changes to upgrade scripts.
            Hide
            Ankit Agarwal added a comment -

            Description field can be made optional from site administration>plugins>activity modules>URL in 2.0+.
            Obviously Link should be required filed and should go through proper validation.

            Show
            Ankit Agarwal added a comment - Description field can be made optional from site administration>plugins>activity modules>URL in 2.0+. Obviously Link should be required filed and should go through proper validation.
            Hide
            Ankit Agarwal added a comment -

            It seems like "http://" is considered as a valid url by PARAM_URL.
            Created MDL-29489 to deal with this issue.

            Show
            Ankit Agarwal added a comment - It seems like "http://" is considered as a valid url by PARAM_URL. Created MDL-29489 to deal with this issue.
            Hide
            Sam Hemelryk added a comment -

            Hi Ankit,

            I've just been looking at this.
            Two things I've spotted:

            1. If we are going to be upgrading the url.externalurl field we need to have some plan for fields that already have an externalurl = null. Trying to upgrade the table if it contains nulls will result in errors.
            2. In regards to the form validation there is another solution to overriding the validation function. By adding $mform->setType('external url', PARAM_URL) the field will be validated against that param type.

            Other than that it looks good - don't forget to create branches for the Stable versions as well seeing as this is a bug.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Ankit, I've just been looking at this. Two things I've spotted: If we are going to be upgrading the url.externalurl field we need to have some plan for fields that already have an externalurl = null. Trying to upgrade the table if it contains nulls will result in errors. In regards to the form validation there is another solution to overriding the validation function. By adding $mform->setType('external url', PARAM_URL) the field will be validated against that param type. Other than that it looks good - don't forget to create branches for the Stable versions as well seeing as this is a bug. Cheers Sam
            Hide
            Ankit Agarwal added a comment -

            Hi Sam,
            Thanks for review.
            1-> Agree on that, made changes accordingly.
            2-> setType doesn't properly handle the issue and its better to give user a relevant information about the error.

            Currently pushing the Db changes to master only and validation related changes to all branches.
            Thanks

            Show
            Ankit Agarwal added a comment - Hi Sam, Thanks for review. 1-> Agree on that, made changes accordingly. 2-> setType doesn't properly handle the issue and its better to give user a relevant information about the error. Currently pushing the Db changes to master only and validation related changes to all branches. Thanks
            Hide
            Sam Hemelryk added a comment -

            Hi Ankit,

            The 20 and 21 branches look spot on, the master branch needs a little bit more work at present however.

            The upgrade code you have for url.externalurl could be improved yet by executing straight update code.

            $DB->execute("UPDATE {url} SET externalurl = '' WHERE externalurl IS NULL");
            

            This sort of SQL is perfectly fine within upgrade code - it saves us having to iterate potentially thousands of records which is important as upgrades can be a real performance drain as it is. In the this case I don't think there would be any let alone many but still best practice.

            The other thing that crosses my mind is that while this prevents the problem from happening in future I assume existing empty externalurl fields are still going to lead to the error page correct? In which case really we should look for a solution to that as well. I did note while playing with this that if I create a URL resource without an external URL and click Save and Display I get a valid page (although no URL of course) so I think it would be possible and that likely there is some mechanism going wrong deeper within section printing. Anyway something to investigate, you can always create a new bug for that if need be.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Ankit, The 20 and 21 branches look spot on, the master branch needs a little bit more work at present however. The upgrade code you have for url.externalurl could be improved yet by executing straight update code. $DB->execute("UPDATE {url} SET externalurl = '' WHERE externalurl IS NULL"); This sort of SQL is perfectly fine within upgrade code - it saves us having to iterate potentially thousands of records which is important as upgrades can be a real performance drain as it is. In the this case I don't think there would be any let alone many but still best practice. The other thing that crosses my mind is that while this prevents the problem from happening in future I assume existing empty externalurl fields are still going to lead to the error page correct? In which case really we should look for a solution to that as well. I did note while playing with this that if I create a URL resource without an external URL and click Save and Display I get a valid page (although no URL of course) so I think it would be possible and that likely there is some mechanism going wrong deeper within section printing. Anyway something to investigate, you can always create a new bug for that if need be. Cheers Sam
            Hide
            Ankit Agarwal added a comment -

            Hi Sam,
            Making changes.
            On another note since Oracle considers '' as null, wont it create any issues?

            As per the second part. I will create an issue to track with existing URL resources with invalid/empty locations fields.It might be just best to re-filter output data before printing and generate an error if needed.

            Thanks

            Show
            Ankit Agarwal added a comment - Hi Sam, Making changes. On another note since Oracle considers '' as null, wont it create any issues? As per the second part. I will create an issue to track with existing URL resources with invalid/empty locations fields.It might be just best to re-filter output data before printing and generate an error if needed. Thanks
            Hide
            Sam Hemelryk added a comment -

            Hi Ankik - yes sorry you shouldn't use '' but $DB->sql_empty instead.
            Rest sounds good thanks

            Show
            Sam Hemelryk added a comment - Hi Ankik - yes sorry you shouldn't use '' but $DB->sql_empty instead. Rest sounds good thanks
            Hide
            Ankit Agarwal added a comment -

            Thanks sam, up for integration now

            Created MDL-29619 to deal with existing empty/invalid urls in database.

            Show
            Ankit Agarwal added a comment - Thanks sam, up for integration now Created MDL-29619 to deal with existing empty/invalid urls in database.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            anybody hear about $DB->set_field() and family ? LOL

            Note I still have to look to this to see why that transformation is needed...

            Show
            Eloy Lafuente (stronk7) added a comment - anybody hear about $DB->set_field() and family ? LOL Note I still have to look to this to see why that transformation is needed...
            Hide
            Ankit Agarwal added a comment -

            Hi Eloy,
            Want me to change the master code to use set_field?

            Show
            Ankit Agarwal added a comment - Hi Eloy, Want me to change the master code to use set_field?
            Hide
            Eloy Lafuente (stronk7) added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            Sam Hemelryk added a comment -

            Hi Ankit,

            Just had a quick look at this - I think Eloy is asking for execute to be changed to set field which is fair enough Hehe should've spotted that sorry.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Ankit, Just had a quick look at this - I think Eloy is asking for execute to be changed to set field which is fair enough Hehe should've spotted that sorry. Cheers Sam
            Hide
            Ankit Agarwal added a comment -

            Hi,
            Made changes!
            Thanks

            Show
            Ankit Agarwal added a comment - Hi, Made changes! Thanks
            Hide
            Aparup Banerjee added a comment -

            stopping review, i think Eloy wanted to have a look at this from his 'why that transformation is needed...' comment above.

            I did have a look though and it seems ok, just wasn't comfy with validation and display(MDL-29619) being separately handled for stable.

            Setting Eloy as The Integrator!

            Show
            Aparup Banerjee added a comment - stopping review, i think Eloy wanted to have a look at this from his 'why that transformation is needed...' comment above. I did have a look though and it seems ok, just wasn't comfy with validation and display( MDL-29619 ) being separately handled for stable. Setting Eloy as The Integrator!
            Hide
            Eloy Lafuente (stronk7) added a comment - - edited

            LOL, ETI, so near to ETE :-D
            (ete = common-colloquial diminutive in Spanish for near everything)

            Show
            Eloy Lafuente (stronk7) added a comment - - edited LOL, ETI, so near to ETE :-D (ete = common-colloquial diminutive in Spanish for near everything)
            Hide
            Eloy Lafuente (stronk7) added a comment - - edited

            Oh,

            sorry I think this really cannot be integrated in its current incarnation:

            1) Unless there is some bug, all the dml functions do NOT need $DB->sql_empty() at all when used in params. Only when hardcoding-embeding '' into one SQL fragment it is needed. Drivers handle placeholders ok automatically afaik.

            2) Worse, when you implement the validate() method on any mod_form, the first thing it must call is

            $errors = parent::validation($data, $files);
            

            or we will missing a lot of validations there.

            3) Also, I can see why formally we are modifying the DB column to NOT NULL, so ok, but it gives us 0 advantage over keeping it nullable and with nulls. Note this is just one reflexion about the bloody empties we have been abusing along the years, grrr. They mean nothing in a big % of cases (and they could be replaced by nulls in a lot of them). (feel free to forget this comment, hehe)

            Show
            Eloy Lafuente (stronk7) added a comment - - edited Oh, sorry I think this really cannot be integrated in its current incarnation: 1) Unless there is some bug, all the dml functions do NOT need $DB->sql_empty() at all when used in params. Only when hardcoding-embeding '' into one SQL fragment it is needed. Drivers handle placeholders ok automatically afaik. 2) Worse, when you implement the validate() method on any mod_form, the first thing it must call is $errors = parent::validation($data, $files); or we will missing a lot of validations there. 3) Also, I can see why formally we are modifying the DB column to NOT NULL, so ok, but it gives us 0 advantage over keeping it nullable and with nulls. Note this is just one reflexion about the bloody empties we have been abusing along the years, grrr. They mean nothing in a big % of cases (and they could be replaced by nulls in a lot of them). (feel free to forget this comment, hehe)
            Hide
            Ankit Agarwal added a comment -

            Hi Eloy (again! ),

            1-> So we can use either in this case or its moodle standard to use Sql_empty() when hardcoding SQL and '' , when passing as argument to DML functions?

            2-> Docs should really reflect this ( http://docs.moodle.org/dev/lib/formslib.php_Validation)

            Making changes
            Thanks

            Show
            Ankit Agarwal added a comment - Hi Eloy (again! ), 1-> So we can use either in this case or its moodle standard to use Sql_empty() when hardcoding SQL and '' , when passing as argument to DML functions? 2-> Docs should really reflect this ( http://docs.moodle.org/dev/lib/formslib.php_Validation ) Making changes Thanks
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Hi Ankit,

            finally I've left this out from this integration... I couldn't avoid think and think about why the externalurl column was defined as nullable and lately I started thinking that, perhaps, it was caused by something in the migration from 1.9 resources to 2.x url module.

            So, just to be in the safe side, this has been delayed. Could you take a look / ask Petr about any reason for having that column nullable? I believe we need his opinion before pushing it upstream.

            Thanks and ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Hi Ankit, finally I've left this out from this integration... I couldn't avoid think and think about why the externalurl column was defined as nullable and lately I started thinking that, perhaps, it was caused by something in the migration from 1.9 resources to 2.x url module. So, just to be in the safe side, this has been delayed. Could you take a look / ask Petr about any reason for having that column nullable? I believe we need his opinion before pushing it upstream. Thanks and ciao
            Hide
            Eloy Lafuente (stronk7) added a comment - - edited

            About your Qs 2 comments above:

            1) Yes, both will work but it's preferable to avoid the helper if not necessary. Some day all current uses should be reviewed and, cleaned if necessary, so don't worry. (See MDL-29765)

            2) I've updated http://docs.moodle.org/dev/lib/formslib.php_Validation, feel free to amend it.

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - - edited About your Qs 2 comments above: 1) Yes, both will work but it's preferable to avoid the helper if not necessary. Some day all current uses should be reviewed and, cleaned if necessary, so don't worry. (See MDL-29765 ) 2) I've updated http://docs.moodle.org/dev/lib/formslib.php_Validation , feel free to amend it. Ciao
            Hide
            Eloy Lafuente (stronk7) added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Just commented with Petr about this and he thinks it has sense to change the column to not null.

            So I'll take a quick look to upgrade to see if there is something obvious that can lead to problems and done for next week.

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Just commented with Petr about this and he thinks it has sense to change the column to not null. So I'll take a quick look to upgrade to see if there is something obvious that can lead to problems and done for next week. Ciao
            Hide
            Eloy Lafuente (stronk7) added a comment -

            After looking to upgrade.php I did not find anything preventing this to land, so integrated, thanks!

            Two tiny comments:

            1) in comments, after the // it's better to add one space.
            2) use the xmldb editor for editing install.xml files, plz

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - After looking to upgrade.php I did not find anything preventing this to land, so integrated, thanks! Two tiny comments: 1) in comments, after the // it's better to add one space. 2) use the xmldb editor for editing install.xml files, plz Ciao
            Hide
            Ankit Agarwal added a comment -

            Will keep in mind those two points

            Show
            Ankit Agarwal added a comment - Will keep in mind those two points
            Hide
            Sam Hemelryk added a comment -

            Tested + passed = Thanks

            Show
            Sam Hemelryk added a comment - Tested + passed = Thanks
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Many thanks for all the hard work. This is now part of Moodle, your favorite LMS.

            Closing as fixed, ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Many thanks for all the hard work. This is now part of Moodle, your favorite LMS. Closing as fixed, ciao
            Hide
            Petr Skoda added a comment -

            Oh, the $data['externalurl'] = clean_param($data['externalurl'], PARAM_URL); is wrong because it accepts only http URLs is a perfect format, but we also supported urls like teamspeak: . I am going to file a regression issue and revert it.

            Show
            Petr Skoda added a comment - Oh, the $data ['externalurl'] = clean_param($data ['externalurl'] , PARAM_URL); is wrong because it accepts only http URLs is a perfect format, but we also supported urls like teamspeak: . I am going to file a regression issue and revert it.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            crap, does it? grrr...

            Show
            Eloy Lafuente (stronk7) added a comment - crap, does it? grrr...
            Hide
            Ankit Agarwal added a comment -

            Thanks for spotting and fixing it

            Show
            Ankit Agarwal added a comment - Thanks for spotting and fixing it

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: