Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.9.5, 2.3
    • Fix Version/s: 2.3
    • Component/s: Administration
    • Labels:
    • Testing Instructions:
      Hide

      Testing difficulty: HARD. This depends on a response from the server. The best way to test all possible cases is to deploy a fake.php script (attached) and set $CFG->alternativeupdateproviderurl to point to it. To test cron, you may want to execute

      delete from mdl_config_plugins where plugin='core_plugin';
      

      to make sure that the first cron execution will actually execute the fetch. Also consider setting $CFG->divertallemailsto so that you catch all emailed notifications.

      Show
      Testing difficulty: HARD. This depends on a response from the server. The best way to test all possible cases is to deploy a fake.php script (attached) and set $CFG->alternativeupdateproviderurl to point to it. To test cron, you may want to execute delete from mdl_config_plugins where plugin='core_plugin'; to make sure that the first cron execution will actually execute the fetch. Also consider setting $CFG->divertallemailsto so that you catch all emailed notifications.
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-20438-update-notification
    • Rank:
      2480

      Description

      This issue tracks the progress on the Available update notifications project. See http://docs.moodle.org/dev/Available_update_notifications for more info.

      1. fake.php
        5 kB
        David Mudrak
      2. smurf.xml
        26 kB
        Dan Poltawski
      1. screenshot-plugins-overview.png
        114 kB

        Issue Links

          Activity

          Hide
          David Mudrak added a comment -

          I am re-using the original issue to track this project. The link to Petr's proposal from October 2009 is in the wiki. Adding some watchers who might be interested in this.

          I started work on the spec document for this at http://docs.moodle.org/dev/Available_update_notifications

          Before we can continue, we need to decide the two design aspects mentioned in the spec: (1) where the primary information is/are stored and (2) how it gets to Moodle sites.

          Show
          David Mudrak added a comment - I am re-using the original issue to track this project. The link to Petr's proposal from October 2009 is in the wiki. Adding some watchers who might be interested in this. I started work on the spec document for this at http://docs.moodle.org/dev/Available_update_notifications Before we can continue, we need to decide the two design aspects mentioned in the spec: (1) where the primary information is/are stored and (2) how it gets to Moodle sites.
          Hide
          David Mudrak added a comment -

          Ping. I was actually expecting sort of flame war here. I just did not want to loose time by designing something in details just to find integrators not happy with the product at the end.

          I must admit my +1 goes for the heavy-weight REST server provided by local_plugins and a REST client inbuilt in all Moodle 2.3 sites (with all the external lib and friends). If nobody objects, I will continue designing the thing that way.

          Show
          David Mudrak added a comment - Ping. I was actually expecting sort of flame war here. I just did not want to loose time by designing something in details just to find integrators not happy with the product at the end. I must admit my +1 goes for the heavy-weight REST server provided by local_plugins and a REST client inbuilt in all Moodle 2.3 sites (with all the external lib and friends). If nobody objects, I will continue designing the thing that way.
          Hide
          Martin Dougiamas added a comment - - edited

          Thanks David!

          Michael D mentioned an idea which I really like about the potential for us to collect statistics about plugin use via the call to us - he's put it on the spec already (probably such things should be discussed here and be put there by David in future though). It's really good for us to know what plugins are being used out there so we can make better decisions about core vs plugins etc

          This affects the design in that we should not be thinking about sending the whole dataset and doing all the processing on the client.

          Joining the update notifications to registration is also something we can do to help our overall statistics, in fact, it would make sense if MOOCH in fact was the thing that handled everything (rather than directly querying moodle.org, we could make periodic copies of the plugins data, and moodle data, into some structure in MOOCH).

          My thinking here is that we already have to maintain MOOCH and it already has login-less web services set up, so why not take advantage of that?

          Show
          Martin Dougiamas added a comment - - edited Thanks David! Michael D mentioned an idea which I really like about the potential for us to collect statistics about plugin use via the call to us - he's put it on the spec already (probably such things should be discussed here and be put there by David in future though). It's really good for us to know what plugins are being used out there so we can make better decisions about core vs plugins etc This affects the design in that we should not be thinking about sending the whole dataset and doing all the processing on the client. Joining the update notifications to registration is also something we can do to help our overall statistics, in fact, it would make sense if MOOCH in fact was the thing that handled everything (rather than directly querying moodle.org, we could make periodic copies of the plugins data, and moodle data, into some structure in MOOCH). My thinking here is that we already have to maintain MOOCH and it already has login-less web services set up, so why not take advantage of that?
          Hide
          Jérôme Mouneyrac added a comment -

          I just talked with Martin to clarify some points about putting this on MOOCH. Here are the current summary:

          • sites must register to MOOCH to see the update notification option
          • as sites are registered, they have a ws token, they can call the Moodle web service normally.
          • we need to created a new local plugin to implement this
          • we need to hack somehow the hub plugin. The hub plugin could detect the new installed local plugin and then add the new local plugin ws functions to the hub pre-build service (this same service that is linked to all site tokens)
          • or we don't create a local plugin but implement all this update notification in the hub server plugin (i.e. all hubs will support this update system)
          • Of course all this is only in the idea we (will) need more than a simple public script like a worpdress RSS feed
          Show
          Jérôme Mouneyrac added a comment - I just talked with Martin to clarify some points about putting this on MOOCH. Here are the current summary: sites must register to MOOCH to see the update notification option as sites are registered, they have a ws token, they can call the Moodle web service normally. we need to created a new local plugin to implement this we need to hack somehow the hub plugin. The hub plugin could detect the new installed local plugin and then add the new local plugin ws functions to the hub pre-build service (this same service that is linked to all site tokens) or we don't create a local plugin but implement all this update notification in the hub server plugin (i.e. all hubs will support this update system) Of course all this is only in the idea we (will) need more than a simple public script like a worpdress RSS feed
          Hide
          David Mudrak added a comment -

          This was discussed today during the meeting and these were the points where developers did not reach consensus yet:

          • Requiring MOOCH registration. On contrary to the proposal above, David and Eloy are not happy with interfering hubs into this.
          • Providing the list of currently installed plugins during the request. David and Tim objected that there is no need to send the list of currently installed plugins. Just the version information is enough and the response would contain the list of all available plugins for the given version. The processing of the list would happen at the client side (eg it would display just available updates or the list of available plugins to download in the future - without the need to change API of the service!). Eloy and David think that sending the info about installed plugins should happen during the registration only.

          Note that we don't have much time for 2.3 development. On the other hand, we don't want to change the API of the service in 6 months again. From my point of view, this is the conceptual question to answer: Should the access to the information about available plugins be public/open (for all clients including non-Moodle apps), or should it be only for Moodle sites, or should it be only for Moodle sites registered at a hub, or should it be only for those registered at MOOCH? Answering this determines whether we can or can't send the list of currently installed plugins.

          With regards to my +1 towards heavy-weight REST WS above - my reasoning was pretty selfish. I know we will need this WS for yet another Plugins related project (AMOS/Plugins integration) so this was a nice opportunity to learn something about WS. However, if we decide for the open one-way communication channel (ie Plugins just serving the info about all available plugins without collecting stats), we can make it as a light-weight REST service without auth and friends.

          Writing this, I can see sort of compromise solution. Let us imagine API that works both with and without additional query params. So the plain query (GET available plugins) would just return all plugins registered in the Plugins. However, the caller might specify target Moodle version/build/branch and then only filtered plugins would be returned. Similarly, the caller might specify the frankenstyle names of plugins it is interested in and the filtering would happen again.

          Into Moodle 2.3, we would implement the client that queries the Plugins and provides the target version and build and currently installed plugins. So we could get some rough estimations of plugins usage world-wide even without matching them with registered sites.

          Also note that the API could be used by alternative clients, too. For example a simple JavaScript client that asks for all available versions/branches of mod_book and displays them on skodak's blog page :-p

          p.s. nobody came with a solution of what the source of Moodle version itself will be. That is, where will we read the information about available Moodle core update.

          Show
          David Mudrak added a comment - This was discussed today during the meeting and these were the points where developers did not reach consensus yet: Requiring MOOCH registration. On contrary to the proposal above, David and Eloy are not happy with interfering hubs into this. Providing the list of currently installed plugins during the request. David and Tim objected that there is no need to send the list of currently installed plugins. Just the version information is enough and the response would contain the list of all available plugins for the given version. The processing of the list would happen at the client side (eg it would display just available updates or the list of available plugins to download in the future - without the need to change API of the service!). Eloy and David think that sending the info about installed plugins should happen during the registration only. Note that we don't have much time for 2.3 development. On the other hand, we don't want to change the API of the service in 6 months again. From my point of view, this is the conceptual question to answer: Should the access to the information about available plugins be public/open (for all clients including non-Moodle apps), or should it be only for Moodle sites, or should it be only for Moodle sites registered at a hub, or should it be only for those registered at MOOCH? Answering this determines whether we can or can't send the list of currently installed plugins. With regards to my +1 towards heavy-weight REST WS above - my reasoning was pretty selfish. I know we will need this WS for yet another Plugins related project (AMOS/Plugins integration) so this was a nice opportunity to learn something about WS. However, if we decide for the open one-way communication channel (ie Plugins just serving the info about all available plugins without collecting stats), we can make it as a light-weight REST service without auth and friends. Writing this, I can see sort of compromise solution. Let us imagine API that works both with and without additional query params. So the plain query (GET available plugins) would just return all plugins registered in the Plugins. However, the caller might specify target Moodle version/build/branch and then only filtered plugins would be returned. Similarly, the caller might specify the frankenstyle names of plugins it is interested in and the filtering would happen again. Into Moodle 2.3, we would implement the client that queries the Plugins and provides the target version and build and currently installed plugins. So we could get some rough estimations of plugins usage world-wide even without matching them with registered sites. Also note that the API could be used by alternative clients, too. For example a simple JavaScript client that asks for all available versions/branches of mod_book and displays them on skodak's blog page :-p p.s. nobody came with a solution of what the source of Moodle version itself will be. That is, where will we read the information about available Moodle core update.
          Hide
          Martin Dougiamas added a comment - - edited

          OK,so after some thought, even though I did want to use it to encourage registrations, it's not super important.

          OK, so how about this (refactoring what I think you wrote above):

          1) Super-lightweight REST script at http://moodle.org/plugins/rest.php that can accept optional parameters for Moodle version, Moodle SiteID and plugin names. When it gets those parameters it stores them for our records, and returns just the subset of data that is required. When it doesn't get those parameters it can return everything. And there can be other parameters for different filters (eg by category, reviews etc)

          2) A super-lightweight REST script on http://download.moodle.org/rest.php that returns the latest Moodle information. This data comes from an XML file or PHP array with all releases. We can make this the main source of info, and generate http://download.moodle.org from it as well.

          3) Moodle 2.3 has two new features. "Check for updates now", and "Check for updates weekly and inform me". Both of these will send the version/siteid/pluginname parameters, and check both of the locations above (1 & 2). The first will link straight to an admintool page (in their Moodle site) that does the check and shows the returned plugins+info and contains links to downloads. The second will do the check in the background via cron and if the result is non-null, then will send a message containing a link to the above admintool.

          Show
          Martin Dougiamas added a comment - - edited OK,so after some thought, even though I did want to use it to encourage registrations, it's not super important. OK, so how about this (refactoring what I think you wrote above): 1) Super-lightweight REST script at http://moodle.org/plugins/rest.php that can accept optional parameters for Moodle version, Moodle SiteID and plugin names. When it gets those parameters it stores them for our records, and returns just the subset of data that is required. When it doesn't get those parameters it can return everything. And there can be other parameters for different filters (eg by category, reviews etc) 2) A super-lightweight REST script on http://download.moodle.org/rest.php that returns the latest Moodle information. This data comes from an XML file or PHP array with all releases. We can make this the main source of info, and generate http://download.moodle.org from it as well. 3) Moodle 2.3 has two new features. "Check for updates now", and "Check for updates weekly and inform me". Both of these will send the version/siteid/pluginname parameters, and check both of the locations above (1 & 2). The first will link straight to an admintool page (in their Moodle site) that does the check and shows the returned plugins+info and contains links to downloads. The second will do the check in the background via cron and if the result is non-null, then will send a message containing a link to the above admintool.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          It might be good to preserve the word REST to be overused: moodle.org/plugins/lastversion.php?moodleversion=XXX&pluginname=ZZZZ&siteid=YYYY

          Show
          Jérôme Mouneyrac added a comment - - edited It might be good to preserve the word REST to be overused: moodle.org/plugins/lastversion.php?moodleversion=XXX&pluginname=ZZZZ&siteid=YYYY
          Hide
          David Mudrak added a comment -

          Attaching a screenshot of the Plugins overview page in my 2.3dev. Note the information about the available update.

          Show
          David Mudrak added a comment - Attaching a screenshot of the Plugins overview page in my 2.3dev. Note the information about the available update.
          Hide
          David Mudrak added a comment -

          Publishing the patch for the eventual peer-review of this work-in-progress.

          Show
          David Mudrak added a comment - Publishing the patch for the eventual peer-review of this work-in-progress.
          Hide
          David Mudrak added a comment -

          Submitting to Petr for peer-review. Going to improve the dev docs yet.

          Show
          David Mudrak added a comment - Submitting to Petr for peer-review. Going to improve the dev docs yet.
          Hide
          David Mudrak added a comment -

          Attaching fake.php that simulates the server response (usefull for testing and debugging).

          Show
          David Mudrak added a comment - Attaching fake.php that simulates the server response (usefull for testing and debugging).
          Hide
          Petr Škoda added a comment -

          "if ($fetchupdates) { require_sesskey(); ...) on admin/index.php page might not work without sessions, but the session if guaranteed to be working after main upgrade/installation only

          the rest looks ok to me, +1

          Show
          Petr Škoda added a comment - "if ($fetchupdates) { require_sesskey(); ...) on admin/index.php page might not work without sessions, but the session if guaranteed to be working after main upgrade/installation only the rest looks ok to me, +1
          Hide
          David Mudrak added a comment -

          Thanks Petr for the peer-review. I just fixed the places he spotted, rebased against the latest origin/master and am vectoring this for the final landing.

          Show
          David Mudrak added a comment - Thanks Petr for the peer-review. I just fixed the places he spotted, rebased against the latest origin/master and am vectoring this for the final landing.
          Hide
          Dan Poltawski added a comment -

          Assinging this to Eloy to integrate as I believe he volunteered to do that

          Show
          Dan Poltawski added a comment - Assinging this to Eloy to integrate as I believe he volunteered to do that
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Did I?

          Show
          Eloy Lafuente (stronk7) added a comment - Did I?
          Hide
          Dan Poltawski added a comment -

          Ah, maybe I read too much into David's comments

          http://moodle.org/local/chatlogs/index.php?conversationid=9843#c341528

          Show
          Dan Poltawski added a comment - Ah, maybe I read too much into David's comments http://moodle.org/local/chatlogs/index.php?conversationid=9843#c341528
          Hide
          David Mudrak added a comment -

          OK, could be my misinterpretation of this private chat:

          (12:48:04) mudrd8mz@jabber.cz/glum: BTW would you like to integrate my patch or shall I ask Petr or shall I leave it to someone else to decide?
          (12:48:31) Eloy: I prefer to just integrate it after anyone else peer review.
          (12:48:57) mudrd8mz@jabber.cz/glum: ok, I'll aks someone for peer-review
          (12:49:03) Eloy:

          So it's not that Eloy actually volunteered himself, I just did not give him much of choice The idea behind Eloy integrating this was that he was already involved in the whole machinery (especially the server side). However, it's completely up to you Eldars to decide.

          Show
          David Mudrak added a comment - OK, could be my misinterpretation of this private chat: (12:48:04) mudrd8mz@jabber.cz/glum: BTW would you like to integrate my patch or shall I ask Petr or shall I leave it to someone else to decide? (12:48:31) Eloy: I prefer to just integrate it after anyone else peer review. (12:48:57) mudrd8mz@jabber.cz/glum: ok, I'll aks someone for peer-review (12:49:03) Eloy: So it's not that Eloy actually volunteered himself, I just did not give him much of choice The idea behind Eloy integrating this was that he was already involved in the whole machinery (especially the server side). However, it's completely up to you Eldars to decide.
          Hide
          Dan Poltawski added a comment -

          Taking this from Eloy

          Show
          Dan Poltawski added a comment - Taking this from Eloy
          Hide
          Dan Poltawski added a comment -

          First review and I can't really find anything to comment on.

          One thing I wonder out loud about is if it was worth the 'complexity' of introducing the maturity options to this update checks. From my point of view it seems like this is a feature which should be focused only on stable releases since our goal with this is surely to keep moodle admins on stable and secure releases.

          My big concern with this change is that we ensure this is well tested before release, the last thing we want is to be 'falsely claiming' everything is up to date.

          This is tricky to test since it requires the server components and settings and installed versions etc all to work together. There are no contrib plugins currently reporting support for 2.3 so we also can't test this at the moment. Discussed this a bit with david and perhaps we need to add testing of the update checker as a sort of QA test and explicit areas of testing before the release.

          Show
          Dan Poltawski added a comment - First review and I can't really find anything to comment on. One thing I wonder out loud about is if it was worth the 'complexity' of introducing the maturity options to this update checks. From my point of view it seems like this is a feature which should be focused only on stable releases since our goal with this is surely to keep moodle admins on stable and secure releases. My big concern with this change is that we ensure this is well tested before release, the last thing we want is to be 'falsely claiming' everything is up to date. This is tricky to test since it requires the server components and settings and installed versions etc all to work together. There are no contrib plugins currently reporting support for 2.3 so we also can't test this at the moment. Discussed this a bit with david and perhaps we need to add testing of the update checker as a sort of QA test and explicit areas of testing before the release.
          Hide
          David Mudrak added a comment -

          Thanks Dan for the review.

          if it was worth the 'complexity' of introducing the maturity options to this update checks

          I fully understand this note. I was considering this aspect, too. At the end, "when in doubt, make it an option" approach was followed with default values set to what 99% of sites probably want. Still I believe there are many "more curious" settings in our admin tree (not that it is an excuse to introduce new ones).

          And yes, we probably should discuss with Marina if it is possible to open 2.3 as the supported version in the Plugins. Maybe it can't be done before the actual release because the version number is used there, too...

          Show
          David Mudrak added a comment - Thanks Dan for the review. if it was worth the 'complexity' of introducing the maturity options to this update checks I fully understand this note. I was considering this aspect, too. At the end, "when in doubt, make it an option" approach was followed with default values set to what 99% of sites probably want. Still I believe there are many "more curious" settings in our admin tree (not that it is an excuse to introduce new ones). And yes, we probably should discuss with Marina if it is possible to open 2.3 as the supported version in the Plugins. Maybe it can't be done before the actual release because the version number is used there, too...
          Hide
          Dan Poltawski added a comment -

          Hi David,

          Please see the 'smurf report' on your changes

          Show
          Dan Poltawski added a comment - Hi David, Please see the 'smurf report' on your changes
          Hide
          David Mudrak added a comment -

          Hi Dan. Thanks for generating the report. Please note that some of the reported problems are not valid IMHO. E.g. I have not found anything in the Coding style that would forbid me to use print_r(). And the "Useless method overriding detected" is actually completely wrong as the overriding is indeed required in the reported case.

          As I expressed several times already, there are rules that I violate intentionally as I do not agree with them, mostly for aesthetic reasons (blank line after <?php) or because I have not been told a reasonably good reason for them yet (like the @author and @licence in classes where implicit inheritance works well). WRT to the long lines, I always try to choose between the two evils the one that IMHO serves the code readibility more.

          Anyway, the report provides really good feedback on my code syntax. Thanks for it. I'm glad seeing that no review detected serious mistakes in the code so far.

          Show
          David Mudrak added a comment - Hi Dan. Thanks for generating the report. Please note that some of the reported problems are not valid IMHO. E.g. I have not found anything in the Coding style that would forbid me to use print_r(). And the "Useless method overriding detected" is actually completely wrong as the overriding is indeed required in the reported case. As I expressed several times already, there are rules that I violate intentionally as I do not agree with them, mostly for aesthetic reasons (blank line after <?php) or because I have not been told a reasonably good reason for them yet (like the @author and @licence in classes where implicit inheritance works well). WRT to the long lines, I always try to choose between the two evils the one that IMHO serves the code readibility more. Anyway, the report provides really good feedback on my code syntax. Thanks for it. I'm glad seeing that no review detected serious mistakes in the code so far.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          David, I think the 3 cases you expose above are pretty spot:

          1) the extra blank line after <?php. It's currently a warning only. Just the recommendation was set to "eat it".
          2) the inheritance files->classes (and classes->classes). I think the vast majority of systems I tried performed them auto-magically. I think we could propose that to become also one recommendation (right now it's an error if I'm not wrong).
          3) the long line limits were moved some weeks ago so we have up to 180 chars before getting an error. It's warning starting in 132 or so.

          Then there are the print_r() and "overrides" ones that I had no idea about them at all. Those need to be revisited for sure, feel free to open an issue.

          So I think more or less all us (integrators) agree those are not critical. But others like:

          A) missing phpdocs completely in classes and/or methods/functions.
          B) indenting
          C) wrong spacing/lining after "if/for/try/catch..."

          certainly seem to have their point and, progressively, will become 100% mandatory. And current master seems to be a good target to start observing a lot of them.

          Just my thoughts... we need to discuss this... my personal vote goes to ignore the 1...n ones but clearly enforce the A-C ones...

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - David, I think the 3 cases you expose above are pretty spot: 1) the extra blank line after <?php. It's currently a warning only. Just the recommendation was set to "eat it". 2) the inheritance files->classes (and classes->classes). I think the vast majority of systems I tried performed them auto-magically. I think we could propose that to become also one recommendation (right now it's an error if I'm not wrong). 3) the long line limits were moved some weeks ago so we have up to 180 chars before getting an error. It's warning starting in 132 or so. Then there are the print_r() and "overrides" ones that I had no idea about them at all. Those need to be revisited for sure, feel free to open an issue. So I think more or less all us (integrators) agree those are not critical. But others like: A) missing phpdocs completely in classes and/or methods/functions. B) indenting C) wrong spacing/lining after "if/for/try/catch..." certainly seem to have their point and, progressively, will become 100% mandatory. And current master seems to be a good target to start observing a lot of them. Just my thoughts... we need to discuss this... my personal vote goes to ignore the 1...n ones but clearly enforce the A-C ones... Ciao
          Hide
          Dan Poltawski added a comment - - edited

          The only 'major' issues in the codechecker left where two trivial whitespace problems so I commited a patch to fix and integrated after another review.

          Show
          Dan Poltawski added a comment - - edited The only 'major' issues in the codechecker left where two trivial whitespace problems so I commited a patch to fix and integrated after another review.
          Hide
          Dan Poltawski added a comment -

          Oh no, we are breaking the CLI installer:
          Default exception handler: Error reading from database Debug: Table 'ci_installed_680_0.cii_config_plugins' doesn't exist
          SELECT name,value FROM cii_config_plugins WHERE plugin = ?
          [array (
          0 => 'core_plugin',
          )]

          • line 410 of /lib/dml/moodle_database.php: dml_read_exception thrown
          • line 872 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database->query_end()
          • line 1165 of /lib/dml/moodle_database.php: call to mysqli_native_moodle_database->get_records_sql()
          • line 1114 of /lib/dml/moodle_database.php: call to moodle_database->get_records_select()
          • line 1205 of /lib/dml/moodle_database.php: call to moodle_database->get_records()
          • line 1304 of /lib/moodlelib.php: call to moodle_database->get_records_menu()
          • line 832 of /lib/pluginlib.php: call to get_config()
          • line 685 of /lib/pluginlib.php: call to available_update_checker->restore_response()
          • line 1664 of /lib/pluginlib.php: call to available_update_checker->get_update_info()
          • line 125 of /lib/pluginlib.php: call to plugininfo_base->check_available_updates()
          • line 285 of /lib/pluginlib.php: call to plugin_manager->get_plugins()
          • line 680 of /admin/cli/install.php: call to plugin_manager->all_plugins_ok()
          Show
          Dan Poltawski added a comment - Oh no, we are breaking the CLI installer: Default exception handler: Error reading from database Debug: Table 'ci_installed_680_0.cii_config_plugins' doesn't exist SELECT name,value FROM cii_config_plugins WHERE plugin = ? [array ( 0 => 'core_plugin', )] line 410 of /lib/dml/moodle_database.php: dml_read_exception thrown line 872 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database->query_end() line 1165 of /lib/dml/moodle_database.php: call to mysqli_native_moodle_database->get_records_sql() line 1114 of /lib/dml/moodle_database.php: call to moodle_database->get_records_select() line 1205 of /lib/dml/moodle_database.php: call to moodle_database->get_records() line 1304 of /lib/moodlelib.php: call to moodle_database->get_records_menu() line 832 of /lib/pluginlib.php: call to get_config() line 685 of /lib/pluginlib.php: call to available_update_checker->restore_response() line 1664 of /lib/pluginlib.php: call to available_update_checker->get_update_info() line 125 of /lib/pluginlib.php: call to plugininfo_base->check_available_updates() line 285 of /lib/pluginlib.php: call to plugin_manager->get_plugins() line 680 of /admin/cli/install.php: call to plugin_manager->all_plugins_ok()
          Hide
          Dan Poltawski added a comment -

          Hi David,

          Looks like we have a discrepancy with the installers CLI installer is calling all_plugins_ok() where are web installer doesn't seem to.

          I came up with a hacky solution for this https://gist.github.com/a7461a8ed43980b0ae5e but really the fact we are doing two different things with the two installers seems more serious problem..

          So hoping to have some insight on your thoughts

          Show
          Dan Poltawski added a comment - Hi David, Looks like we have a discrepancy with the installers CLI installer is calling all_plugins_ok() where are web installer doesn't seem to. I came up with a hacky solution for this https://gist.github.com/a7461a8ed43980b0ae5e but really the fact we are doing two different things with the two installers seems more serious problem.. So hoping to have some insight on your thoughts
          Hide
          Dan Poltawski added a comment -

          Unfortunately David is unwell today, get well soon David! So progressing this issue by creating a new issue: MDL-32329

          And in the meantime committing my interim patch.

          Show
          Dan Poltawski added a comment - Unfortunately David is unwell today, get well soon David! So progressing this issue by creating a new issue: MDL-32329 And in the meantime committing my interim patch.
          Hide
          Dan Poltawski added a comment -

          Ok temporary fix has been deployed, with the long term solution to be addressed in MDL-32329

          Show
          Dan Poltawski added a comment - Ok temporary fix has been deployed, with the long term solution to be addressed in MDL-32329
          Hide
          Dan Poltawski added a comment -

          OK I have tested a variety of scenarios and I have found a few issues while testing which are linked here.

          These can be addressed separately so passing this test, thanks!

          Show
          Dan Poltawski added a comment - OK I have tested a variety of scenarios and I have found a few issues while testing which are linked here. These can be addressed separately so passing this test, thanks!
          Hide
          David Mudrak added a comment -

          Hi guys. Firstly thanks a lot for the responsible testing of this. I am sorry I was not able to help you much with the fixes, being sick. I definitely agree with Eloy's wise words on the code checker. I'll follow the report suggestions. Should I prepare a patch in a separate issue? Or did Dan fix them?

          WRT the installer issue, is Dan's solution really that hacky? The alternative could be to subclass the plugin_manager to something like bootstrap_plugin_manager but for now, there's nothing wrong with Dan's patch.

          Show
          David Mudrak added a comment - Hi guys. Firstly thanks a lot for the responsible testing of this. I am sorry I was not able to help you much with the fixes, being sick. I definitely agree with Eloy's wise words on the code checker. I'll follow the report suggestions. Should I prepare a patch in a separate issue? Or did Dan fix them? WRT the installer issue, is Dan's solution really that hacky? The alternative could be to subclass the plugin_manager to something like bootstrap_plugin_manager but for now, there's nothing wrong with Dan's patch.
          Hide
          Dan Poltawski added a comment -

          David: with regard to codechecker, I fixed the two "wrong spacing/lining after "if/for/try/catch..." issues. The only remaining 'major' issues are missing phpdocs on the test classes.

          With regard to the 'hacky' solution I only use that term because I didn't know if it was the right solution Otherwise thats fine - although as noted in MDL-32329 we really need to ensure the two installers follow same path!

          Show
          Dan Poltawski added a comment - David: with regard to codechecker, I fixed the two "wrong spacing/lining after "if/for/try/catch..." issues. The only remaining 'major' issues are missing phpdocs on the test classes. With regard to the 'hacky' solution I only use that term because I didn't know if it was the right solution Otherwise thats fine - although as noted in MDL-32329 we really need to ensure the two installers follow same path!
          Hide
          Aparup Banerjee added a comment -

          The code here has been spread to upstream moodle repositories and mirrors for anyone to use .

          Closing, have a good weekend!

          Show
          Aparup Banerjee added a comment - The code here has been spread to upstream moodle repositories and mirrors for anyone to use . Closing, have a good weekend!
          Hide
          Tim Barker added a comment -

          Added 4 new QA tests: MDLQA-1549, MDLQA-1550, MDLQA-1551 and MDLQA-1552. I've removed the QA test required flag from this tracker item. If there is any extra coverage anyone would like adding please comment on this issue and re-add the QA test required flag and I'll pick it up.

          Show
          Tim Barker added a comment - Added 4 new QA tests: MDLQA-1549, MDLQA-1550, MDLQA-1551 and MDLQA-1552. I've removed the QA test required flag from this tracker item. If there is any extra coverage anyone would like adding please comment on this issue and re-add the QA test required flag and I'll pick it up.
          Hide
          Helen Foster added a comment -

          Available update notifications documentation now available:

          http://docs.moodle.org/23/en/Notifications (linked to from the update notifications page in moodle)

          also mentioned in http://docs.moodle.org/23/en/Installing_plugins#Plugins_overview and http://docs.moodle.org/23/en/Messaging_settings#My_profile_settings

          Please shout if anything needs adding or amending.

          Show
          Helen Foster added a comment - Available update notifications documentation now available: http://docs.moodle.org/23/en/Notifications (linked to from the update notifications page in moodle) also mentioned in http://docs.moodle.org/23/en/Installing_plugins#Plugins_overview and http://docs.moodle.org/23/en/Messaging_settings#My_profile_settings Please shout if anything needs adding or amending.
          Hide
          Helen Foster added a comment -

          Re-adding the docs_required label to remind me to add documentation about 'Notify about new builds' setting.

          Show
          Helen Foster added a comment - Re-adding the docs_required label to remind me to add documentation about 'Notify about new builds' setting.
          Hide
          Helen Foster added a comment -

          Additional update notification settings now mentioned in http://docs.moodle.org/23/en/Notifications#Update_notifications

          Show
          Helen Foster added a comment - Additional update notification settings now mentioned in http://docs.moodle.org/23/en/Notifications#Update_notifications

            People

            • Votes:
              3 Vote for this issue
              Watchers:
              13 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: