Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.1.1
    • Fix Version/s: 2.2
    • Component/s: Web Services
    • Labels:
    • Rank:
      18823

      Description

      It exists a draft for the web service convention: http://docs.moodle.org/dev/Web_services_Roadmap#Naming_convention_.28DRAFT_NEED_APPROVAL.29

      This issue currently block all web service integration in core.

      I would suggest to copy the current naming convention suggestion and give your own one, in the discussion page.

        Issue Links

          Activity

          Hide
          Jérôme Mouneyrac added a comment -

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

          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.

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

          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)

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

          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

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

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

          Show
          Petr Škoda added a comment - There seems to be something wrong here, we should not be deprecating stuff in the middle of stable branches, right?
          Hide
          Jérôme Mouneyrac added a comment -

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

          Show
          Jérôme Mouneyrac added a comment - 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).
          Hide
          Petr Škoda added a comment -

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

          Show
          Petr Škoda added a comment - 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...
          Hide
          Jérôme Mouneyrac added a comment -

          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.

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

          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.

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

          Still good for integration as it's now master only

          Show
          Jérôme Mouneyrac added a comment - Still good for integration as it's now master only
          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
          Petr Škoda added a comment -

          + '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

          Show
          Petr Škoda added a comment - + '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
          Hide
          Jérôme Mouneyrac added a comment -

          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.

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

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

          Show
          Jérôme Mouneyrac added a comment - Could I get back this issue from integration please, I'd like to send it to peer review to dongsheng.
          Hide
          Sam Hemelryk added a comment -

          Reopening issue as requested (yay for changes)

          Show
          Sam Hemelryk added a comment - Reopening issue as requested (yay for changes)
          Hide
          Jérôme Mouneyrac added a comment -

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

          Show
          Jérôme Mouneyrac added a comment - Dongsheng can you peer review the changes? Note: I'm not 100% sure about core_user_getbycourseid / core_user_getprofilebycourse function name.
          Hide
          Martin Dougiamas added a comment -

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

          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.

          Show
          Jérôme Mouneyrac added a comment - - edited 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.
          Hide
          Martin Dougiamas added a comment -

          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!

          Show
          Martin Dougiamas added a comment - 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!
          Hide
          Dongsheng Cai added a comment -

          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.

          Show
          Dongsheng Cai added a comment - 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.
          Hide
          Martin Dougiamas added a comment -

          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.

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

          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.

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

          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.

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

          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.

          Show
          Jérôme Mouneyrac added a comment - 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.
          Hide
          Aparup Banerjee added a comment - - edited

          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.
          Show
          Aparup Banerjee added a comment - - edited 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.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          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.

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

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

          Show
          Jérôme Mouneyrac added a comment - I linked the new issues: MDL-29717 and MDL-29718
          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
          Sam Hemelryk added a comment -

          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

          Show
          Sam Hemelryk added a comment - 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: 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. 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. 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
          Hide
          Jérôme Mouneyrac added a comment -

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

          Show
          Jérôme Mouneyrac added a comment - #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.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          +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

          Show
          Eloy Lafuente (stronk7) added a comment - +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
          Hide
          Jérôme Mouneyrac added a comment -

          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.

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

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

          Show
          Martin Dougiamas added a comment - +1 to #2. I think I said elsewhere that all the function names should have verb_noun
          Hide
          Sam Hemelryk added a comment -

          Thanks Jerome - reopening as requested

          Show
          Sam Hemelryk added a comment - Thanks Jerome - reopening as requested
          Hide
          Jérôme Mouneyrac added a comment -

          Pushing the changes

          Show
          Jérôme Mouneyrac added a comment - Pushing the changes
          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
          Martin Dougiamas added a comment -

          +100 to get this through asap

          Show
          Martin Dougiamas added a comment - +100 to get this through asap
          Hide
          Jérôme Mouneyrac added a comment -

          Rebased.

          Show
          Jérôme Mouneyrac added a comment - Rebased.
          Hide
          Aparup Banerjee added a comment -

          setting integrator to Eloy to get this in asap.

          Show
          Aparup Banerjee added a comment - setting integrator to Eloy to get this in asap.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          (all4sam)

          Show
          Eloy Lafuente (stronk7) added a comment - (all4sam)
          Hide
          Sam Hemelryk added a comment -

          Thanks Jerome - this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks Jerome - this has been integrated now
          Hide
          Martin Dougiamas added a comment -

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

          Show
          Martin Dougiamas added a comment - Jerome, can you please update http://docs.moodle.org/dev/Web_services_Roadmap to make it reflect the final version? Cheers!
          Hide
          Jérôme Mouneyrac added a comment -

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

          Show
          Jérôme Mouneyrac added a comment - Roadmap updated, I'll create a issue for the Web service API Roadmap, with each functions as subtask.
          Hide
          Jérôme Mouneyrac added a comment -

          I created this META for the Roadmap: MDL-29934

          Show
          Jérôme Mouneyrac added a comment - I created this META for the Roadmap: MDL-29934
          Hide
          Aparup Banerjee added a comment -

          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)

          Show
          Aparup Banerjee added a comment - 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)
          Hide
          Eloy Lafuente (stronk7) added a comment -

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

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Done, your delicious hacks have been sent upstream, many thanks! Closing as fixed, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: