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
    • Rank:
      18141

      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.

      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 Škoda 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 Škoda 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: