Moodle
  1. Moodle
  2. MDL-32329

Web/CLI installers discreprency about calling all_plugins_ok()

    Details

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

      Testing difficulty: medium (requires complete control over source code and database)

      1. Prepare the environment as if you wanted to do the fresh install of 2.3dev. (empty DB, empty moodledata). Do not install yet though.
      2. Modify some plugins version.php to simulate unsatisfied dependencies. On my machine, I used something like:

      diff --git a/enrol/ldap/version.php b/enrol/ldap/version.php
      index 3971fb5..2c4a2fa 100644
      --- a/enrol/ldap/version.php
      +++ b/enrol/ldap/version.php
      @@ -27,5 +27,5 @@
       defined('MOODLE_INTERNAL') || die();
       
       $plugin->version   = 2011112900;        // The current plugin version (Date: YYYYMMDDXX)
      -$plugin->requires  = 2011112900;        // Requires this Moodle version
      +$plugin->requires  = 9999999999;        // Requires this Moodle version
       $plugin->component = 'enrol_ldap';      // Full name of the plugin (used for diagnostics)
      diff --git a/mod/workshop/version.php b/mod/workshop/version.php
      index 12a8373..f65f9c7 100644
      --- a/mod/workshop/version.php
      +++ b/mod/workshop/version.php
      @@ -25,6 +25,6 @@
       defined('MOODLE_INTERNAL') || die();
       
       $module->version   = 2012041701;        // the current module version (YYYYMMDDXX)
      -$module->requires  = 2012032300;        // requires this Moodle version
      +$module->requires  = 9999999999;        // requires this Moodle version
       $module->component = 'mod_workshop';    // full name of the plugin (used for diagnostics)
       $module->cron      = 60;                // give as a chance every minute
      diff --git a/question/type/calculatedsimple/version.php b/question/type/calculatedsimple/version.php
      index 5dcb02e..fb3343b 100644
      --- a/question/type/calculatedsimple/version.php
      +++ b/question/type/calculatedsimple/version.php
      @@ -30,7 +30,7 @@ $plugin->version   = 2011102700;
       
       $plugin->requires  = 2011102700;
       $plugin->dependencies = array(
      -    'qtype_numerical'  => 2011102700,
      +    'qtype_numerical'  => 9999999999,
           'qtype_calculated' => 2011102700,
       );
      

      3. Try all three ways of install: admin/cli/install.php, admin/cli/install_database.php and via the web.
      4. TEST: Make sure that in all three cases, the install procedure halts on plugin dependencies check.
      5. Undo local changes in version.php files (hint: `git stash` command is your friend here) and proceed with installation.
      6. Once the site is installed and set-up, tweak some version.php files again to simulate unsatisfied dependencies (`git stash pop`).
      7. Increase the main core $version in /version.php
      8. TEST: Make sure that in both ways of upgrading (admin/cli/upgrade.php and via the web) the upgrade stops on plugin dependencies check.
      9. Undo the change in the main version.php and instead increase the version of any installed plugin (eg. mod/forum/version.php).
      10. TEST: Again, make sure that in both ways of upgrading (admin/cli/upgrade.php and via the web) the upgrade stops on plugin dependencies check.

      Show
      Testing difficulty: medium (requires complete control over source code and database) 1. Prepare the environment as if you wanted to do the fresh install of 2.3dev. (empty DB, empty moodledata). Do not install yet though. 2. Modify some plugins version.php to simulate unsatisfied dependencies. On my machine, I used something like: diff --git a/enrol/ldap/version.php b/enrol/ldap/version.php index 3971fb5..2c4a2fa 100644 --- a/enrol/ldap/version.php +++ b/enrol/ldap/version.php @@ -27,5 +27,5 @@ defined('MOODLE_INTERNAL') || die(); $plugin->version = 2011112900; // The current plugin version (Date: YYYYMMDDXX) -$plugin->requires = 2011112900; // Requires this Moodle version +$plugin->requires = 9999999999; // Requires this Moodle version $plugin->component = 'enrol_ldap'; // Full name of the plugin (used for diagnostics) diff --git a/mod/workshop/version.php b/mod/workshop/version.php index 12a8373..f65f9c7 100644 --- a/mod/workshop/version.php +++ b/mod/workshop/version.php @@ -25,6 +25,6 @@ defined('MOODLE_INTERNAL') || die(); $module->version = 2012041701; // the current module version (YYYYMMDDXX) -$module->requires = 2012032300; // requires this Moodle version +$module->requires = 9999999999; // requires this Moodle version $module->component = 'mod_workshop'; // full name of the plugin (used for diagnostics) $module->cron = 60; // give as a chance every minute diff --git a/question/type/calculatedsimple/version.php b/question/type/calculatedsimple/version.php index 5dcb02e..fb3343b 100644 --- a/question/type/calculatedsimple/version.php +++ b/question/type/calculatedsimple/version.php @@ -30,7 +30,7 @@ $plugin->version = 2011102700; $plugin->requires = 2011102700; $plugin->dependencies = array( - 'qtype_numerical' => 2011102700, + 'qtype_numerical' => 9999999999, 'qtype_calculated' => 2011102700, ); 3. Try all three ways of install: admin/cli/install.php, admin/cli/install_database.php and via the web. 4. TEST: Make sure that in all three cases, the install procedure halts on plugin dependencies check. 5. Undo local changes in version.php files (hint: `git stash` command is your friend here) and proceed with installation. 6. Once the site is installed and set-up, tweak some version.php files again to simulate unsatisfied dependencies (`git stash pop`). 7. Increase the main core $version in /version.php 8. TEST: Make sure that in both ways of upgrading (admin/cli/upgrade.php and via the web) the upgrade stops on plugin dependencies check. 9. Undo the change in the main version.php and instead increase the version of any installed plugin (eg. mod/forum/version.php). 10. TEST: Again, make sure that in both ways of upgrading (admin/cli/upgrade.php and via the web) the upgrade stops on plugin dependencies check.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-32329-all-plugins-ok
    • Rank:
      39149

      Description

      As discovered whilst testing MDL-20438, the CLI installer calls all_plugins_ok() which then calls a get_config before the config_plugins table exists.

      In the web installer this code path isn't followed at all.

      So there are two isuses:
      1) The CLI installer is following a different code path to the web installer, this is a nono! We discovered two bugs as a result of this in this weeks integration cycle

      2) I am about to commit a 'hacky' patch to avoid calling the update checker during install. This needs fixing properly with Davids input.

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment -

          Hi, there is an issue for missing plugin check before install somewhere.

          Show
          Petr Škoda added a comment - Hi, there is an issue for missing plugin check before install somewhere.
          Hide
          Dan Poltawski added a comment -

          Is it eloys comment in: MDL-29474
          "1) Web install dependencies check must be installed (TODO)."

          Show
          Dan Poltawski added a comment - Is it eloys comment in: MDL-29474 "1) Web install dependencies check must be installed (TODO)."
          Hide
          David Mudrak added a comment -

          Well. all_plugins_ok() is checked during the web based install/upgrade too. It's just less obvious as Tim Hunt decided to implement it in core_admin_renderer::upgrade_plugin_check_page(). I am going to look at some alternative as I really do not like this abuse of renderers to perform the executive side of the code flow.

          Show
          David Mudrak added a comment - Well. all_plugins_ok() is checked during the web based install/upgrade too. It's just less obvious as Tim Hunt decided to implement it in core_admin_renderer::upgrade_plugin_check_page(). I am going to look at some alternative as I really do not like this abuse of renderers to perform the executive side of the code flow.
          Hide
          David Mudrak added a comment -

          OK, I was wrong. all_plugins_ok() is not checked on install. I think I understand this issue now.

          Show
          David Mudrak added a comment - OK, I was wrong. all_plugins_ok() is not checked on install. I think I understand this issue now.
          Hide
          David Mudrak added a comment -

          Submitting a patchset for integration. Highlighted changes:

          • plugin_manager::all_plugins_ok() now provides the list of plugins with unsatisfied dependencies
          • during the upgrade, the admin/index.php is the one that actually calls all_plugins_ok(). Previously, the renderer method was doing it.
          Show
          David Mudrak added a comment - Submitting a patchset for integration. Highlighted changes: plugin_manager::all_plugins_ok() now provides the list of plugins with unsatisfied dependencies during the upgrade, the admin/index.php is the one that actually calls all_plugins_ok(). Previously, the renderer method was doing it.
          Hide
          Aparup Banerjee added a comment -

          Hi David, just looked through the commits here:

          • just curious as to the uses of array_unique() everywhere. i don't understand why we're keeping possible duplicate components in $failedplugins. why not simply make $failedplugins a unique array within plugin_manager::all_plugins_ok() ? there isn't any way atm to distinguish the components (unless this array was associative). so i see this more as possible confusion then helpful atm imo.
          • admin/renderer.php : unsatisfied_dependencies_page() phpdoc is missing a @param $reloadurl.
          • admin/renderer.php : line 690 has a wrapping style i've not encountered before . it looked like args for '$pluginman->are_dependencies_satisfied(' were all on the next line until closer inspection.

          everything else looks perfect

          Show
          Aparup Banerjee added a comment - Hi David, just looked through the commits here: just curious as to the uses of array_unique() everywhere. i don't understand why we're keeping possible duplicate components in $failedplugins. why not simply make $failedplugins a unique array within plugin_manager::all_plugins_ok() ? there isn't any way atm to distinguish the components (unless this array was associative). so i see this more as possible confusion then helpful atm imo. admin/renderer.php : unsatisfied_dependencies_page() phpdoc is missing a @param $reloadurl. admin/renderer.php : line 690 has a wrapping style i've not encountered before . it looked like args for '$pluginman->are_dependencies_satisfied(' were all on the next line until closer inspection. everything else looks perfect
          Hide
          David Mudrak added a comment -

          Hi Aparup. Thanks for the review.

          • my reasoning behind the array_unique() is that one day, either we or some custom renderer might want to count the frequency of each component in the returned list. So the reported string could be like "Dependencies check failed for: mod_workshop (2x), enrol_ldap". The "2x" would mean that there are two unsatisfied dependencies (one against the core and the other against some other plugin, for example).
          • missing @param fixed
          • you are absolutely right, very well spotted! I fixed the style according the recommendation we have in the Coding style

          I have also rebased the branch against the recent origin/master.

          Show
          David Mudrak added a comment - Hi Aparup. Thanks for the review. my reasoning behind the array_unique() is that one day, either we or some custom renderer might want to count the frequency of each component in the returned list. So the reported string could be like "Dependencies check failed for: mod_workshop (2x), enrol_ldap". The "2x" would mean that there are two unsatisfied dependencies (one against the core and the other against some other plugin, for example). missing @param fixed you are absolutely right, very well spotted! I fixed the style according the recommendation we have in the Coding style I have also rebased the branch against the recent origin/master.
          Hide
          Aparup Banerjee added a comment -

          Thanks, this has landed in master. service checks are are due soon.

          Show
          Aparup Banerjee added a comment - Thanks, this has landed in master. service checks are are due soon.
          Hide
          Rajesh Taneja added a comment -

          Works as expected
          Thanks for fixing this, David.

          Show
          Rajesh Taneja added a comment - Works as expected Thanks for fixing this, David.
          Hide
          Dan Poltawski added a comment -

          Congratulations!

          Your work has made into the latest Moodle release!

          You are only authorised to celebrate after testing 15 Moodle 2.3 QA tests, thanks!

          Show
          Dan Poltawski added a comment - Congratulations! Your work has made into the latest Moodle release! You are only authorised to celebrate after testing 15 Moodle 2.3 QA tests, thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: