We had a meeting last week with Martin, Dongsheng and I. We came up to this solution:

• moodle_ is replace by core_
• user/course/... are never mentionned
• mod/plugin use the frankenstyle
• the second word is always a verb (or plugin name)
• moodle_enrol_manual_enrol_users => enrol_manual_create / moodle_enrol_manual_unenrol_users => enrol_manual_delete

I think that the API Documentation will just be messy with a solution like that (see screen capture), without keeping the user/course/... We probably will have to hard code the API documentation to be sorted in a human way.

The naming convention draft document (http://docs.moodle.org/dev/Web_services_Roadmap) has been updated with the latest meeting solution (matching the code sent to integration)

Summary for the integration review:

Do you agree with:
1- the missing user/course/...
2- moodle_enrol_manual_enrol_users => enrol_manual_create (e.g. enrol_PLUGINNAME_create/delete/update)
3- keeping the previous web service functions, just marking the description as DEPRECATED
4- not changing the externallib.php files and keeping writing them the way they are written (class name specially).

I guess you will likely agree with other non mentioned changes

There seems to be something wrong here, we should not be deprecating stuff in the middle of stable branches, right?

Here are the solutions I can see:
a) not renaming/changing anything, but writing the futur functions with the new naming convention
b) renaming current one
c) deprecating current function in the description and adding similar functions respecting the new naming convention

As b) would break compatibility with existing clients, I guess Petr is suggesting a).

I mean once branch gets stable you should not remove, deprecate or add any WS stuff. All the changes should be done in master only especially now that we have the short release cycles. You have only one attempt to do it right when dealing with public APIs, ideally all the public api stuff should be changed early in dev cycle so that people have enough time to test it in master and complain if something does not work as expected or is broken - in master any new stuff can be improved/changed/fixed/reverted...

Ah ok I misunderstood you. I agree with you. I'm adding Dongsheng and Martin as watcher. It was decided to backport during the last meeting.

I guess we'll probably go for master only, and backport to 2.1/2.0 only if really necessary. I'm removing the git urls about 2.1/2.0.

Still good for integration as it's now master only

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

+ 'core_send_instantmessages' => array(
+ 'core_create_notes' => array(

This is still not frankenstyle, when using core, these would be most probably:

• core_message_sendinstant
• core_notes_create

see function get_core_subsystems()

my -1

I'm ok with these changes, and the generated doc will look better I talk about that with Martin and Dongsheng, and make the changes.

Could I get back this issue from integration please, I'd like to send it to peer review to dongsheng.

Reopening issue as requested (yay for changes)

Dongsheng can you peer review the changes?
Note: I'm not 100% sure about core_user_getbycourseid / core_user_getprofilebycourse function name.

This is going around in circles! Changing the verb position is a huge change from even last week.

The problem with changing WS external function names to core_subsystem_verbnoun is that they don't match the method names in externallib.php at all.

eg in the current patch in git I see:

WS function 'core_group_create' ----> class 'moodle_group_external', method 'create_groups'
WS function 'core_group_getmembers' ----> class 'moodle_group_external', method 'get_members'

This is just too confusing and messy. We can't just leave off the noun sometimes because it happens to match the component name. Many of them deal in multiple nouns (eg forums have forums, discussions, ratings etc)

It was already decided to make the method names start with a verb, and because these are worse to change, I would keep those and make the WS names match the method.

This is in fact exactly what we had in 2.0 already: moodle_group_create_groups()

There is no good reason to change that, except possibly to core_group_create_groups() if "moodle" isn't good.

So my +1 for

• fullcomponent = frankenstyle like core_xxxx or mod_xxxx
• classname = fullcomponent_external
• methodname = verb_noun(s)
• WS function names = fullcomponent_methodname

Please confirm if I understand well:

db/services.php:

'moodle_group_create_groups' => array(

        'classname'   => 'core_group_external', //previously moodle_group_external

        'methodname'  => 'create_groups', //a) always start with a verb. b) must match the end of the web service function name.

        'classpath'   => 'group/externallib.php',

        'description' => 'DEPRECATED: use core_group_create_groups()',

        'type'        => 'write',

        'capabilities'=> 'moodle/course:managegroups',

    ),

    'core_group_create_groups' => array(

        'classname'   => 'core_group_external',

        'methodname'  => 'create_groups',

        'classpath'   => 'group/externallib.php',

        'description' => 'Creates new groups.',

        'type'        => 'write',

        'capabilities'=> 'moodle/course:managegroups',

    ),

group/externallib.php:
1) Create new core_group_external class
2) All moodle_group_external class function codes will be moved to their matching core_group_external class functions. All moodle_group_external class function codes will be just a call to their matching core_group_external class functions.

Yep, that looks OK to me and will retain backward compatibility.

Note that existing non-core classes like moodle_enrol_manual_external would also have to be renamed to enrol_manual_external.

If anyone can see any problems with this you have a day to complain!

Hi all

I am looking at core_group_create_group and core_message_send_instantmessage. core_group and core_message are frankenstyle component names.

I don't see why important to use frankenstyle in core web service names? Web service client could be java, C++ or any other languages, they cannot reuse moodle code base, what is the real benefits of frankenstyle in this case except dev documentation?

When web service client developer want to send message using moodle web service, all they want to know is there is a web service called send_instantmessage, they don't care if it's moodle core code or not, so I think send_instantmessage is a better name than core_message_send_instantmessage, it's clean and straight forward. Although I agree to use frankenstyle for module and plugin web service names, this could avoid the potential name conflicts.

I'd agree if we had total control of the system, but what we need here is a single, consistent, recommended naming guide that is guaranteed never to cause conflicts, even if different people design different parts of it at the same time. Otherwise new modules can be installed at any time and could break web services.

Apu, I send it to you for peer review.

The new naming convention can be see 2 posts above in Martin comment.

The best way for reviewing this is to cherry pick and read first the db/services.php and /enrol/manual/db/services.php. Then read all externallib.php (except the one in lib). You can come to see me I'll explain it quickly.

Reviewing is about checking:
a) new web service names match the new naming convention
b) new core class names are correct
c) new externallib.php functions have correct name
d) if externallib.php functions have a new name, check validate_parameters( xxxx_params(), ...) call.
d) the deprecated class functions are now calling the new functions

Some points that I hesitated:

• enrol_manual_create: this was decided in the last meeting (all enrol plugin are enrol_pluginname_create)
• moodle_user_get_course_participants_by_id => core_user_get_course_user_profiles (made it clearer, better description)
• didn't modify the mobile service for backward compatibility and I didn't add new functions to avoid redundancy.
• moodle_enrol_get_enrolled_users is deprecated. There is an alternative function to use but it does not behave the same way, so I just marked it as deprecated.

I retested most of the moodle_xxxx web service functions (20 of them), they were still all working fine.

Hi Jerome, i'm in the midst of looking at this..but noting quickly admin/webservice/documentation.php needs to be updated to show the new calls as well.

Good catch. The documentation is generated automatically, and we should not display the deprecated ones. The "mapping/deprecated/renamed" attribut that you suggested to me will also fix this issue I think.

Hi Jerome, just reviewing this and heres some feedback (feel free to make separate issue if its not from this issue ):

• deprecation in ws: while we are using 'DEPRECATED' in the moodle_* services description we are signifying that we will remove it in future. Will we? If we do, it might be better to consider using versioning within the namespace url of the published web services. ie: http://sitename.com/.../webservicename/yyyy/mm/dd ?

• renaming : yea it might be nice to have a ws that shows the history/mapping of the api calls according to ws version. maybe the deprecated classes should use this to perform the mapped calls too?

• just reiterating that admin/webservice/documentation.php needs to be updated to show the new calls as well.

• the description in deprecated calls mentions an alternate function, possible to provide url to function or deprecatedby field?

• some of the *_returns() document @return external_description but return null. append 'or null' ?

• missing deprecated comment in code above moodle_* classes in a few externallib.php's ( message , webservice)

• enrol/externalib.php
  • line 151 : needs clean up of debug statement varlog()
  • about 'moodle_enrol_get_enrolled_users' : see deprecation related comment above.

