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?

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)

> 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.

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

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

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

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

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.

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

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.

All fixed Sam

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

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

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.

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

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

Thanks Jerome, this has been integrated now

This is working as expected.

Test passed.

Y E S !

Closing as fixed, many thanks!

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