Moodle
  1. Moodle
  2. MDL-39664

mdeploy.php must not fail on its download_file() where $source url is a redirected url.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5
    • Component/s: Administration
    • Labels:
      None
    • Testing Instructions:
      Hide

      0) open page https://moodle.org/plugins/pluginversions.php?plugin=mod_bigbluebuttonbn

      1) download manually and install a plugin from the plugins directory that is of a version that is compatible with yours but that isn't the latest version. (Here i used big blue button but had to set updateminmaturity setting to RC for the next step)

      2) got to plugins overview page and test that an update is available for bigbluebutton (to that lastest RC one, can be seen in the download versions tab for the plugin )

      3) continue to upgrade the plugin, test that <moodledata>/mdeploy/mdeploy.log does not report any errors in fetching the zip and upgrading the plugin.

      your log output should be similar to the following:

      2013-05-14 12:32:38 === MDEPLOY EXECUTION START ===
      2013-05-14 12:32:38 Successfully authorized using the passphrase file
      2013-05-14 12:32:38 Plugin upgrade requested
      2013-05-14 12:32:38 Downloading package https://download.moodle.org/download.php/direct/addons/2911/mod_bigbluebuttonbn_moodle25_2013050100.zip
      2013-05-14 12:32:38 Using custom CA certificate /Users/aparup/Sitesdata/p_data/master/mysql/moodleorgca.crt
      2013-05-14 12:32:42 Package downloaded into /Users/aparup/Sitesdata/p_data/master/mysql/mdeploy/var/4971bee4a727f6c31b348a997b06a13c.0.zip
      2013-05-14 12:32:42 MD5 checksum ok
      2013-05-14 12:32:42 Current plugin code location: /Users/aparup/Sites/p/mod/bigbluebuttonbn
      2013-05-14 12:32:42 Moving the current code into archive: /Users/aparup/Sitesdata/p_data/master/mysql/mdeploy/archive/bigbluebuttonbn_1368505962.0
      2013-05-14 12:32:42 Package successfully extracted
      
      Show
      0) open page https://moodle.org/plugins/pluginversions.php?plugin=mod_bigbluebuttonbn 1) download manually and install a plugin from the plugins directory that is of a version that is compatible with yours but that isn't the latest version. (Here i used big blue button but had to set updateminmaturity setting to RC for the next step) 2) got to plugins overview page and test that an update is available for bigbluebutton (to that lastest RC one, can be seen in the download versions tab for the plugin ) 3) continue to upgrade the plugin, test that <moodledata>/mdeploy/mdeploy.log does not report any errors in fetching the zip and upgrading the plugin. your log output should be similar to the following: 2013-05-14 12:32:38 === MDEPLOY EXECUTION START === 2013-05-14 12:32:38 Successfully authorized using the passphrase file 2013-05-14 12:32:38 Plugin upgrade requested 2013-05-14 12:32:38 Downloading package https: //download.moodle.org/download.php/direct/addons/2911/mod_bigbluebuttonbn_moodle25_2013050100.zip 2013-05-14 12:32:38 Using custom CA certificate /Users/aparup/Sitesdata/p_data/master/mysql/moodleorgca.crt 2013-05-14 12:32:42 Package downloaded into /Users/aparup/Sitesdata/p_data/master/mysql/mdeploy/ var /4971bee4a727f6c31b348a997b06a13c.0.zip 2013-05-14 12:32:42 MD5 checksum ok 2013-05-14 12:32:42 Current plugin code location: /Users/aparup/Sites/p/mod/bigbluebuttonbn 2013-05-14 12:32:42 Moving the current code into archive: /Users/aparup/Sitesdata/p_data/master/mysql/mdeploy/archive/bigbluebuttonbn_1368505962.0 2013-05-14 12:32:42 Package successfully extracted
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
      MDL-39664_m24
    • Pull Master Branch:
    • Rank:
      50387

      Description

      After implementing a download redirect by download.moodle.org to moodle.org/plugins we discovered that the mdeploy.php script broke...

      2013-05-14 11:35:17 === MDEPLOY EXECUTION START ===
      2013-05-14 11:35:17 Successfully authorized using the passphrase file
      2013-05-14 11:35:17 Plugin upgrade requested
      2013-05-14 11:35:17 Downloading package https://download.moodle.org/download.php/direct/addons/2911/mod_bigbluebuttonbn_moodle25_2013050100.zip
      2013-05-14 11:35:17 Using custom CA certificate /Users/aparup/Sitesdata/p_data/master/mysql/moodleorgca.crt
      2013-05-14 11:35:18 Curl remote error.
      2013-05-14 11:35:18 Array
      (
      [url] => https://download.moodle.org/download.php/direct/addons/2911/mod_bigbluebuttonbn_moodle25_2013050100.zip
      [content_type] => text/html; charset=UTF-8
      [http_code] => 302
      [header_size] => 720
      [request_size] => 133
      [filetime] => -1
      [ssl_verify_result] => 0
      [redirect_count] => 0
      [total_time] => 1.288314
      [namelookup_time] => 0.000633
      [connect_time] => 0.061531
      [pretransfer_time] => 0.228487
      [size_upload] => 0
      [size_download] => 0
      [speed_download] => 0
      [speed_upload] => 0
      [download_content_length] => -1
      [upload_content_length] => 0
      [starttransfer_time] => 1.288297
      [redirect_time] => 0
      [certinfo] => Array
      (
      )

      [primary_ip] => 141.101.113.179
      [primary_port] => 443
      [local_ip] => 192.168.100.135
      [local_port] => 64804
      [redirect_url] => https://moodle.org/plugins/download.php/2911/mod_bigbluebuttonbn_moodle25_2013050100.zip
      )
      2013-05-14 11:35:18 cURL error 0
      2013-05-14 11:35:18 Unable to download the file from https://download.moodle.org/download.php/direct/addons/2911/mod_bigbluebuttonbn_moodle25_2013050100.zip into /Users/aparup/Sitesdata/p_data/master/mysql/mdeploy/var/4971bee4a727f6c31b348a997b06a13c.18.zip
      2013-05-14 11:35:18 exception 'download_file_exception' with message 'Unable to download the package' in /Users/aparup/Sites/p/mdeploy.php:761
      Stack trace:
      #0 /Users/aparup/Sites/p/mdeploy.php(1499): worker->execute()
      #1

      {main}

      This is about simply following redirect (without using moodle library, but copying it, as mdeploy is separate)

      ps:As a sub-task or something else, i'm wondering that some alarm bells that should have gone off haven't wrt testing infrastructure.

        Issue Links

          Activity

          Hide
          Aparup Banerjee added a comment - - edited

          added branch.
          still facing some md5 mismatch issue after the download is successful though. fixing.
          edit: doh, need to follow the 302 redirect!

          Show
          Aparup Banerjee added a comment - - edited added branch. still facing some md5 mismatch issue after the download is successful though. fixing. edit: doh, need to follow the 302 redirect!
          Hide
          Aparup Banerjee added a comment -

          works here, pushing up for integration

          Show
          Aparup Banerjee added a comment - works here, pushing up for integration
          Hide
          Damyon Wiese added a comment -

          Hi Aparup,

          Some comments:

          1. I think it would be worth also setting CURLOPT_MAXREDIRECTS to something like 5, to prevent redirect loops.
          2. There is a weird character in the commit message
          3. You shouldn't log a warning if there are no cacerts found - that is normal behaviour on working unix systems.

          Show
          Damyon Wiese added a comment - Hi Aparup, Some comments: 1. I think it would be worth also setting CURLOPT_MAXREDIRECTS to something like 5, to prevent redirect loops. 2. There is a weird character in the commit message 3. You shouldn't log a warning if there are no cacerts found - that is normal behaviour on working unix systems.
          Hide
          Aparup Banerjee added a comment -

          thanks, will fix up 1 & 2.

          3: should i remove it or just rephrase it... it was helpful to know why the system was failing to fetch. perhaps something like 'cacert not found, skipping.' ?

          Show
          Aparup Banerjee added a comment - thanks, will fix up 1 & 2. 3: should i remove it or just rephrase it... it was helpful to know why the system was failing to fetch. perhaps something like 'cacert not found, skipping.' ?
          Hide
          Damyon Wiese added a comment -

          It uses the system installed certs in this case (but only on unix - because windows does not have any). So maybe a message like: "cacerts file not found, relying on system cacerts"

          Show
          Damyon Wiese added a comment - It uses the system installed certs in this case (but only on unix - because windows does not have any). So maybe a message like: "cacerts file not found, relying on system cacerts"
          Hide
          Aparup Banerjee added a comment -

          updated branch

          Show
          Aparup Banerjee added a comment - updated branch
          Hide
          Damyon Wiese added a comment -

          Thanks Aparup,

          I have integrated this to master now. I tested it in integration with the local_codechecker plugin and everything worked correctly - but I'll get a tester to test it again (the more testing the better at this stage).

          Show
          Damyon Wiese added a comment - Thanks Aparup, I have integrated this to master now. I tested it in integration with the local_codechecker plugin and everything worked correctly - but I'll get a tester to test it again (the more testing the better at this stage).
          Hide
          David Mudrak added a comment -

          Hi. Thanks for working on this! Couple of notes.

          1. Crap, sorry. I did not realize that mdeploy has not been following redirections since day 0 :-/
          2. We need to backport this into stables as well right? The redirect URL is common for all Moodle sites so by changing the download URL, we probably made this stop working on stables.
          3. Why was the "and" changed to "&&" at https://github.com/nebgor/moodle/compare/moodle:master...MDL-39664#L0R1115 ? Can we please put "and" back?
          Show
          David Mudrak added a comment - Hi. Thanks for working on this! Couple of notes. Crap, sorry. I did not realize that mdeploy has not been following redirections since day 0 :-/ We need to backport this into stables as well right? The redirect URL is common for all Moodle sites so by changing the download URL, we probably made this stop working on stables. Why was the "and" changed to "&&" at https://github.com/nebgor/moodle/compare/moodle:master...MDL-39664#L0R1115 ? Can we please put "and" back?
          Hide
          David Mudrak added a comment -

          Could MDL-39652 be caused by this?

          Show
          David Mudrak added a comment - Could MDL-39652 be caused by this?
          Hide
          Aparup Banerjee added a comment - - edited

          ah Rosie: this would be useful for your testing http://docs.moodle.org/25/en/SSL_certificate_for_moodle.org

          DAvid, yes i agree it needs backporting to 2.4 (2.3 also?)

          crap i didn't realise i broke the is_array test there.. fixing.

          Edit:
          i've pushed up the fix for the 'and' and pushed up branch MDL-39664_m24 (it picked across cleanly).

          Show
          Aparup Banerjee added a comment - - edited ah Rosie: this would be useful for your testing http://docs.moodle.org/25/en/SSL_certificate_for_moodle.org DAvid, yes i agree it needs backporting to 2.4 (2.3 also?) crap i didn't realise i broke the is_array test there.. fixing. Edit: i've pushed up the fix for the 'and' and pushed up branch MDL-39664 _m24 (it picked across cleanly).
          Hide
          Damyon Wiese added a comment -

          I've pushed the extra fix for this to master, but we can't backport to 24 right now because the release is already built. I'll have to create a new issue for the backport.

          Show
          Damyon Wiese added a comment - I've pushed the extra fix for this to master, but we can't backport to 24 right now because the release is already built. I'll have to create a new issue for the backport.
          Hide
          Damyon Wiese added a comment -

          Linking to new backport issue.

          Show
          Damyon Wiese added a comment - Linking to new backport issue.
          Hide
          Rossiani Wijaya added a comment -

          This is working as expected for 2.5.

          Thanks Apu for helping testing this.

          Test passed.

          Show
          Rossiani Wijaya added a comment - This is working as expected for 2.5. Thanks Apu for helping testing this. Test passed.
          Hide
          Aparup Banerjee added a comment - - edited

          The redirects have been turned off. This could use another test (just to be sure).

          Show
          Aparup Banerjee added a comment - - edited The redirects have been turned off. This could use another test (just to be sure).
          Hide
          Damyon Wiese added a comment -

          I have just retest this both with and without this patch and both are working for me.

          Show
          Damyon Wiese added a comment - I have just retest this both with and without this patch and both are working for me.
          Hide
          Mary Cooch added a comment -

          I'm just going through the docs_required labels doing some housekeeping and I wondered if this should be dev_docs required rather than user docs? (If it's user docs then when the rush has died down I'd appreciate some help understanding it )

          Show
          Mary Cooch added a comment - I'm just going through the docs_required labels doing some housekeeping and I wondered if this should be dev_docs required rather than user docs? (If it's user docs then when the rush has died down I'd appreciate some help understanding it )
          Hide
          Aparup Banerjee added a comment -

          MaryC, afaik this should be the document : http://docs.moodle.org/24/en/Automatic_updates_deployment

          i don't know how this would add anything to user docs though. this should be transparent totally. (even for dev imo)

          Show
          Aparup Banerjee added a comment - MaryC, afaik this should be the document : http://docs.moodle.org/24/en/Automatic_updates_deployment i don't know how this would add anything to user docs though. this should be transparent totally. (even for dev imo)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          One of the very last fixes before 2.5, well done!

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - One of the very last fixes before 2.5, well done! Closing as fixed, ciao
          Hide
          Mary Cooch added a comment -

          Removing docs _required label; thanks

          Show
          Mary Cooch added a comment - Removing docs _required label; thanks

            People

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

              Dates

              • Created:
                Updated:
                Resolved: