Moodle
  1. Moodle
  2. MDL-29218

repository plugin plugin_init call not blocking

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.3, 2.4.5, 2.5.1
    • Fix Version/s: 2.4.6, 2.5.2
    • Component/s: Repositories
    • Labels:
    • Database:
      MySQL
    • Testing Instructions:
      Hide

      (difficulty: moderate, requires to add one line of code to simulate a failure)

      Test 1: repository creation/initialization management

      1. Open the repository/flickr_public/lib.php file and change the plugin_init function as below:
        diff --git a/repository/flickr_public/lib.php b/repository/flickr_public/lib.php
        index a0a5c72..8ab8847 100644
        --- a/repository/flickr_public/lib.php
        +++ b/repository/flickr_public/lib.php
        @@ -527,6 +527,7 @@ class repository_flickr_public extends repository {
              * is run when moodle administrator add the plugin
              */
             public static function plugin_init() {
        +return false;
                 //here we create a default instance for this type
         
                 $id = repository::static_function('flickr_public','create', 'flickr_public', 0, get_system_context(), array('name'=>'', 'email_address' => null, 'usewatermarks' => false), 0);
        
      2. Go to Home ► Site administration ► Plugins ► Repositories ► Manage repositories, find Flickr public in the repositories list and choose Enabled and visible from the dropdown
      3. Put a valid or invalid API key in the filed and click Save
      4. You get an expected Call plugin_init failed exception error
      5. Press continue and you will be redirected to the home page
      6. Go to Home ► Site administration ► Plugins ► Repositories ► Manage repositories: the Flickr public repository is enabled but not visible

      Test 2: look for any regression

      1. Remove the hack added in the test above (return false;)
      2. Configure a repository as per the official docs, e.g. Flickr public. The operation must be successful
      3. Access that repository: again the operation must be successful
      Show
      (difficulty: moderate, requires to add one line of code to simulate a failure) Test 1: repository creation/initialization management Open the repository/flickr_public/lib.php file and change the plugin_init function as below: diff --git a/repository/flickr_public/lib.php b/repository/flickr_public/lib.php index a0a5c72..8ab8847 100644 --- a/repository/flickr_public/lib.php +++ b/repository/flickr_public/lib.php @@ -527,6 +527,7 @@ class repository_flickr_public extends repository { * is run when moodle administrator add the plugin */ public static function plugin_init() { + return false ; //here we create a default instance for this type $id = repository::static_function('flickr_public','create', 'flickr_public', 0, get_system_context(), array('name'=>'', 'email_address' => null , 'usewatermarks' => false ), 0); Go to Home ► Site administration ► Plugins ► Repositories ► Manage repositories , find Flickr public in the repositories list and choose Enabled and visible from the dropdown Put a valid or invalid API key in the filed and click Save You get an expected Call plugin_init failed exception error Press continue and you will be redirected to the home page Go to Home ► Site administration ► Plugins ► Repositories ► Manage repositories : the Flickr public repository is enabled but not visible Test 2: look for any regression Remove the hack added in the test above ( return false; ) Configure a repository as per the official docs, e.g. Flickr public . The operation must be successful Access that repository: again the operation must be successful
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
      m24_MDL-29218_plugin_init_not_blocking_when_false
    • Pull 2.5 Branch:
      m25_MDL-29218_plugin_init_not_blocking_when_false
    • Pull Master Branch:
      m26_MDL-29218_plugin_init_not_blocking_when_false
    • Rank:
      18760

      Description

      If you try to enable a repository plugin but some of the requirements in plugin_init function is not met (the function returns false), you correctly get an error message.
      But if you enter again in Home / Site administration / Plugins / Repositories / Manage repositories you will see that the repository plugin is enabled and you can add repository instances.

      I think that this should be blocking and the repository plugin shouldn't be enabled at all.

      To reproduce the issue I modified flickr_public repository plugin to force the plugin_init() function to return false (to simulate an error status). This is the modified version of plugin_init() function in repository/flickr_public/lib.php

       /**
       * is run when moodle administrator add the plugin
       */
      public static function plugin_init() {
          return false;
          //here we create a default instance for this type
      
          $id = repository::static_function('flickr_public','create', 'flickr_public', 0, get_system_context(), array('name'=>'', 'email_address' => null, 'usewatermarks' => false), 0);
          if (empty($id)) {
              return false;
          } else {
              return true;
          }
      }
      
      • Go to Home / Site administration / Plugins / Repositories / Manage repositories, find Flickr public in the repository list and choose Enabled and visible from dropdown
      • Put a valid or invalid API key in the filed and click Save
      • You get an "Call plugin_init failed" error (this is expected)
      • Press continue and you will be redirected to moodle home page
      • Go to Home / Site administration / Plugins / Repositories / Manage repositories
      • It is expected that Flickr public repository is not enabled but you can see that it is enabled instead and you can press settings and add repository instances

        Activity

        Hide
        Matteo Scaramuccia added a comment -

        I guess a missing somewhere, probably here:

        $ git diff repository.php
        diff --git a/admin/repository.php b/admin/repository.php
        index c3409a5..6960c7a 100644
        --- a/admin/repository.php
        +++ b/admin/repository.php
        @@ -121,8 +121,10 @@ if (($action == 'edit') || ($action == 'new')) {
                     $success = $repositorytype->update_options($settings);
                 } else {
                     $type = new repository_type($plugin, (array)$fromform, $visible);
        -            $type->create();
                     $success = true;
        +            if(!$repoid = $type->create(true)) {
        +                $success = false;
        +            }
                     $data = data_submitted();
                 }
                 if ($success) {
        
        

        or:

        • let the repo instance be created and hide it (repository_type::hide(true)) in case of failure.

        I will look at it during the next days: I'm interested in understanding the Moodle repos backend.

        Show
        Matteo Scaramuccia added a comment - I guess a missing somewhere, probably here: $ git diff repository.php diff --git a/admin/repository.php b/admin/repository.php index c3409a5..6960c7a 100644 --- a/admin/repository.php +++ b/admin/repository.php @@ -121,8 +121,10 @@ if (($action == 'edit') || ($action == ' new ')) { $success = $repositorytype->update_options($settings); } else { $type = new repository_type($plugin, (array)$fromform, $visible); - $type->create(); $success = true ; + if (!$repoid = $type->create( true )) { + $success = false ; + } $data = data_submitted(); } if ($success) { or: let the repo instance be created and hide it ( repository_type::hide(true) ) in case of failure. I will look at it during the next days: I'm interested in understanding the Moodle repos backend.
        Hide
        Michael de Raadt added a comment -

        Thanks for reporting this and providing a solution.

        I've put it on our backlog and we'll try to get to it as soon as we can.

        Comes with the Dongsheng seal of approval.

        Show
        Michael de Raadt added a comment - Thanks for reporting this and providing a solution. I've put it on our backlog and we'll try to get to it as soon as we can. Comes with the Dongsheng seal of approval.
        Hide
        Dan Poltawski added a comment -

        Wow, just tested this and I thought it was working ok because you get the error, but indeed, the plugin is enabled after the error.

        Show
        Dan Poltawski added a comment - Wow, just tested this and I thought it was working ok because you get the error, but indeed, the plugin is enabled after the error.
        Hide
        Dan Poltawski added a comment -

        Hi Matteo,

        I notice that you've created a patch for this and it looks OK to me.

        If you have time, could you create a git branch for it and then we could finally get this old bug fixed?

        If not, feel free to assign it to me and i'll prepare your patch for peer review.

        thanks!

        Show
        Dan Poltawski added a comment - Hi Matteo, I notice that you've created a patch for this and it looks OK to me. If you have time, could you create a git branch for it and then we could finally get this old bug fixed? If not, feel free to assign it to me and i'll prepare your patch for peer review. thanks!
        Hide
        Matteo Scaramuccia added a comment -

        Will do in the weekend.

        Show
        Matteo Scaramuccia added a comment - Will do in the weekend.
        Hide
        Dan Poltawski added a comment -

        Many thanks.

        Show
        Dan Poltawski added a comment - Many thanks.
        Hide
        Matteo Scaramuccia added a comment -

        Hi Dan,
        I've kept the exception, here are the PRs: not sure about 2.3.

        Show
        Matteo Scaramuccia added a comment - Hi Dan, I've kept the exception, here are the PRs: not sure about 2.3.
        Hide
        Matteo Scaramuccia added a comment -

        Please Dan, while reviewing my code look at the repository/lib.php overall file too.
        While looking at it I've found a small issue: together with a trivial typo in PHPDoc the code for repository::static_function could be already open to manage repository objects too: a separate issue to complete the extension (see the diff below) or to remove that code to be compliant with the current declared signature?

        diff --git a/repository/lib.php b/repository/lib.php
        index 317882c..4c3fca7 100644
        --- a/repository/lib.php
        +++ b/repository/lib.php
        @@ -1139,8 +1139,8 @@ abstract class repository implements cacheable_object {
              * Call a static function. Any additional arguments than plugin and function will be passed through.
              *
              * @static
        -     * @param string $plugin repository plugin name
        -     * @param string $function funciton name
        +     * @param mixed $plugin repository plugin name or object
        +     * @param string $function function name
              * @return mixed
              */
             public static function static_function($plugin, $function) {
        @@ -1170,7 +1170,7 @@ abstract class repository implements cacheable_object {
                 }
        
                 require_once($typedirectory);
        -        return call_user_func_array(array('repository_' . $plugin, $function), $args);
        +        return call_user_func_array(array('repository_' . $pname, $function), $args);
             }
        
             /**
        @@ -2412,7 +2412,7 @@ abstract class repository implements cacheable_object {
        
             /**
              * For oauth like external authentication, when external repository direct user back to moodle,
        -     * this funciton will be called to set up token and token_secret
        +     * this function will be called to set up token and token_secret
              */
             public function callback() {
             }
        
        
        Show
        Matteo Scaramuccia added a comment - Please Dan, while reviewing my code look at the repository/lib.php overall file too. While looking at it I've found a small issue: together with a trivial typo in PHPDoc the code for repository::static_function could be already open to manage repository objects too: a separate issue to complete the extension (see the diff below) or to remove that code to be compliant with the current declared signature? diff --git a/repository/lib.php b/repository/lib.php index 317882c..4c3fca7 100644 --- a/repository/lib.php +++ b/repository/lib.php @@ -1139,8 +1139,8 @@ abstract class repository implements cacheable_object { * Call a static function. Any additional arguments than plugin and function will be passed through. * * @ static - * @param string $plugin repository plugin name - * @param string $function funciton name + * @param mixed $plugin repository plugin name or object + * @param string $function function name * @ return mixed */ public static function static_function($plugin, $function) { @@ -1170,7 +1170,7 @@ abstract class repository implements cacheable_object { } require_once($typedirectory); - return call_user_func_array(array('repository_' . $plugin, $function), $args); + return call_user_func_array(array('repository_' . $pname, $function), $args); } /** @@ -2412,7 +2412,7 @@ abstract class repository implements cacheable_object { /** * For oauth like external authentication, when external repository direct user back to moodle, - * this funciton will be called to set up token and token_secret + * this function will be called to set up token and token_secret */ public function callback() { }
        Hide
        Dan Poltawski added a comment -

        Hi Matteo,

        I'm not quite following your second point?

        Show
        Dan Poltawski added a comment - Hi Matteo, I'm not quite following your second point?
        Hide
        Matteo Scaramuccia added a comment -

        Hi Dan,
        please open repository::static_function function and search for $pname: it has been thought/coded to support the given $plugin to be a string or an object but never used so the PHPDoc tells the truth about the given $plugin (string) but we could allow $pname to do its job and extend the support of $plugin to be mixed (string or object); see the last line, in the return call. On the opposite, we should remove $pname's code to stick with the given $plugin being a string.

        Kind of cleanup, with a BC api change.

        Show
        Matteo Scaramuccia added a comment - Hi Dan, please open repository::static_function function and search for $pname : it has been thought/coded to support the given $plugin to be a string or an object but never used so the PHPDoc tells the truth about the given $plugin ( string ) but we could allow $pname to do its job and extend the support of $plugin to be mixed ( string or object ); see the last line, in the return call. On the opposite, we should remove $pname 's code to stick with the given $plugin being a string . Kind of cleanup, with a BC api change.
        Hide
        Matteo Scaramuccia added a comment -

        @Dan: ping. Would you like me to rebase the current implementation w/o the enh (== both plugin name and object)?

        Show
        Matteo Scaramuccia added a comment - @Dan: ping. Would you like me to rebase the current implementation w/o the enh (== both plugin name and object)?
        Hide
        Dan Poltawski added a comment -

        Hi Matteo,

        Really sorry I managed to miss this (and then was on holiday). Please feel free to shout louder at me if doing that badly in the future!

        I think that it makes sense to fix both of these issues in the one issue if you can provide testing instructions for both then lease rebase and i'll send to integration.

        Show
        Dan Poltawski added a comment - Hi Matteo, Really sorry I managed to miss this (and then was on holiday). Please feel free to shout louder at me if doing that badly in the future! I think that it makes sense to fix both of these issues in the one issue if you can provide testing instructions for both then lease rebase and i'll send to integration.
        Hide
        Matteo Scaramuccia added a comment -

        Hi Dan,
        backing from my holidays too .
        Would you like me to:

        1. (code cleanup) drop the lines around $pname;
        2. (BC, 2.6 only) add support for both string (repository name) and object (repository object). upgrade.txt update included;
        3. (BC) same as above but for all versions;

        ?
        I think (1) is the safest option, since it seems none has ever required to pass the object directly but from your comment it could seem you're seconding (2) or even (3), too.

        TIA,
        Matteo

        Show
        Matteo Scaramuccia added a comment - Hi Dan, backing from my holidays too . Would you like me to: (code cleanup) drop the lines around $pname ; (BC, 2.6 only) add support for both string (repository name) and object (repository object). upgrade.txt update included; (BC) same as above but for all versions; ? I think (1) is the safest option, since it seems none has ever required to pass the object directly but from your comment it could seem you're seconding (2) or even (3), too. TIA, Matteo
        Hide
        Dan Poltawski added a comment -

        Hi Matteo,

        I think 1, get rid of all the useless code. Support for object can never have worked (and i don't see it having sense to add it) because of the file_exists check and I don't think it makes sense to add it now.

        cheers,
        Dan

        Show
        Dan Poltawski added a comment - Hi Matteo, I think 1, get rid of all the useless code. Support for object can never have worked (and i don't see it having sense to add it) because of the file_exists check and I don't think it makes sense to add it now. cheers, Dan
        Hide
        Matteo Scaramuccia added a comment -

        Changed according with the comments above.
        2.3 has been updated&changed too, since the branch was created at the time of the first proposal.

        Show
        Matteo Scaramuccia added a comment - Changed according with the comments above. 2.3 has been updated&changed too, since the branch was created at the time of the first proposal.
        Hide
        Matteo Scaramuccia added a comment -

        Hi Dan,
        could you look at the new commits?

        TIA,
        Matteo

        Show
        Matteo Scaramuccia added a comment - Hi Dan, could you look at the new commits? TIA, Matteo
        Hide
        Dan Poltawski added a comment -

        Hi Matteo,

        Looks good, sending for integration.

        My only comment is not to put in the commit message 'more details in the tracker', either put the details in the commit or not at all

        thanks for persevering with me!

        Show
        Dan Poltawski added a comment - Hi Matteo, Looks good, sending for integration. My only comment is not to put in the commit message 'more details in the tracker', either put the details in the commit or not at all thanks for persevering with me!
        Hide
        Matteo Scaramuccia added a comment -

        "more details": now, lesson learned!

        TNX,
        Matteo

        Show
        Matteo Scaramuccia added a comment - "more details": now, lesson learned! TNX, Matteo
        Hide
        Matteo Scaramuccia added a comment -

        Rebased.

        Show
        Matteo Scaramuccia added a comment - Rebased.
        Hide
        Damyon Wiese added a comment -

        Thanks Matteo,

        I tested this on 24 and it works as described.

        Integrated to 24, 25 and master.

        Cheers, Damyon

        Show
        Damyon Wiese added a comment - Thanks Matteo, I tested this on 24 and it works as described. Integrated to 24, 25 and master. Cheers, Damyon
        Hide
        Andrew Davis added a comment -

        Im not able to get the error in test 1. I've made the code change but enabling flickr always seems to think it has worked.

        Show
        Andrew Davis added a comment - Im not able to get the error in test 1. I've made the code change but enabling flickr always seems to think it has worked.
        Hide
        Matteo Scaramuccia added a comment -

        Hi Andrew,
        did you remove any public flickr repo instance, hack its plugin_init() with return false; and re-add one instance?

        Show
        Matteo Scaramuccia added a comment - Hi Andrew, did you remove any public flickr repo instance, hack its plugin_init() with return false; and re-add one instance?
        Hide
        Andrew Davis added a comment -

        I disabled the flickr repo type (which warned me it would automatically remove instances from memory), changed the code then re-enabled it and entered bogus configuration values.

        Show
        Andrew Davis added a comment - I disabled the flickr repo type (which warned me it would automatically remove instances from memory), changed the code then re-enabled it and entered bogus configuration values.
        Hide
        Damyon Wiese added a comment -

        Andrew - are you confusing the flickr and flickr_public repositories ? (I did that when I tested this).

        Show
        Damyon Wiese added a comment - Andrew - are you confusing the flickr and flickr_public repositories ? (I did that when I tested this).
        Hide
        Andrew Davis added a comment - - edited

        That may have been what I was doing actually. Im getting something else now. If I set flickr public to enabled and visible I get the error but when I go back to the manage plugins screen it is set to enabled but hidden. Kind of makes sense. Keep it hidden until the config is sorted out however the testing instructions say it shouldnt be enabled. Is the code or the testing instructions incorrect?

        Test 2 passes without any issues.

        Show
        Andrew Davis added a comment - - edited That may have been what I was doing actually. Im getting something else now. If I set flickr public to enabled and visible I get the error but when I go back to the manage plugins screen it is set to enabled but hidden. Kind of makes sense. Keep it hidden until the config is sorted out however the testing instructions say it shouldnt be enabled. Is the code or the testing instructions incorrect? Test 2 passes without any issues.
        Hide
        Matteo Scaramuccia added a comment -

        Hi Andrew,
        thanks for spotting the issue on the testing instructions!
        At the time of writing them, I was confident to reach that behavior but then I've found that it was not simple to change it given the code design I found... missed to update the instructions since that.

        Show
        Matteo Scaramuccia added a comment - Hi Andrew, thanks for spotting the issue on the testing instructions! At the time of writing them, I was confident to reach that behavior but then I've found that it was not simple to change it given the code design I found... missed to update the instructions since that.
        Hide
        Andrew Davis added a comment -

        Cool. Passing.

        Show
        Andrew Davis added a comment - Cool. Passing.
        Hide
        Dan Poltawski added a comment -

        Congratulations! This change has been integrated upstream and is now available from our git and download mirrors. To celebrate, here is a joke:

        A SQL query goes into a bar, walks up to two tables and asks, "Can I join you?"

        Show
        Dan Poltawski added a comment - Congratulations! This change has been integrated upstream and is now available from our git and download mirrors. To celebrate, here is a joke: A SQL query goes into a bar, walks up to two tables and asks, "Can I join you?"

          People

          • Votes:
            1 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: