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

pluginlib.php plugin_external_source() functions ignore Mercurial repositories

    Details

    • Testing Instructions:
      Hide

      1. Create a Mercurial repository holding a 3d party plugin in you Moodle installation ensuring the plugin's folder is writeable by the web server (repository folder ".hg" should be just inside you plugin directory, not outside it - e.g. version.php for example should be in the root directory of repository). You would want an obsolete version of plugin with newer version on Moodle plugin database.
      2. Try to update this plugin from Moodle Plugins database and see that you get confirmation warning about plugin been from external mercurial source and not been able to update it from repository anymore if you continue. Say "no" to updating, to be able to do next test.

      Show
      1. Create a Mercurial repository holding a 3d party plugin in you Moodle installation ensuring the plugin's folder is writeable by the web server (repository folder ".hg" should be just inside you plugin directory, not outside it - e.g. version.php for example should be in the root directory of repository). You would want an obsolete version of plugin with newer version on Moodle plugin database. 2. Try to update this plugin from Moodle Plugins database and see that you get confirmation warning about plugin been from external mercurial source and not been able to update it from repository anymore if you continue. Say "no" to updating, to be able to do next test.
    • Affected Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      There are plugin_external_source() functions checking that plugin come from external repository. They check for CVS, SNV, Git but not Mercurial repository.

      Easy to fix one: there should be check for .hg folder there too.

      Our team use Mercurial to develop plugins.

        Gliffy Diagrams

          Activity

          Hide
          salvetore Michael de Raadt added a comment -

          Thanks for suggesting this.

          Feel free to help us work on this issue. If you are able to provide a patch or links to your Git repository branch, please add a "patch" label so we will spot it.

          Show
          salvetore Michael de Raadt added a comment - Thanks for suggesting this. Feel free to help us work on this issue. If you are able to provide a patch or links to your Git repository branch, please add a "patch" label so we will spot it.
          Hide
          oa_sychev Oleg Sychev added a comment -

          Easy thing, thought I don't really understand why Moodle has two identical functions in different files. Well, maybe it has it's reasons...

          Show
          oa_sychev Oleg Sychev added a comment - Easy thing, thought I don't really understand why Moodle has two identical functions in different files. Well, maybe it has it's reasons...
          Show
          cibot CiBoT added a comment - Results for MDL-42456 Remote repository: https://github.com/oasychev/moodle Remote branch MDL-42456 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/795 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/795/artifact/work/smurf.html
          Hide
          oa_sychev Oleg Sychev added a comment -

          Hi, Michael and everyone. You may not getting e-mails for this issue updates due to general problems on tracker, please look at it - there is a code patch there now.

          Show
          oa_sychev Oleg Sychev added a comment - Hi, Michael and everyone. You may not getting e-mails for this issue updates due to general problems on tracker, please look at it - there is a code patch there now.
          Hide
          poltawski Dan Poltawski added a comment -

          Hi Oleg,

          Thanks a lot for the patch!

          1. I'm not clear why we have two of these functions either - my first thought was that this was because one was a duplicate of the limited functions used for the Moodle deployer (which doesn't have whole Moodle environment available). However, as far as I can see that doesn't look to be the case. I'm wondering if David Mudrak can shed any light on that.
          2. As a bug this fix should be back ported to our supported stable branches (26 and 25 as well as master). Could you provide branches for those?
          3. It would be helpful for the tester if you could provide a sample hg plugin which does this - do you happen to know of one? Which could be used?

          thanks!
          Dan

          Show
          poltawski Dan Poltawski added a comment - Hi Oleg, Thanks a lot for the patch! I'm not clear why we have two of these functions either - my first thought was that this was because one was a duplicate of the limited functions used for the Moodle deployer (which doesn't have whole Moodle environment available). However, as far as I can see that doesn't look to be the case. I'm wondering if David Mudrak can shed any light on that. As a bug this fix should be back ported to our supported stable branches (26 and 25 as well as master). Could you provide branches for those? It would be helpful for the tester if you could provide a sample hg plugin which does this - do you happen to know of one? Which could be used? thanks! Dan
          Hide
          mudrd8mz David Mudrak added a comment -

          Hi guys

          to be completely honest, I can't remember why the functionality is copied there. One potential reason could be that at the time when the original deployer class was written, it was pretty expensive to get the instance of the plugin_manager. It used to load all installed plugins etc as it was intended to display those full overview pages. So maybe my reasoning was that it was not worth of initializing the full plugin_manager just to check if there are some directories present. And thence maybe I decided that copying the method was lesser of two evils.

          However, I know that the whole machinery has evolved a bit since those times. But I'm really not involved much in this area any more. IIRC Petr Skoda got his hands dirty by digging into this recently. He might have a suggestion for eventual refactoring.

          In the meantime, my +1 for this quick patch that modifies both places.

          Show
          mudrd8mz David Mudrak added a comment - Hi guys to be completely honest, I can't remember why the functionality is copied there. One potential reason could be that at the time when the original deployer class was written, it was pretty expensive to get the instance of the plugin_manager. It used to load all installed plugins etc as it was intended to display those full overview pages. So maybe my reasoning was that it was not worth of initializing the full plugin_manager just to check if there are some directories present. And thence maybe I decided that copying the method was lesser of two evils. However, I know that the whole machinery has evolved a bit since those times. But I'm really not involved much in this area any more. IIRC Petr Skoda got his hands dirty by digging into this recently. He might have a suggestion for eventual refactoring. In the meantime, my +1 for this quick patch that modifies both places.
          Hide
          skodak Petr Skoda added a comment -

          I do not see any problem with duplication now, +1 for the patch - we can refactor it later

          Show
          skodak Petr Skoda added a comment - I do not see any problem with duplication now, +1 for the patch - we can refactor it later
          Hide
          cibot CiBoT added a comment -

          Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

          Show
          cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Thanks Oleg, this has been integrated now.

          Show
          samhemelryk Sam Hemelryk added a comment - Thanks Oleg, this has been integrated now.
          Hide
          markn Mark Nelson added a comment -

          I tested this on 2.5 and 2.6, but not on master. There are no plugins in the Moodle plugin DB that support 2.7 (that I know of - I don't actually think it's even possible to upload a plugin for this version atm), so it will never find an update.

          For 2.5 and 2.6, step 2 works, but step 3 does not. Thinking about this, I don't actually think there is ever a warning about the CVS software being used before deleting a plugin (at least I have never seen it). Are you sure this is actually supposed to occur?

          Show
          markn Mark Nelson added a comment - I tested this on 2.5 and 2.6, but not on master. There are no plugins in the Moodle plugin DB that support 2.7 (that I know of - I don't actually think it's even possible to upload a plugin for this version atm), so it will never find an update. For 2.5 and 2.6, step 2 works, but step 3 does not. Thinking about this, I don't actually think there is ever a warning about the CVS software being used before deleting a plugin (at least I have never seen it). Are you sure this is actually supposed to occur?
          Hide
          rajeshtaneja Rajesh Taneja added a comment - - edited

          Ping: Oleg / Dan / Sam,

          Can you please confirm if step 3 is expected to work (Please refer Mark's comment above.)

          Show
          rajeshtaneja Rajesh Taneja added a comment - - edited Ping: Oleg / Dan / Sam, Can you please confirm if step 3 is expected to work (Please refer Mark's comment above.)
          Hide
          rajeshtaneja Rajesh Taneja added a comment -

          I am sending this back for testing and will test this.

          Show
          rajeshtaneja Rajesh Taneja added a comment - I am sending this back for testing and will test this.
          Hide
          rajeshtaneja Rajesh Taneja added a comment -

          As Mark has left for the day, I will test this on his behalf.

          Show
          rajeshtaneja Rajesh Taneja added a comment - As Mark has left for the day, I will test this on his behalf.
          Hide
          rajeshtaneja Rajesh Taneja added a comment -

          Thanks for fixing this Oleg,

          I created .hg directory and tried updating plugin and I could see mercurial message.

          Steps I followed:

          1. I installed registration activity
          2. Decremented addon version in db and version.php
          3. Went to plugin overview and could see "install update" button.
          4. Add .hg directory to registration plugin
          5. Clicked "Install update" and it showed message with Mercurial.

          While uninstalling plugin, I didn't get any specific repository message, but that seems to be drawback in step 3 of testing instructions.

          Passing this as patch allows moodle to detect mercurial repository as it do for git/svn/cvs.

          FYI: Thanks to Dan P for helping me in testing.

          Show
          rajeshtaneja Rajesh Taneja added a comment - Thanks for fixing this Oleg, I created .hg directory and tried updating plugin and I could see mercurial message. Steps I followed: I installed registration activity Decremented addon version in db and version.php Went to plugin overview and could see "install update" button. Add .hg directory to registration plugin Clicked "Install update" and it showed message with Mercurial. While uninstalling plugin, I didn't get any specific repository message, but that seems to be drawback in step 3 of testing instructions. Passing this as patch allows moodle to detect mercurial repository as it do for git/svn/cvs. FYI: Thanks to Dan P for helping me in testing.
          Hide
          samhemelryk Sam Hemelryk added a comment -

          This weeks weekly release is now available and includes your code.
          A big pat on the back to you again for once more being a cog in the Moodle machine.

          Best wishes, the Moodle integration team.

          Show
          samhemelryk Sam Hemelryk added a comment - This weeks weekly release is now available and includes your code. A big pat on the back to you again for once more being a cog in the Moodle machine. Best wishes, the Moodle integration team.
          Hide
          oa_sychev Oleg Sychev added a comment -

          I'm sorry for not been able to respond for a week, I was on my holidays well away from the computer and internet. It seems, all get along well, thanks for all involved!

          Show
          oa_sychev Oleg Sychev added a comment - I'm sorry for not been able to respond for a week, I was on my holidays well away from the computer and internet. It seems, all get along well, thanks for all involved!

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                10/Mar/14