Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: Future Dev
    • Fix Version/s: 2.4
    • Component/s: Language, Web Services
    • Labels:

      Description

      • return all string from a component
      • return a specific string (name, component)

        Gliffy Diagrams

          Activity

          Hide
          mudrd8mz David Mudrak added a comment -

          Hi Jerome. I have some questions and comments.

          Firstly, the name of the methods. Why do they have core_webservice_ prefix? Surely strings management is not a part of the webservice core subsystem. I would expect that the correct frankenstyle for these methods is a plain core_ (following the conventions defined at http://docs.moodle.org/dev/Web_services_Roadmap#Naming_convention) and thence the name of the methods should have just core_ prefix.

          Second, relying on current_language() is tricky. Are you sure we don't want the WS client let specify the language explicitly. Is there a way how a client can change their current language? What if a client needs to display string in another language?

          Also, I am pretty sure that the included PHPUnit tests may fail pretty easily. What if the WS user has a language set to, say, Czech. And then the one who executes the test has their language set to English. If I understand it correctly, the web service call would return the strings in Czech and they would be compared to the English ones. Or am I missing something?

          Show
          mudrd8mz David Mudrak added a comment - Hi Jerome. I have some questions and comments. Firstly, the name of the methods. Why do they have core_webservice_ prefix? Surely strings management is not a part of the webservice core subsystem. I would expect that the correct frankenstyle for these methods is a plain core_ (following the conventions defined at http://docs.moodle.org/dev/Web_services_Roadmap#Naming_convention ) and thence the name of the methods should have just core_ prefix. Second, relying on current_language() is tricky. Are you sure we don't want the WS client let specify the language explicitly. Is there a way how a client can change their current language? What if a client needs to display string in another language? Also, I am pretty sure that the included PHPUnit tests may fail pretty easily. What if the WS user has a language set to, say, Czech. And then the one who executes the test has their language set to English. If I understand it correctly, the web service call would return the strings in Czech and they would be compared to the English ones. Or am I missing something?
          Hide
          jerome Jérôme Mouneyrac added a comment - - edited

          Thanks David for the quick review. I don't think there is any issue.

          a) I thought to put the functions in lang/externallib.php, but I had a doubt that maybe I should not add anything there. I don't want to add anything to the lib/externallib.php which is a lib and not an external class container. I thought it was ok to have the file in the webservice folder. I checked the subsystem, core_webservice is correct. It's also how it's been used previously.

          b) I thought about changing the language, but I think it's going to be pretty rare that a user want a language different than the one he's using in the interface. We can add it later thought.

          c) for current_language(), it's been used in both test function and external function, and as the user is the same the result will be the same, it should not fail. (PS: the unit tests test the external functions, not the web service functions)

          Show
          jerome Jérôme Mouneyrac added a comment - - edited Thanks David for the quick review. I don't think there is any issue. a) I thought to put the functions in lang/externallib.php, but I had a doubt that maybe I should not add anything there. I don't want to add anything to the lib/externallib.php which is a lib and not an external class container. I thought it was ok to have the file in the webservice folder. I checked the subsystem, core_webservice is correct. It's also how it's been used previously. b) I thought about changing the language, but I think it's going to be pretty rare that a user want a language different than the one he's using in the interface. We can add it later thought. c) for current_language(), it's been used in both test function and external function, and as the user is the same the result will be the same, it should not fail. (PS: the unit tests test the external functions, not the web service functions)
          Hide
          mudrd8mz David Mudrak added a comment -

          > I checked the subsystem, core_webservice is correct

          Well yes - but IMHO it should hold just WS methods related to WS themselves - like the core_webservice_get_site_info() that returns (among others) list of web services. I just conceptually do not see get_string as a part of the core_webservice subsystem.

          > We can add it later though

          Why don't we just add the lang parameter right now and let it default to the current language? We would not need to change the API (or even add new methods) later.

          > as the user is the same the result will be the same

          I'm still not sure but I have to trust you - I have not actually tested it. I just know that current_language() is tricky as there are several factors that may influence it (mostly the context - a course may force a language). I can't remember in what context the WS are called though.

          Show
          mudrd8mz David Mudrak added a comment - > I checked the subsystem, core_webservice is correct Well yes - but IMHO it should hold just WS methods related to WS themselves - like the core_webservice_get_site_info() that returns (among others) list of web services. I just conceptually do not see get_string as a part of the core_webservice subsystem. > We can add it later though Why don't we just add the lang parameter right now and let it default to the current language? We would not need to change the API (or even add new methods) later. > as the user is the same the result will be the same I'm still not sure but I have to trust you - I have not actually tested it. I just know that current_language() is tricky as there are several factors that may influence it (mostly the context - a course may force a language). I can't remember in what context the WS are called though.
          Hide
          jerome Jérôme Mouneyrac added a comment -

          Thanks David, I changed both 1/ and 2/.

          Show
          jerome Jérôme Mouneyrac added a comment - Thanks David, I changed both 1/ and 2/.
          Hide
          nebgor Aparup Banerjee 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
          nebgor Aparup Banerjee 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
          samhemelryk Sam Hemelryk added a comment -

          Hi Jerome,

          Two things I noted while looking at this:

          1. It doesn't look like it is possible to fetch a string that uses just a single $a param. e.g. "Moodle {a}

            ". Is that right or have I missed something? It seems pretty crucial to making sure these methods are useful and complete. (would be worth a unit test as well, just noting)

          2. I think it would also be a good idea to implement the lang option for get_string and get_strings as well. It's a simple option, and would ensure that the webservices shared the same parameters as their counterparts, even if it is not used as regularly it is still of value and helps ensure we don't have to come revisit these core web services too often.

          Really #1 is the important one, I think that 100% needs to be addressed (feel free to correct me if I've misread it).
          #2 is really more of a seeing as its going back anyway lets complete this thing.

          Many thanks
          Sam

          Show
          samhemelryk Sam Hemelryk added a comment - Hi Jerome, Two things I noted while looking at this: It doesn't look like it is possible to fetch a string that uses just a single $a param. e.g. "Moodle {a} ". Is that right or have I missed something? It seems pretty crucial to making sure these methods are useful and complete. (would be worth a unit test as well, just noting) I think it would also be a good idea to implement the lang option for get_string and get_strings as well. It's a simple option, and would ensure that the webservices shared the same parameters as their counterparts, even if it is not used as regularly it is still of value and helps ensure we don't have to come revisit these core web services too often. Really #1 is the important one, I think that 100% needs to be addressed (feel free to correct me if I've misread it). #2 is really more of a seeing as its going back anyway lets complete this thing. Many thanks Sam
          Hide
          cibot CiBoT added a comment -

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

          Show
          cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          jerome Jérôme Mouneyrac added a comment - - edited

          Hi Sam,

          1) get_string and get_strings support the string $a params. get_component_strings() does not support $a params because the client dev does not want the completed strings but the ones with $a->xxx. So the client dev can call the two other get_string(s) functions.

          2) The external function get_component_strings() supports lang. I'll add lang support to the two others functions.

          Thanks for reviewing.

          Show
          jerome Jérôme Mouneyrac added a comment - - edited Hi Sam, 1) get_string and get_strings support the string $a params. get_component_strings() does not support $a params because the client dev does not want the completed strings but the ones with $a->xxx. So the client dev can call the two other get_string(s) functions. 2) The external function get_component_strings() supports lang. I'll add lang support to the two others functions. Thanks for reviewing.
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Hi Jerome,

          Thanks for the reply.

          I was just looking again, I must be missing something sorry.
          I get how a string request for something with {$a->one} {$a->two} would look thanks to the unit tests:

          // From the lang file:
          $string['addservice'] = 'Add a new service: {$a->name} (id: {$a->id})';
          // From the unit tests:
          $returnedstring = core_external::get_string(
              'addservice',
              'webservice',
              array(
                  array(
                      'name' => 'name',
                      'value' => $service->name
                  ),
                  array(
                      'name' => 'id',
                      'value' => $service->id
                  )
              )
          );

          But I don't get how I would structure the request for just a single {$a}

          // From the lang file:
          $string['missingrequiredcapability'] = 'The capability {$a} is required.';
          // Trying to imagine it.
          $returnedstring = core_external::get_string(
              'missingrequiredcapability',
              'webservice',
              array(
                  ????????
              )
          );

          Previously I had look at core_external::get_string and seen that it builds the params into an object by using the name => value from the webservice params. However in the above case there is no name.

          Sorry about any confusion just wrapping my head around it.

          Cheers
          Sam

          Show
          samhemelryk Sam Hemelryk added a comment - Hi Jerome, Thanks for the reply. I was just looking again, I must be missing something sorry. I get how a string request for something with {$a->one} {$a->two} would look thanks to the unit tests: // From the lang file: $string['addservice'] = 'Add a new service: {$a->name} (id: {$a->id})'; // From the unit tests: $returnedstring = core_external::get_string( 'addservice', 'webservice', array( array( 'name' => 'name', 'value' => $service->name ), array( 'name' => 'id', 'value' => $service->id ) ) ); But I don't get how I would structure the request for just a single {$a} // From the lang file: $string['missingrequiredcapability'] = 'The capability {$a} is required.'; // Trying to imagine it. $returnedstring = core_external::get_string( 'missingrequiredcapability', 'webservice', array( ???????? ) ); Previously I had look at core_external::get_string and seen that it builds the params into an object by using the name => value from the webservice params. However in the above case there is no name. Sorry about any confusion just wrapping my head around it. Cheers Sam
          Hide
          jerome Jérôme Mouneyrac added a comment -

          Ah right, I thought that the core get_string() was automatically matching $a if there were only one element in the array. Sorry, fixing it.

          Show
          jerome Jérôme Mouneyrac added a comment - Ah right, I thought that the core get_string() was automatically matching $a if there were only one element in the array. Sorry, fixing it.
          Hide
          jerome Jérôme Mouneyrac added a comment -

          All fixed Sam

          Show
          jerome Jérôme Mouneyrac added a comment - All fixed Sam
          Hide
          poltawski Dan Poltawski 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
          poltawski Dan Poltawski 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
          jleyva Juan Leyva added a comment -

          Jerome, not sure if this is done in another bug, but I don't know if you have updated the Mobile service for adding this new WS

          Can you confirm that?

          Thanks

          Show
          jleyva Juan Leyva added a comment - Jerome, not sure if this is done in another bug, but I don't know if you have updated the Mobile service for adding this new WS Can you confirm that? Thanks
          Hide
          jerome Jérôme Mouneyrac added a comment -

          Juan: I think core_get_component_strings() is enough. I implemented the get_strings ones as a bonus but there were no planned for the mobile app. We just needed a way to get the lang file, so I just added core_get_component_strings() to the mobile service. Let me know if you need the other get_string(s) web service function.

          Show
          jerome Jérôme Mouneyrac added a comment - Juan: I think core_get_component_strings() is enough. I implemented the get_strings ones as a bonus but there were no planned for the mobile app. We just needed a way to get the lang file, so I just added core_get_component_strings() to the mobile service. Let me know if you need the other get_string(s) web service function.
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Hi Jerome,

          This is down as a blocker, is it?
          (just trying to establish reasons and judge whether this is needs to land)

          Many thanks
          Sam

          Show
          samhemelryk Sam Hemelryk added a comment - Hi Jerome, This is down as a blocker, is it? (just trying to establish reasons and judge whether this is needs to land) Many thanks Sam
          Hide
          jerome Jérôme Mouneyrac added a comment -

          Hi Sam,
          I set this as a blocker because it was needed for the Moodle Mobile HTML5 app.
          Cheers,
          Jerome

          Show
          jerome Jérôme Mouneyrac added a comment - Hi Sam, I set this as a blocker because it was needed for the Moodle Mobile HTML5 app. Cheers, Jerome
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Thanks Jerome, this has been integrated now

          Show
          samhemelryk Sam Hemelryk added a comment - Thanks Jerome, this has been integrated now
          Hide
          rwijaya Rossiani Wijaya added a comment -

          This is working as expected.

          Test passed.

          Show
          rwijaya Rossiani Wijaya added a comment - This is working as expected. Test passed.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Y E S !

          Closing as fixed, many thanks!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Y E S ! Closing as fixed, many thanks!
          Hide
          jerome Jérôme Mouneyrac added a comment -

          The functions have been added to ws doc listing: http://docs.moodle.org/dev/Web_services_Roadmap#Core_web_service_functions

          Show
          jerome Jérôme Mouneyrac added a comment - The functions have been added to ws doc listing: http://docs.moodle.org/dev/Web_services_Roadmap#Core_web_service_functions

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                3/Dec/12