Moodle
  1. Moodle
  2. MDL-34962

core_webservice_get_site_info should include the Moodle release number in returned values

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.1
    • Fix Version/s: 2.4
    • Component/s: Web Services
    • Labels:
    • Rank:
      43533

      Description

      A web service client will generally be designed and tested to run against a set of web service functions that are grouped together to make up a web service (e.g MOODLE_OFFICIAL_MOBILE_SERVICE or LIGHTWORK).

      These web services may change between different versions of Moodle and when a client connects it is important for it to know that it is connecting to a Moodle version with which it is guaranteed to work correctly (i.e the client development team has tested and released the client for that version of Moodle).

      In order to determine this, the very first web service function that all clients should call will be core_webservice_get_site_info. This function needs to be modified to return the release number of the Moodle version. It currently only returns the version (which it does multiple times for each moodle web service function). A web service client could use the release number as follows:

      • It would compare the returned release number (e.g 2.2.1) against the release number/numbers for which the client has been designed to run
      • If the release numbers do not match, it could use this information to:
        • Display a message to the user telling them that they are connecting to an old version of Moodle which is not supported by the client
        • Automatically trigger an update of the client software to support the version to which they are connecting
        • Display a message prompting the user to update their client software

        Issue Links

          Activity

          Hide
          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
          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
          Jérôme Mouneyrac added a comment -

          Hi Paul,
          as you mention core_webservice_get_site_info returns a field called 'version' => "The version number of moodle site/local plugin linked to the function".

          Clients should implement behaviour against these function versions. If the user can access some Moodle core web service functions, then the client will automatically knows the Moodle version. Otherwise your client will only know the plugin version which should be enough as the user can only access these plugin functions.

          I think the returned function versions let you implement all points you wrote. You can even enable/disable your client features with these informations.

          Show
          Jérôme Mouneyrac added a comment - Hi Paul, as you mention core_webservice_get_site_info returns a field called 'version' => "The version number of moodle site/local plugin linked to the function". Clients should implement behaviour against these function versions. If the user can access some Moodle core web service functions, then the client will automatically knows the Moodle version. Otherwise your client will only know the plugin version which should be enough as the user can only access these plugin functions. I think the returned function versions let you implement all points you wrote. You can even enable/disable your client features with these informations.
          Hide
          Paul Charsley added a comment -

          Hi Jerome,

          The version numbers returned are not user friendly names and most clients will want to be able to display a user friendly release number in the same way that the Moodle interface does. In addition, the Moodle release number represents important site information that should be included in the return values.

          Cheers, Paul

          Show
          Paul Charsley added a comment - Hi Jerome, The version numbers returned are not user friendly names and most clients will want to be able to display a user friendly release number in the same way that the Moodle interface does. In addition, the Moodle release number represents important site information that should be included in the return values. Cheers, Paul
          Hide
          Jérôme Mouneyrac added a comment -

          No problem, provide a patch. Don't forget to do an admin check so we don't return it if the user is not an admin.

          Show
          Jérôme Mouneyrac added a comment - No problem, provide a patch. Don't forget to do an admin check so we don't return it if the user is not an admin.
          Hide
          Paul Charsley added a comment -

          Hi Jerome,

          The user calling this web service function will not be an admin user. This web service function currently returns the Moodle version to a non admin user, why can it not also return the human friendly Moodle version name?

          Cheers, Paul

          Show
          Paul Charsley added a comment - Hi Jerome, The user calling this web service function will not be an admin user. This web service function currently returns the Moodle version to a non admin user, why can it not also return the human friendly Moodle version name? Cheers, Paul
          Hide
          Jérôme Mouneyrac added a comment -

          My last comment was to not return a human friendly version to make it difficult for a user to know the version number. And actually after thinking about it I feel like it may be wrong to match function with Moodle/Plugin version number for security reason.

          Returning Moodle human readable version for any user, it's encouraging the client developer to display the version number to the user, when really the non admin user doesn't need to know much more than "The Moodle version is not compatible/too old/...". The user does not need to know the exact version number if the user is not an admin.

          Show
          Jérôme Mouneyrac added a comment - My last comment was to not return a human friendly version to make it difficult for a user to know the version number. And actually after thinking about it I feel like it may be wrong to match function with Moodle/Plugin version number for security reason. Returning Moodle human readable version for any user, it's encouraging the client developer to display the version number to the user, when really the non admin user doesn't need to know much more than "The Moodle version is not compatible/too old/...". The user does not need to know the exact version number if the user is not an admin.
          Hide
          Paul Charsley added a comment -

          Hi Jerome,
          I do not believe that it represents a serious security risk to return the Moodle release number in a web service.
          See also the comments in MDL-26852 and bear in mind that I am only proposing to return the release number for logged in users who have specifically been granted a web service session token by the system administrator.
          It is important to remember that Web service client applications will be built and tested against specific Moodle releases and therefore must be able to access the Moodle release number of the site they are connecting to. What's more, most web client applications are likely to to be designed and built for the use of teachers and students who won't have admin rights.
          Cheers, Paul

          Show
          Paul Charsley added a comment - Hi Jerome, I do not believe that it represents a serious security risk to return the Moodle release number in a web service. See also the comments in MDL-26852 and bear in mind that I am only proposing to return the release number for logged in users who have specifically been granted a web service session token by the system administrator. It is important to remember that Web service client applications will be built and tested against specific Moodle releases and therefore must be able to access the Moodle release number of the site they are connecting to. What's more, most web client applications are likely to to be designed and built for the use of teachers and students who won't have admin rights. Cheers, Paul
          Hide
          Jérôme Mouneyrac added a comment -

          Ah good catch. Ok in this case we should send human readable versions in an additional field.

          Show
          Jérôme Mouneyrac added a comment - Ah good catch. Ok in this case we should send human readable versions in an additional field.
          Hide
          Paul Charsley added a comment -

          Ready for peer review. I have also added a unit test.

          Show
          Paul Charsley added a comment - Ready for peer review. I have also added a unit test.
          Hide
          Jérôme Mouneyrac added a comment -

          ah thanks for all the fixes: define, static, comments, typo, large lines ... and the PHPunit test Sending to integration. All the code looks good to me. I ran the PHPunit test, no issues.

          Show
          Jérôme Mouneyrac added a comment - ah thanks for all the fixes: define, static, comments, typo, large lines ... and the PHPunit test Sending to integration. All the code looks good to me. I ran the PHPunit test, no issues.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Ho, just reviewing this.. some comments:

          1) First of all, not important as far as we can cherry-pick the commit, it seems that your (Paul) master branch is outdated, or you don't keep it in sync with upstream one 100%. That leads to problems trying any merge operation back to upstream. As said, no big problem because we can always cherry-pick, but...

          2) Just thinking about how this is returning information... wouldn't it be more consistent if we:

          a) Always return both the main version and release information.
          b) For components, we return the version (and perhaps also release, note it can exist) for any component nor being moodle/core (yes, what happens with "core", weren't we moving to that from "moodle" ?). Perhaps for BC we should keep also the "moodle" ones, bur clearly deprecated IMO.
          c) For components, we return also the component name, right now it's impossible to get that information, and may be useful.

          3) Also I find the 'The version number of moodle site/local plugin...' definition really inaccurate, IMO it should be 'The version number of the component the function belongs to' or something like that, more if we consider 2c above.

          4) Any change like this does require labeling with api_change/dev_docs_required and some changes to happen both in upgrade.txt files and Moodle Docs?

          For your consideration, I'll keep this open for some hours... ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Ho, just reviewing this.. some comments: 1) First of all, not important as far as we can cherry-pick the commit, it seems that your (Paul) master branch is outdated, or you don't keep it in sync with upstream one 100%. That leads to problems trying any merge operation back to upstream. As said, no big problem because we can always cherry-pick, but... 2) Just thinking about how this is returning information... wouldn't it be more consistent if we: a) Always return both the main version and release information. b) For components, we return the version (and perhaps also release, note it can exist) for any component nor being moodle/core (yes, what happens with "core", weren't we moving to that from "moodle" ?). Perhaps for BC we should keep also the "moodle" ones, bur clearly deprecated IMO. c) For components, we return also the component name, right now it's impossible to get that information, and may be useful. 3) Also I find the 'The version number of moodle site/local plugin...' definition really inaccurate, IMO it should be 'The version number of the component the function belongs to' or something like that, more if we consider 2c above. 4) Any change like this does require labeling with api_change/dev_docs_required and some changes to happen both in upgrade.txt files and Moodle Docs? For your consideration, I'll keep this open for some hours... ciao
          Hide
          Paul Charsley added a comment -

          Hi,

          I've just rebased with the latest changes. Sorry for not doing it earlier.

          I agree with many of your comments. Currently the code returns the same main Moodle version for each of the Core and deprecated Moodle web services. However, I didn't want to modify that since this issue is specifically about just adding the Moodle release number. Perhaps, a separate issue should be created for the additional changes you suggest.

          I will update the Moodle docs with this change.

          Thanks, Paul

          Show
          Paul Charsley added a comment - Hi, I've just rebased with the latest changes. Sorry for not doing it earlier. I agree with many of your comments. Currently the code returns the same main Moodle version for each of the Core and deprecated Moodle web services. However, I didn't want to modify that since this issue is specifically about just adding the Moodle release number. Perhaps, a separate issue should be created for the additional changes you suggest. I will update the Moodle docs with this change. Thanks, Paul
          Hide
          Jérôme Mouneyrac added a comment - - edited

          I'm ok with all comments too. Some additional comments:

          2-b) yes add

          || $function->component == "core"

          and human readable release field.
          2-c) WS function names already contain the component names. However I don't think it causes any issue to return the component name in a separate field. It's as you feel, I will not add it myself as I'm not sure component name is useful to the client (you can imagine two third party plugin with the same name, but it's unlikely that their two functions have same name and same version)
          4) yes it could break clients that crashes when receiving additional info on web service call.

          You can make the change now or create a new issue for these improvements.

          Show
          Jérôme Mouneyrac added a comment - - edited I'm ok with all comments too. Some additional comments: 2-b) yes add || $function->component == "core" and human readable release field. 2-c) WS function names already contain the component names. However I don't think it causes any issue to return the component name in a separate field. It's as you feel, I will not add it myself as I'm not sure component name is useful to the client (you can imagine two third party plugin with the same name, but it's unlikely that their two functions have same name and same version) 4) yes it could break clients that crashes when receiving additional info on web service call. You can make the change now or create a new issue for these improvements.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Oki, I'm going to reopen this to allow you (Paul, Jerome) to fulfill the needed points, no pressure for this to land 1 week later.

          Thanks and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Oki, I'm going to reopen this to allow you (Paul, Jerome) to fulfill the needed points, no pressure for this to land 1 week later. Thanks and ciao
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Paul Charsley added a comment -

          I've updated the code to include the changes discussed.

          Cheers, Paul

          Show
          Paul Charsley added a comment - I've updated the code to include the changes discussed. Cheers, Paul
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks Paul, it looks good. Sending to integration.

          Show
          Jérôme Mouneyrac added a comment - Thanks Paul, it looks good. Sending to integration.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Paul Charsley added a comment -

          Rebase done

          Show
          Paul Charsley added a comment - Rebase done
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (master only), thanks!

          PS: I've added one extra commit tidying the tests a bit (missing parent::setup(), moving some prepare stuff to different levels...)

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (master only), thanks! PS: I've added one extra commit tidying the tests a bit (missing parent::setup(), moving some prepare stuff to different levels...)
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Passing as far as I tried it while tidying the unit tests.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Passing as far as I tried it while tidying the unit tests.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Gutta cavat lapidem, non vi sed saepe cadendo - Ovidio

          This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads).

          Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Gutta cavat lapidem, non vi sed saepe cadendo - Ovidio This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads). Thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: