Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-69758

Web sevices: core_get_course_contents errors with sketchy URLs

    XMLWordPrintable

    Details

    • Testing Instructions:
      Hide
      • The easiest way to call this service is using the Moodle mobile app. So, ensure your website is set up to allow the app.
      • Choose a test user who will be able to use the app.

      BEFORE patch:

      1. Logged in as admin, go to a course that your test user is enrolled in, or enrol them in a new course if required.
      2. Add a new URL to the course with the following (probably valid, and definitely working, but stupid) URL: http://www.open.ac.uk/libraryservices//////beingdigital/
      3. In the Moodle app, log in as the test user and go to the course. Pull down from the top of the screen to force it to reload the course (just in case you already had it cached).
      4. Note: At time of writing, the URL still does not work if you click on it from the app. This is expected.
      5. Look at the web server error log (with Apache web server this is called error.log by default and is in the 'logs' directory, but it depends on configuration).
      6. You should see error from the time of your Moodle app request with text similar to the following:
        • PHP Warning: count(): Parameter must be an array or an object that implements Countable in [FILESYSTEM PATH] externallib.php
        • PHP Warning: Invalid argument supplied for foreach() in [FILESYSTEM PATH]/externallib.php on line 329

      AFTER patch:

      1. Repeat steps 1-5
      2. Look at the web server error log (with Apache web server this is called error.log by default and is in the 'logs' directory, but it depends on configuration).
      3. You should NOT see the error that was being shown before the patch.
      Show
      The easiest way to call this service is using the Moodle mobile app. So, ensure your website is set up to allow the app. Choose a test user who will be able to use the app. BEFORE patch: Logged in as admin, go to a course that your test user is enrolled in, or enrol them in a new course if required. Add a new URL to the course with the following (probably valid, and definitely working, but stupid) URL: http://www.open.ac.uk/libraryservices//////beingdigital/ In the Moodle app, log in as the test user and go to the course. Pull down from the top of the screen to force it to reload the course (just in case you already had it cached). Note: At time of writing, the URL still does not work if you click on it from the app. This is expected. Look at the web server error log (with Apache web server this is called error.log by default and is in the 'logs' directory, but it depends on configuration). You should see error from the time of your Moodle app request with text similar to the following: PHP Warning: count(): Parameter must be an array or an object that implements Countable in [FILESYSTEM PATH] externallib.php PHP Warning: Invalid argument supplied for foreach() in [FILESYSTEM PATH] /externallib.php on line 329 AFTER patch: Repeat steps 1-5 Look at the web server error log (with Apache web server this is called error.log by default and is in the 'logs' directory, but it depends on configuration). You should NOT see the error that was being shown before the patch.
    • Affected Branches:
      MOODLE_310_STABLE, MOODLE_38_STABLE, MOODLE_39_STABLE, MOODLE_400_STABLE
    • Fixed Branches:
      MOODLE_38_STABLE, MOODLE_39_STABLE
    • Pull 3.9 Branch:
      MDL-69758-m39
    • Pull 3.10 Branch:
      MDL-69758-m310
    • Pull Master Branch:
      MDL-69758-master

      Description

      We saw errors like this in the server logs:

      PHP Warning: count(): Parameter must be an array or an object that implements Countable in /srv/learn2.open.ac.uk/www/html/course/externallib.php on line 324

      This is after the following code:

                                  $getcontentfunction = $cm->modname.'_export_contents';
                                  if (function_exists($getcontentfunction)) {
                                      $contents = $getcontentfunction($cm, $baseurl);
      

      The _export_contents function is expected to return an array. If it does not, you get these warnings.

      The cause turned out to be in url_export_contents:

          $isurl = clean_param($fullurl, PARAM_URL);
          if (empty($isurl)) {
              return null;
          }
      

      And this was happening because the URL was set to:

      http://www.open.ac.uk/libraryservices//beingdigital/

      Note the double slash - sketchy, but it works. (The URL also works with only a single slash there.)

      From the mobile app (where this webservice is called), clicking on the link doesn't work - it does nothing.

      If you change the URL to a single slash then it doesn't cause this error.

      There is clearly (imo) a bug in url_export_contents - instead of returning null, it should return an empty array if there's an error. I looked at the other _export_contents functions and didn't find any other examples of the same problem.

      However, is it correct that clean_param returns empty in this case? I had a quick look at the URI RFC and don't think it forbids empty path segments... I'm going to leave that for a separate issue if we need to fix it... Likewise, I wonder if mod_url should validate the URL against the same function on the settings form? Maybe hard because of placeholders or something?

        Attachments

          Activity

            People

            Assignee:
            quen Sam Marshall
            Reporter:
            quen Sam Marshall
            Peer reviewer:
            Tim Hunt Tim Hunt
            Integrator:
            Eloy Lafuente (stronk7) Eloy Lafuente (stronk7)
            Tester:
            Anna Carissa Sadia Anna Carissa Sadia
            Participants:
            Component watchers:
            Amaia Anabitarte, Carlos Escobedo, Ferran Recio, Ilya Tregubov, Sara Arjona (@sarjona), Juan Leyva, Amaia Anabitarte, Carlos Escobedo, Ferran Recio, Ilya Tregubov, Sara Arjona (@sarjona)
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:
              Fix Release Date:
              9/Nov/20

                Time Tracking

                Estimated:
                Original Estimate - Not Specified
                Not Specified
                Remaining:
                Remaining Estimate - 0 minutes
                0m
                Logged:
                Time Spent - 2 hours, 5 minutes
                2h 5m