I fixed all mentioned points except:
1) issue: mobile service (and third parties) currently use deprecated functions.
solution: adding a renaming attribut to the service.php functions. This requires a bit more detailled specs. Web service servers should detect a function marked as deprecated/renamed. If the user is allowed to use the new matching function, then the web service server allows the access. Of course this could only be used for exact copy of the deprecated function.
2) issue: deprecated function are displayed into the API documentation
solution: adding a deprecated attribut to the service.php functions.
3) versioning.

I'll create an issue for 1) and 2).

PS: I'm not sure about versioning. Last time we talk about it, it was decided that we will not have web service versioning.

I linked the new issues: MDL-29717 and MDL-29718

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 guys,

I'm stopping the review presently - I've been looking over these changes and pondering them for the past while and have noted the following three things:

1. All deprecated functions really need to be marked as deprecated in their PHP doc, should include the version, and the new equivalent as well. e.g. _@deprecated since Moodle 2.2 please use core_blah::do_something instead. (This should be done for all functions as well as classes). I also think that the service description in services.php should include the version the function was deprecated.
2. enrol_manual_external::create I think should be renamed to something more descriptive such as enrol or enrol_users. I suspect that in the future we will want a create function for creating manual enrolment instances (which is what I thought this was doing before seeing the name change and reading the function to find out it was in fact enrolling users.
3. I'm not sure about the location of the method core_enrol_external::get_users_courses. At first I thought it's straight out in the wrong place and should be a method of core_user_external however I'm not 100% sure.
   Looking at core_user_external I see it has a method that gets all of the users enrolled in a course. core_enrol_external::get_users_courses does sort of the reverse getting all of the courses a user is enrolled in. I think one of these methods is in the wrong place however I'm not sure whether core_enrol_external::get_users_courses should become core_user_external::get_users_courses, or whether core_user_external::get_users_by_courseid should become core_enrol_external::get_enrolled_users (noting that it does use enrolment API)

I would say that #1 would be great to have done but could (if no one has time now) be done in a separate issue after integration although I would only be happy if it were guaranteed to be done and not put into the backlog.
#2 I think need to be done before this is integrated OR a very good argument put up as to why should stay named as it is.
#3 I don't know about - I think we should see what people think about this. Personally the more I look at it the more I think core_user_external::get_users_by_courseid should be moved to core_enrol_external.

Please please please lets get this decided so that it can go in well before 2.2 enters QA testing phase!
I have left this in integration presently so that we can get people's views.

Cheers
Sam

#1 as you requested, I can guarantee you I'll change it if it is integrated. But as I'll probably have to do more changes after #2 and #3 are decided, I'll make the changes before integration. +1

#2 From a client developer point of view, enrol_manual_enrol_users it's clearer to me. To summarise I would say we probably didn't think about the confusion with creating enrolment instance. +1 to go back to enrol_manual_enrol_users

#3 They could be both course external functions, they could be also user, and they could be enrol too I think they are most likely enrol. We could move get_users_by_courseid() into enrol external lib - +1 for that if we move one of these functions. PS: the deprecated functions will stay in their original files whatever we move or don't move the new function.

+1 to move to enrol external any method using the enrolments API and somehow, following the naming convention introduce the "enrolled" word instead of so many user/course/users/courses. (that said by one non native speaker, of course)

Ciao

It makes to rename core_course_get_users_by_courseid() into core_enrol_get_enrolled_users() as the deprecated (but not renamed) function moodle_enrol_get_enrolled_users() advocate to use core_course_get_users_by_courseid() now. I guess we all agree on #3.

Martin can you give your agreement on #2?

Sam, you can send it back to development, I'll push the changes to integration tomorrow

Thanks.

+1 to #2. I think I said elsewhere that all the function names should have verb_noun

Thanks Jerome - reopening as requested

Pushing the changes

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

+100 to get this through asap

Rebased.

setting integrator to Eloy to get this in asap.

(all4sam)

Thanks Jerome - this has been integrated now

Jerome, can you please update http://docs.moodle.org/dev/Web_services_Roadmap to make it reflect the final version? Cheers!

Roadmap updated, I'll create a issue for the Web service API Roadmap, with each functions as subtask.

I created this META for the Roadmap: MDL-29934

all the ws functions have been tested with Jerome's unit test scripts. (he's probably got an issue where its from i can't find it)

Done, your delicious hacks have been sent upstream, many thanks!

Closing as fixed, ciao