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

moodle notices while browsing contents of a course on the moodle app (get_course_contents)

          Details

          • Type: Bug
          • Status: Closed
          • Priority: Minor
          • Resolution: Fixed
          • Affects Version/s: 2.3, 2.3.6, 2.4.3, 2.5
          • Fix Version/s: 2.3.7, 2.4.4
          • Component/s: Course, Web Services
          • Labels:
          • Testing Instructions:
            Hide

            For course containing a empty section, call core_course_get_contents. No warnings should be displayed. (mandatory for 2.3)

            Alternative possible test (for 2.4 and 2.5 only):
            If you want don't write a client you can use the Moodle Mobile app. Enable mobile on moodle, download moodle mobile app, use it to go to see course content, it does a core_course_get_course_contents call. Check there is no warning in your log (the one mentioned in the issue description)

            Show
            For course containing a empty section, call core_course_get_contents. No warnings should be displayed. (mandatory for 2.3) Alternative possible test (for 2.4 and 2.5 only): If you want don't write a client you can use the Moodle Mobile app. Enable mobile on moodle, download moodle mobile app, use it to go to see course content, it does a core_course_get_course_contents call. Check there is no warning in your log (the one mentioned in the issue description)
          • Affected Branches:
            MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
          • Fixed Branches:
            MOODLE_23_STABLE, MOODLE_24_STABLE
          • Pull from Repository:
          • Pull Master Branch:
            MDL-30775-master
          • Pull Master Diff URL:

            Description

            [Fri Dec 16 14:56:52 2011] [error] [client 192.168.100.131] PHP Notice: Undefined index: 3 in /home/aparup/mcode/integration/course/externallib.php on line 118
            [Fri Dec 16 14:56:52 2011] [error] [client 192.168.100.131] PHP Warning: Invalid argument supplied for foreach() in /home/aparup/mcode/integration/course/externallib.php on line 118

              Gliffy Diagrams

                Attachments

                  Activity

                  Ascending order - Click to sort in descending order
                  Hide
                  Permalink
                  ankit_frenz Ankit Agarwal added a comment - - edited

                  Hi Jerome,
                  The patch looks good in general. Here are a few couple of things you might want to consider:-

                  1. there is no point in calling $modinfosections = $modinfo->get_sections(); repeatedly, it should be moved out of foreach loop.
                  2. It might be a good idea to not to escape the activity url. (get_url->out(false)).

                  Thanks

                  Show
                  ankit_frenz Ankit Agarwal added a comment - - edited Hi Jerome, The patch looks good in general. Here are a few couple of things you might want to consider:- there is no point in calling $modinfosections = $modinfo->get_sections(); repeatedly, it should be moved out of foreach loop. It might be a good idea to not to escape the activity url. (get_url->out(false)). Thanks
                  Hide
                  Permalink
                  jerome Jérôme Mouneyrac added a comment - - edited

                  I've done the changes requested by Ankit. Note that only 2.5 got the url escaping change.
                  This change deserve to be indicated in API change Moodledocs as it could break some clients (even though I don't think it does, at least it doesn't break the official client).

                  If this url escaping change is accepted/integrated then I'll add the API doc change.

                  Tested on 2.5 (by the app) and 2.3 (with the REST sample client).

                  Thank you.

                  Show
                  jerome Jérôme Mouneyrac added a comment - - edited I've done the changes requested by Ankit. Note that only 2.5 got the url escaping change. This change deserve to be indicated in API change Moodledocs as it could break some clients (even though I don't think it does, at least it doesn't break the official client). If this url escaping change is accepted/integrated then I'll add the API doc change. Tested on 2.5 (by the app) and 2.3 (with the REST sample client). Thank you.
                  Hide
                  Permalink
                  jerome Jérôme Mouneyrac added a comment -

                  PS: thanks for reviewing Ankit.

                  Show
                  jerome Jérôme Mouneyrac added a comment - PS: thanks for reviewing Ankit.
                  Hide
                  Permalink
                  poltawski Dan Poltawski 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
                  poltawski Dan Poltawski 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
                  Permalink
                  damyon Damyon Wiese added a comment -

                  Note - posting diff url (24) ignoring whitespace because it's much clearer:

                  https://github.com/mouneyrac/moodle/compare/moodle:MOODLE_24_STABLE...MDL-30775-24?w=1

                  This patch could have only been 4 lines if you used continue - I am not a fan of deeply nested if statements.

                  Show
                  damyon Damyon Wiese added a comment - Note - posting diff url (24) ignoring whitespace because it's much clearer: https://github.com/mouneyrac/moodle/compare/moodle:MOODLE_24_STABLE...MDL-30775-24?w=1 This patch could have only been 4 lines if you used continue - I am not a fan of deeply nested if statements.
                  Hide
                  Permalink
                  damyon Damyon Wiese added a comment -

                  Hi Jerome,

                  Can you confirm that you meant to change the escaping for the icon_url in the stable branches? (The above diff url shows it clearly).

                  Show
                  damyon Damyon Wiese added a comment - Hi Jerome, Can you confirm that you meant to change the escaping for the icon_url in the stable branches? (The above diff url shows it clearly).
                  Hide
                  Permalink
                  jerome Jérôme Mouneyrac added a comment - - edited

                  Ah no it's a mistake! Thanks Damyon.

                  Show
                  jerome Jérôme Mouneyrac added a comment - - edited Ah no it's a mistake! Thanks Damyon.
                  Hide
                  Permalink
                  jerome Jérôme Mouneyrac added a comment -

                  Thanks for the whitespace tip, I'll use it from now.

                  Show
                  jerome Jérôme Mouneyrac added a comment - Thanks for the whitespace tip, I'll use it from now.
                  Hide
                  Permalink
                  jerome Jérôme Mouneyrac added a comment -

                  Ok I made the change except the continue that I don't agree (I find it more confusing to have continue everywhere)

                  Show
                  jerome Jérôme Mouneyrac added a comment - Ok I made the change except the continue that I don't agree (I find it more confusing to have continue everywhere)
                  Hide
                  Permalink
                  damyon Damyon Wiese added a comment -

                  Thanks Jerome,

                  This has been integrated to 23, 24 and master.

                  Show
                  damyon Damyon Wiese added a comment - Thanks Jerome, This has been integrated to 23, 24 and master.
                  Hide
                  Permalink
                  poltawski Dan Poltawski added a comment -

                  testing instructions had wrong function name (assume it was actually core_course_get_contents)

                  Show
                  poltawski Dan Poltawski added a comment - testing instructions had wrong function name (assume it was actually core_course_get_contents)
                  Hide
                  Permalink
                  poltawski Dan Poltawski added a comment -

                  Tested the websivce in 23, 24 and master.

                  Tested the mobile app in master only, looks good.

                  Show
                  poltawski Dan Poltawski added a comment - Tested the websivce in 23, 24 and master. Tested the mobile app in master only, looks good.
                  Hide
                  Permalink
                  poltawski Dan Poltawski added a comment -

                  Thanks! You're changes are now spread to the world through this git and our source control repositories.

                  No time to rest though, we've got days to make 2.5 the best yet!

                  ciao

                  Show
                  poltawski Dan Poltawski added a comment - Thanks! You're changes are now spread to the world through this git and our source control repositories. No time to rest though, we've got days to make 2.5 the best yet! ciao

                    People

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

                      Dates

                      • Created:
                        Updated:
                        Resolved:
                        Fix Release Date:
                        13/May/13