Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Course, Web Services
    • Labels:
    • Testing Instructions:
      Hide

      a) Create on second level category hidden with a not hidden sub category. The web service function should not return the hidden category, neither its sub category. Try again with a user having the capability 'moodle/category:viewhiddencategories', the function should return both categories.
      b) Create a third level category with a sub category. Set the max category level of your Moodle to 3. The web service function should return the third level category but not its sub category.
      c) call the web service function as a user having 'moodle/category:manage' capability, the web service function should return all categories (hidden categories and deep level categories).

      Use this client:
      https://github.com/moodlehq/sample-ws-clients/blob/master/PHP-REST/client.php

      functionname
      core_course_get_categories

      /// PARAMETERS

      $params = array('criteria' => array(array('key' => 'visible', 'value' => '1')), 'addsubcategories' => 0);

      'key' => The category column to search, expected keys (value format) are:
      "id" (int) the category id,
      "name" (string) the category name,
      "parent" (int) the parent category id,
      "idnumber" (string) category idnumber,
      "visible" (int) whether the category is visible or not,
      "theme" (string) category theme'),

      Show
      a) Create on second level category hidden with a not hidden sub category. The web service function should not return the hidden category, neither its sub category. Try again with a user having the capability 'moodle/category:viewhiddencategories', the function should return both categories. b) Create a third level category with a sub category. Set the max category level of your Moodle to 3. The web service function should return the third level category but not its sub category. c) call the web service function as a user having 'moodle/category:manage' capability, the web service function should return all categories (hidden categories and deep level categories). Use this client: https://github.com/moodlehq/sample-ws-clients/blob/master/PHP-REST/client.php functionname core_course_get_categories /// PARAMETERS $params = array('criteria' => array(array('key' => 'visible', 'value' => '1')), 'addsubcategories' => 0); 'key' => The category column to search, expected keys (value format) are: "id" (int) the category id, "name" (string) the category name, "parent" (int) the parent category id, "idnumber" (string) category idnumber, "visible" (int) whether the category is visible or not, "theme" (string) category theme'),
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
      git@github.com:mouneyrac/moodle.git
    • Pull Master Branch:
      MDL-30084-final
    • Rank:
      24715

      Description

      Create web service function core_course_get_categories()

      1. externallib.php.patch
        4 kB
        Fábio Souto
      2. services.php.patch
        0.7 kB
        Fábio Souto
      3. smurf.xml
        8 kB
        Eloy Lafuente (stronk7)

        Issue Links

          Activity

          Hide
          Fábio Souto added a comment -

          I would like to contribute on this feature, since I will need it for an integration on which I am working.

          I've taken a look at the data structure on the database, and would like to expose at least this function, because it would be useful to organize newly created courses via webservices (otherwise I see no way of organizing the created courses).

          I can't seem to find any documentation regarding the architecture of webservices, is there anything about this?

          Show
          Fábio Souto added a comment - I would like to contribute on this feature, since I will need it for an integration on which I am working. I've taken a look at the data structure on the database, and would like to expose at least this function, because it would be useful to organize newly created courses via webservices (otherwise I see no way of organizing the created courses). I can't seem to find any documentation regarding the architecture of webservices, is there anything about this?
          Hide
          Fábio Souto added a comment - - edited

          Hello again,

          I've taken a closer look and managed to understand how things work.
          I have created this webservice but i'm still not sure if it's totally correct, so I would like it to be reviewed.
          I will attach two patches: one to course/externallib.php and another to lib/db/services.php
          for the version mdl 2.2 2011120500.01

          I would be glad to create the remaining functionality related to categories if this one is correctly implemented.
          From my tests it works correctly, but it accepts no parameters.

          Fábio.

          Show
          Fábio Souto added a comment - - edited Hello again, I've taken a closer look and managed to understand how things work. I have created this webservice but i'm still not sure if it's totally correct, so I would like it to be reviewed. I will attach two patches: one to course/externallib.php and another to lib/db/services.php for the version mdl 2.2 2011120500.01 I would be glad to create the remaining functionality related to categories if this one is correctly implemented. From my tests it works correctly, but it accepts no parameters. Fábio.
          Hide
          Fábio Souto added a comment -

          patch to course/externallib.php
          and lib/db/services.php

          Show
          Fábio Souto added a comment - patch to course/externallib.php and lib/db/services.php
          Hide
          Fábio Souto added a comment -

          Sorry for the comment spam, but I've finally read the document for git contributions,
          therefore I have created a git branch for this patch, as advised.
          The branch for this issue can be found on https://github.com/fabiomsouto/moodle/tree/MDL-30084 .

          Therefore the former patches should be ignored and the changes should be obtained from my public repository.

          Show
          Fábio Souto added a comment - Sorry for the comment spam, but I've finally read the document for git contributions, therefore I have created a git branch for this patch, as advised. The branch for this issue can be found on https://github.com/fabiomsouto/moodle/tree/MDL-30084 . Therefore the former patches should be ignored and the changes should be obtained from my public repository.
          Hide
          Jérôme Mouneyrac added a comment -

          Hi Fabio. I'll have a look to your code after January (we focus on the "docs" this month). If you took example from some core files, can you indicate which ones? It's easier for me to know where to look for capabilities/categories related checks when I review. Thanks for sharing your code.

          Show
          Jérôme Mouneyrac added a comment - Hi Fabio. I'll have a look to your code after January (we focus on the "docs" this month). If you took example from some core files, can you indicate which ones? It's easier for me to know where to look for capabilities/categories related checks when I review. Thanks for sharing your code.
          Hide
          Fábio Souto added a comment -

          Hello,

          This one i took inspiration from course/lib.php, function print_whole_category_list() at lines 2103-2143 approximately.

          Fabio

          Show
          Fábio Souto added a comment - Hello, This one i took inspiration from course/lib.php, function print_whole_category_list() at lines 2103-2143 approximately. Fabio
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks a lot Fabio, it's very good!

          Some notes I think we should improve:

          • we should add one categoryid parameter. It would return the category and sub categories
          • we could add one nosubcategories parameter. True by default, if set to False then we just return the requested category.
          • the moodle exception probably should be written like:
            moodle_exception('errorcatcontextnotvalid', 'webservice', '', $exceptionparam)
            
          • some fields seem a bit more admin fields: idnumber, timemodified, visibleold, theme. I think the correct way would be to set these fields into a if has_capability('moodle/category:manage')) (and they already are OPTIONAL which make sens).

          I also checked about the depth settings if there was a missing capibility check. Users can still access the category (bu url) even though if the depth is too big. Actually even logged out you can access any category... It doesn't seem you missed any capibility.

          How much time can you work on that? If you don't have much time I'll add the change but I would prefer that you make the changes and submit only one commit for integration review. You can do multiple 'git reset --soft HEAD^' to go back one commit before without losing your changes.

          Show
          Jérôme Mouneyrac added a comment - Thanks a lot Fabio, it's very good! Some notes I think we should improve: we should add one categoryid parameter. It would return the category and sub categories we could add one nosubcategories parameter. True by default, if set to False then we just return the requested category. the moodle exception probably should be written like: moodle_exception('errorcatcontextnotvalid', 'webservice', '', $exceptionparam) some fields seem a bit more admin fields: idnumber, timemodified, visibleold, theme. I think the correct way would be to set these fields into a if has_capability('moodle/category:manage')) (and they already are OPTIONAL which make sens). I also checked about the depth settings if there was a missing capibility check. Users can still access the category (bu url) even though if the depth is too big. Actually even logged out you can access any category... It doesn't seem you missed any capibility. How much time can you work on that? If you don't have much time I'll add the change but I would prefer that you make the changes and submit only one commit for integration review. You can do multiple 'git reset --soft HEAD^' to go back one commit before without losing your changes.
          Hide
          Fábio Souto added a comment -

          Hey Jerome,

          Right now I have some time, fortunately
          I agree with your suggestions, it will make the webservice more robust, just some questions:

          • Regarding the nosubcategories, suppose I give an ID: we return that category and all subcategories of it, is that it?

          I will give feedback if any more questions arise. Regarding git, wish me luck

          Cheers,
          Fábio

          Show
          Fábio Souto added a comment - Hey Jerome, Right now I have some time, fortunately I agree with your suggestions, it will make the webservice more robust, just some questions: Regarding the nosubcategories, suppose I give an ID: we return that category and all subcategories of it, is that it? I will give feedback if any more questions arise. Regarding git, wish me luck Cheers, Fábio
          Hide
          Jérôme Mouneyrac added a comment -

          Cool, thanks Fabio.

          I was thinking:

          if categoryid is not empty then
              if nosubcategories return only the category 
              else return category + its subcategories
          else return all categories in the DB
          
          Show
          Jérôme Mouneyrac added a comment - Cool, thanks Fabio. I was thinking: if categoryid is not empty then if nosubcategories return only the category else return category + its subcategories else return all categories in the DB
          Hide
          Fábio Souto added a comment - - edited

          Hello,

          Glad we thought the same, I only seen your answer now!
          The updated code can be found at https://github.com/fabiomsouto/moodle/tree/MDL-30084

          Compare to MDL22 https://github.com/fabiomsouto/moodle/compare/MOODLE_22_STABLE...MDL-30084

          Cheers

          Show
          Fábio Souto added a comment - - edited Hello, Glad we thought the same, I only seen your answer now! The updated code can be found at https://github.com/fabiomsouto/moodle/tree/MDL-30084 Compare to MDL22 https://github.com/fabiomsouto/moodle/compare/MOODLE_22_STABLE...MDL-30084 Cheers
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks Fabio, I'll be back on the issue by Monday. I did some extra changes. It's still work in progress as I haven't tested yet: https://github.com/mouneyrac/moodle/commit/661b6778516ff5c42bd2a0c049db943b4267da65
          Let me know if it seems ok to you (except about using twice :categoryid in my request. If I remember I cannot use twice the same DB param, but it look cleaner like that so would like to test if it works)

          Show
          Jérôme Mouneyrac added a comment - Thanks Fabio, I'll be back on the issue by Monday. I did some extra changes. It's still work in progress as I haven't tested yet: https://github.com/mouneyrac/moodle/commit/661b6778516ff5c42bd2a0c049db943b4267da65 Let me know if it seems ok to you (except about using twice :categoryid in my request. If I remember I cannot use twice the same DB param, but it look cleaner like that so would like to test if it works)
          Hide
          Fábio Souto added a comment -

          Ah yes, about the parameters, I did that at first, but I was getting warnings because i wasn't defining the default values at the function header. I thought the default values should only be defined at the xxx_parameters() function, but I see no reason for not doing it this way.

          Other than that, the SQL optimization is great, I didn't know how to do just one query instead of two. I'll give it a test and let you know if it works

          Cheers

          Show
          Fábio Souto added a comment - Ah yes, about the parameters, I did that at first, but I was getting warnings because i wasn't defining the default values at the function header. I thought the default values should only be defined at the xxx_parameters() function, but I see no reason for not doing it this way. Other than that, the SQL optimization is great, I didn't know how to do just one query instead of two. I'll give it a test and let you know if it works Cheers
          Hide
          Fábio Souto added a comment -

          I tested your code, here are the results:

          At line 263 you forgot to correct the validation call for the new parameters, it should be

          $params = self::validate_parameters(self::get_categories_parameters(), array('categoryid' => $categoryid, 'nosubcategories' => $nosubcategories));
          

          As for the SQL you were right:

          <EXCEPTION class="dml_exception">
          <MESSAGE>ERROR: Incorrect number of query parameters. Expected 2, got 1.</MESSAGE>
          </EXCEPTION>
          

          I think its just a matter of renaming the SQL parameter to something else and pass it with the same value of categoryid, it isn't very elegant but it works as we wish it to.

          See you monday,
          Fábio.

          Show
          Fábio Souto added a comment - I tested your code, here are the results: At line 263 you forgot to correct the validation call for the new parameters, it should be $params = self::validate_parameters(self::get_categories_parameters(), array('categoryid' => $categoryid, 'nosubcategories' => $nosubcategories)); As for the SQL you were right: <EXCEPTION class= "dml_exception" > <MESSAGE>ERROR: Incorrect number of query parameters. Expected 2, got 1.</MESSAGE> </EXCEPTION> I think its just a matter of renaming the SQL parameter to something else and pass it with the same value of categoryid, it isn't very elegant but it works as we wish it to. See you monday, Fábio.
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks Fabio. I made the changes as you suggested.

          I also filtered and formated the description. We need to transform the "@@PLUGINFILE@@" into real url. For example you can notice some "@@PLUGINFILE@@" in the description if you include an image with the HTML editor.

          I'll submit the issue for integration. I'll be back on web service next week and I'll finish the review of your other categories contribution.

          Show
          Jérôme Mouneyrac added a comment - Thanks Fabio. I made the changes as you suggested. I also filtered and formated the description. We need to transform the "@@PLUGINFILE@@" into real url. For example you can notice some "@@PLUGINFILE@@" in the description if you include an image with the HTML editor. I'll submit the issue for integration. I'll be back on web service next week and I'll finish the review of your other categories contribution.
          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 -

          Hi, 2 little things that prevent me to land this and 1 question:

          1) The file_rewrite_pluginfile_urls() is returning "pluginfile.php" urls. Shouldn't it return "webservice/pluginfile.php" instead?

          2) The "nosubcategories" parameter. I think that negative parameters in general are not a good idea. Could we switch it to the opposite (yes)"categories", changing the default and so on. But always positive params.

          3) I don't know which is the plan for this, have been looking at MDL-29934 and http://docs.moodle.org/dev/Web_services_Roadmap (that seems slightly outdated), but it's not clear for me if this (new webservices) are expected to land only to master or also to recent stable when possible. My (strict) feelings are that they should be master-only... but I don't know if that applies always.

          So I'm reopening this for minor tidying. Thanks a lot, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi, 2 little things that prevent me to land this and 1 question: 1) The file_rewrite_pluginfile_urls() is returning "pluginfile.php" urls. Shouldn't it return "webservice/pluginfile.php" instead? 2) The "nosubcategories" parameter. I think that negative parameters in general are not a good idea. Could we switch it to the opposite (yes)"categories", changing the default and so on. But always positive params. 3) I don't know which is the plan for this, have been looking at MDL-29934 and http://docs.moodle.org/dev/Web_services_Roadmap (that seems slightly outdated), but it's not clear for me if this (new webservices) are expected to land only to master or also to recent stable when possible. My (strict) feelings are that they should be master-only... but I don't know if that applies always. So I'm reopening this for minor tidying. Thanks a lot, ciao
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Thanks for reviewing Eloy.

          1) it's what we do with all other web service functions. I'll ask Dongsheng his opinion. In my opinion (I'm not a file expert) I suppose you are right. file_rewrite_pluginfile_urls() probably would need a new param to return webservice/pluginfile.php instead of the root one.

          In the meanwhile, this issue submission is consistent with the rest of all web service functions. I don't think we should reject this issue waiting for a global "solution" to be implemented (Dongsheng is very busy atm).

          2) I knew it too, but the code looked better like that. But you are right, I'll come up with something less negative

          3) the Moodledocs could be outdated (but really not much), I try to keep it up to date as much as possible. Let me know when you spot an error. New web service functions only land in master (no backport), it's to encourage updating your Moodle.

          Show
          Jérôme Mouneyrac added a comment - - edited Thanks for reviewing Eloy. 1) it's what we do with all other web service functions. I'll ask Dongsheng his opinion. In my opinion (I'm not a file expert) I suppose you are right. file_rewrite_pluginfile_urls() probably would need a new param to return webservice/pluginfile.php instead of the root one. In the meanwhile, this issue submission is consistent with the rest of all web service functions. I don't think we should reject this issue waiting for a global "solution" to be implemented (Dongsheng is very busy atm). 2) I knew it too, but the code looked better like that. But you are right, I'll come up with something less negative 3) the Moodledocs could be outdated (but really not much), I try to keep it up to date as much as possible. Let me know when you spot an error. New web service functions only land in master (no backport), it's to encourage updating your Moodle.
          Hide
          Jérôme Mouneyrac added a comment -

          Eloy, I'll see with Donghseng for 1) and I will create an issue if necessary (I'll let you know what we end up too).

          I rebased and changed the negative name of the variable.

          Thank you and particularly thanks to Fabio

          Show
          Jérôme Mouneyrac added a comment - Eloy, I'll see with Donghseng for 1) and I will create an issue if necessary (I'll let you know what we end up too). I rebased and changed the negative name of the variable. Thank you and particularly thanks to Fabio
          Hide
          Jérôme Mouneyrac added a comment -

          Ah finally Dongsheng explained to me we just need to change 'pluginfile.php' by 'webservice/pluginfile.php', it's probably what you meant. Changing this

          Show
          Jérôme Mouneyrac added a comment - Ah finally Dongsheng explained to me we just need to change 'pluginfile.php' by 'webservice/pluginfile.php', it's probably what you meant. Changing this
          Hide
          Jérôme Mouneyrac added a comment -

          Done.

          Show
          Jérôme Mouneyrac added a comment - Done.
          Hide
          Fábio Souto added a comment -

          Glad my code is of use! (or at least most of it!)

          Cheers,
          Fabio

          Show
          Fábio Souto added a comment - Glad my code is of use! (or at least most of it!) Cheers, Fabio
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Oh, sorry... but unless I'm reading code wrongly.... it seems that you always add the:

          $sqlparams['parentcategoryid'] = $categoryid;
          

          while it's not always used in the SQL (the "OR" is conditionally added). IMO both the SQL and the param should be added exactly at the same time.

          Also, lol, this is not really important... naming one parameter "thecategoryonly", while better than the previous "negative" one... it's somehow funny. Why not "recursive" or "addchildren" (if you rally only want to get adjacent children and not recurse deep in the tree).

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

          Show
          Eloy Lafuente (stronk7) added a comment - Oh, sorry... but unless I'm reading code wrongly.... it seems that you always add the: $sqlparams['parentcategoryid'] = $categoryid; while it's not always used in the SQL (the "OR" is conditionally added). IMO both the SQL and the param should be added exactly at the same time. Also, lol, this is not really important... naming one parameter "thecategoryonly", while better than the previous "negative" one... it's somehow funny. Why not "recursive" or "addchildren" (if you rally only want to get adjacent children and not recurse deep in the tree). For your consideration, I'll keep this awaiting some hours...ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Reopening, out of schedule for this week. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Reopening, out of schedule for this week. Ciao
          Hide
          Jérôme Mouneyrac added a comment -

          Ah Eloy, I can not trick you with my param names
          I vote for 'addsubcategories', I'm going for that if you're ok, and also changing the location of sqlparams['...
          Thanks

          Show
          Jérôme Mouneyrac added a comment - Ah Eloy, I can not trick you with my param names I vote for 'addsubcategories', I'm going for that if you're ok, and also changing the location of sqlparams['... Thanks
          Hide
          Jérôme Mouneyrac added a comment -

          Done, I also fixed a scope issue found by Tim's codechecker (now I run the two codechecker scripts before been caught by the police )

          Show
          Jérôme Mouneyrac added a comment - Done, I also fixed a scope issue found by Tim's codechecker (now I run the two codechecker scripts before been caught by the police )
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Some hours ago...

          the main moodle.git repository has 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 - Some hours ago... the main moodle.git repository has 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 -

          to integrators: please verify this function matches the hidden course/category browsing logic introduced in 2.0, when asking for a list of visible categories/courses we do not include courses and categories in hidden categories, but when asking for list of my enrolled courses we ignore these restrictions. I am not sure how this should apply to web services, but the results seem to be different from current UI code...

          Show
          Petr Škoda added a comment - to integrators: please verify this function matches the hidden course/category browsing logic introduced in 2.0, when asking for a list of visible categories/courses we do not include courses and categories in hidden categories, but when asking for list of my enrolled courses we ignore these restrictions. I am not sure how this should apply to web services, but the results seem to be different from current UI code...
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Uhm, if I've not read the code incorrectly... this function looks for a given category and its first-level children (if parametrised to do that). So it never goes deeper than 1 level, or to children courses at all.

          Also, it seems that the visibility bit is checked for all those children (1st level) categories, so in practice it's preventing to navigate to that category (because it's never returned).

          The only point that perhaps needs clarification is that it seems that we are not performing the visible + 'category:viewhiddencategories' cap check to the main category and it sounds like we should be doing that, preventing children to be calculated if the main category (the requested) is not accessible. And returning no category information in that case.

          Thoughts, Jerome?

          Ciao

          PS: Thanks Petr!

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Uhm, if I've not read the code incorrectly... this function looks for a given category and its first-level children (if parametrised to do that). So it never goes deeper than 1 level, or to children courses at all. Also, it seems that the visibility bit is checked for all those children (1st level) categories, so in practice it's preventing to navigate to that category (because it's never returned). The only point that perhaps needs clarification is that it seems that we are not performing the visible + 'category:viewhiddencategories' cap check to the main category and it sounds like we should be doing that, preventing children to be calculated if the main category (the requested) is not accessible. And returning no category information in that case. Thoughts, Jerome? Ciao PS: Thanks Petr!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Reopening, too late for this week. Review/clarification of the capability for the "parent" (main) category is needed.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Reopening, too late for this week. Review/clarification of the capability for the "parent" (main) category is needed. Ciao
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Yes its not super good to return only one depth level. I'll fix that.

          It would have been good to use print_whole_category_list() to do it. But it only prints 'echo'. I'll check if I can come up with anything good from adding some $noprint param or more. However I guess I'll go for writing the full logic in the ws function. It's because the ws function also returns the same info than admin page. And also the same info that the other page talked here, about enrolled courses, in "my Moodle" I suppose.

          Show
          Jérôme Mouneyrac added a comment - - edited Yes its not super good to return only one depth level. I'll fix that. It would have been good to use print_whole_category_list() to do it. But it only prints 'echo'. I'll check if I can come up with anything good from adding some $noprint param or more. However I guess I'll go for writing the full logic in the ws function. It's because the ws function also returns the same info than admin page. And also the same info that the other page talked here, about enrolled courses, in "my Moodle" I suppose.
          Hide
          Fábio Souto added a comment - - edited

          To return the whole subtree of categories, the path field could be used.
          For example, a category structure like this:

             1
             |
             2
            /|\
           3 5 6
           | |
           7 8
          

          And I wish to get node 2 and all subcategories

          The node 2 has the path 1/2
          The node 3 has the path 1/2/3
          The node 5 has the path 1/2/5
          The node 6 has the path 1/2/6
          The node 7 has the path 1/2/3/7 and so forth

          So if I ask for all categories in which path LIKE '%/2%' (using SQL), this returns the category and all subcategories I think.
          What do you think of this solution?

          Cheers,
          Fábio

          Show
          Fábio Souto added a comment - - edited To return the whole subtree of categories, the path field could be used. For example, a category structure like this: 1 | 2 /|\ 3 5 6 | | 7 8 And I wish to get node 2 and all subcategories The node 2 has the path 1/2 The node 3 has the path 1/2/3 The node 5 has the path 1/2/5 The node 6 has the path 1/2/6 The node 7 has the path 1/2/3/7 and so forth So if I ask for all categories in which path LIKE '%/2%' (using SQL), this returns the category and all subcategories I think. What do you think of this solution? Cheers, Fábio
          Hide
          Jérôme Mouneyrac added a comment -

          Hi Fabio, yes it seems good to me to look at the path. About the LIKE sql, it has big performance issue when it is used with left %. But nothing we can't avoid, fixing it.

          Show
          Jérôme Mouneyrac added a comment - Hi Fabio, yes it seems good to me to look at the path. About the LIKE sql, it has big performance issue when it is used with left %. But nothing we can't avoid, fixing it.
          Hide
          Jérôme Mouneyrac added a comment -

          I did some changes. It returns all categories that the user is allowed to see. Which means:

          • admin will receive all categories whatever there are hidden or too deep (like in the admin page)
          • visible category are returned if they are under at hidden category or if they are too deep.

          Also worth to mention about returned categories:

          • no params: all categories in Moodle
          • category id: the category matching the id
          • addsubcategories: all sub categories of the category
          Show
          Jérôme Mouneyrac added a comment - I did some changes. It returns all categories that the user is allowed to see. Which means: admin will receive all categories whatever there are hidden or too deep (like in the admin page) visible category are returned if they are under at hidden category or if they are too deep. Also worth to mention about returned categories: no params: all categories in Moodle category id: the category matching the id addsubcategories: all sub categories of the category
          Hide
          Sam Hemelryk 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
          Sam Hemelryk 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 -

          Hi Jerome, one tiny detail.... I don't think the like is correct as far as you are doing "$category->path.'%'" and I'm sure that will lead to problems with, for example, 2 and 22 categories belonging to different branches. IMO you should always add "/%" to avoid false matchings like that.

          About the query, also, perhaps it's better to return the results order by path, so when one children category is checked, we are 100% sure that its parents have been processed already. Right now, you're getting unordered categories, so it's possible that you still have not processed the parents (but you are looking for them in the $excludedcats).

          Also, I've been trying to imagine what the hell validate_context() does, and I've been completely unable to do so. It seems that always is instantiated with system context. Anyway, that's unrelated with this issue. Just have seen it in a lot of functions and was curious.

          Finally, really little detail, I've observed that you're using "//" comments and never add one space + uppercase after them. I'm not sure if that is checked or rulez (surely no), but I'm all about to propose that being a rule (mainly because in all examples such whitespace after "//" exists). Not critical, you know, only a recommendation (for now, hehe).

          So, I'm halting this till next week to allow you to fix those problems in the SQL. In the mean time, I'll try to learn about that validate_context() thingy.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Jerome, one tiny detail.... I don't think the like is correct as far as you are doing "$category->path.'%'" and I'm sure that will lead to problems with, for example, 2 and 22 categories belonging to different branches. IMO you should always add "/%" to avoid false matchings like that. About the query, also, perhaps it's better to return the results order by path, so when one children category is checked, we are 100% sure that its parents have been processed already. Right now, you're getting unordered categories, so it's possible that you still have not processed the parents (but you are looking for them in the $excludedcats). Also, I've been trying to imagine what the hell validate_context() does, and I've been completely unable to do so. It seems that always is instantiated with system context. Anyway, that's unrelated with this issue. Just have seen it in a lot of functions and was curious. Finally, really little detail, I've observed that you're using "//" comments and never add one space + uppercase after them. I'm not sure if that is checked or rulez (surely no), but I'm all about to propose that being a rule (mainly because in all examples such whitespace after "//" exists). Not critical, you know, only a recommendation (for now, hehe). So, I'm halting this till next week to allow you to fix those problems in the SQL. In the mean time, I'll try to learn about that validate_context() thingy. Ciao
          Hide
          Fábio Souto added a comment -

          Hello all,

          Jerome, I've noticed this patch was yet on MOODLE_22_STABLE branch, but since it's going to master, I've cherry picked it into master (it should make life easier to the integrators I suppose).

          You can find it at https://github.com/fabiomsouto/moodle/tree/MDL-30084-master

          Fabio

          Show
          Fábio Souto added a comment - Hello all, Jerome, I've noticed this patch was yet on MOODLE_22_STABLE branch, but since it's going to master, I've cherry picked it into master (it should make life easier to the integrators I suppose). You can find it at https://github.com/fabiomsouto/moodle/tree/MDL-30084-master Fabio
          Hide
          Jérôme Mouneyrac added a comment -

          Hi Fabio, my branch has been created from master, it's just an error on the issue info, I'm changing it. Thanks Eloy for reviewing, I'm changing the code too and resubmitting.

          Show
          Jérôme Mouneyrac added a comment - Hi Fabio, my branch has been created from master, it's just an error on the issue info, I'm changing it. Thanks Eloy for reviewing, I'm changing the code too and resubmitting.
          Hide
          Jérôme Mouneyrac added a comment -

          I made the requested changes Eloy, thanks for catching them

          Show
          Jérôme Mouneyrac added a comment - I made the requested changes Eloy, thanks for catching them
          Hide
          Aparup Banerjee added a comment -

          The main moodle.git repository has just been updated (yesterday) 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
          Aparup Banerjee added a comment - The main moodle.git repository has just been updated (yesterday) 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 -

          Oh,

          the get_records() seems to be missing the proper order, so I bet it will cause problems when called without params.

          Also, surely it would be a great moment to fix the inline comments with the new coding style rules (attaching xml report above, ignore the line-len ones, also there is one whitespace problem, don't forget it).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Oh, the get_records() seems to be missing the proper order, so I bet it will cause problems when called without params. Also, surely it would be a great moment to fix the inline comments with the new coding style rules (attaching xml report above, ignore the line-len ones, also there is one whitespace problem, don't forget it). Ciao
          Hide
          Jérôme Mouneyrac added a comment -

          Juan is suggesting some new params that would make sens: MDL-32455
          Juan/Fabio do you want to take over from the current github code?

          Show
          Jérôme Mouneyrac added a comment - Juan is suggesting some new params that would make sens: MDL-32455 Juan/Fabio do you want to take over from the current github code?
          Hide
          Fábio Souto added a comment -

          Hello all,

          Isn't it better to first integrate a working version and then upgrade it?
          Regarding the addition, are there ever going to be so many categories that justify filtering them by conditions?

          I can take over if you'd like.
          Cheers,
          Fabio

          Show
          Fábio Souto added a comment - Hello all, Isn't it better to first integrate a working version and then upgrade it? Regarding the addition, are there ever going to be so many categories that justify filtering them by conditions? I can take over if you'd like. Cheers, Fabio
          Hide
          Juan Leyva added a comment -

          Hi Fabio,

          I will modify the current code for adding the new params as Jerome suggested, I will post here the final code for your review

          Regards

          Show
          Juan Leyva added a comment - Hi Fabio, I will modify the current code for adding the new params as Jerome suggested, I will post here the final code for your review Regards
          Hide
          Fábio Souto added a comment -

          Hello Juan,

          Ok, thank you. Let me know when it's done so I can fix what Eloy appointed (get_record sort order and style rules)

          Cheers,
          Fabio

          Show
          Fábio Souto added a comment - Hello Juan, Ok, thank you. Let me know when it's done so I can fix what Eloy appointed (get_record sort order and style rules) Cheers, Fabio
          Hide
          Jérôme Mouneyrac added a comment -

          Hi guys,
          I think it's better if Juan implement Eloy's last suggestion too. I learned that you both can not submit to integration (it's a different kind of permission than being assignee). I'd like to avoid ping-pong between assignee(s)/peer-reviewer(s)/integrator(s), or me forgetting to press the submit to integration button
          Cheers,
          Jerome

          Show
          Jérôme Mouneyrac added a comment - Hi guys, I think it's better if Juan implement Eloy's last suggestion too. I learned that you both can not submit to integration (it's a different kind of permission than being assignee). I'd like to avoid ping-pong between assignee(s)/peer-reviewer(s)/integrator(s), or me forgetting to press the submit to integration button Cheers, Jerome
          Hide
          Juan Leyva added a comment - - edited

          Finished and tested, it works fine. Requesting a peer review:

          https://gist.github.com/2404209

          Jerome, I finally kept the addsubcategories as an additional param

          Sorry again for not adding a pull branch, if the peer review is OK I will create the pulll branch at MASTER

          Show
          Juan Leyva added a comment - - edited Finished and tested, it works fine. Requesting a peer review: https://gist.github.com/2404209 Jerome, I finally kept the addsubcategories as an additional param Sorry again for not adding a pull branch, if the peer review is OK I will create the pulll branch at MASTER
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Hi Juan,
          I think we should use a switch

          { case:}

          instead of a foreach. We don't want people to search on every fields and we also want to validate their values. Field list:

          get_categories_parameters():

          • set "value" to PARAM_RAW.
          • describe the expected keys ("id", "name", "parent", "visible", "theme") and their format ("int", "string", "int", ...). I would mention all them in the 'keys' description text.

          get_categories():

          • replace the foreach by a switch. For each 'case:' do a clean_param($value, PARAM_xxx) and throw an exception when it fails. The 'default' should throw an "unexpected value" exception.
          • generally avoid one letter variable

          I don't know if you thought about that so in case: database fields are often linked to different permissions (for example 'visible' => 'moodle/category:manage', 'moodle/category:viewhiddencategories'). Here in our current github/gist implementation, the user can only know the theme if he has 'moodle/category:manage' capability on the categoru context. However if we let users search on 'theme' then they would guess what the theme without the capability. I don't think it's a big deal to be worry here for category theme, but it's important to remember when implementing other functions with sensitive data.

          Thanks Juan.

          Show
          Jérôme Mouneyrac added a comment - - edited Hi Juan, I think we should use a switch { case:} instead of a foreach. We don't want people to search on every fields and we also want to validate their values. Field list: get_categories_parameters(): set "value" to PARAM_RAW. describe the expected keys ("id", "name", "parent", "visible", "theme") and their format ("int", "string", "int", ...). I would mention all them in the 'keys' description text. get_categories(): replace the foreach by a switch. For each 'case:' do a clean_param($value, PARAM_xxx) and throw an exception when it fails. The 'default' should throw an "unexpected value" exception. generally avoid one letter variable I don't know if you thought about that so in case: database fields are often linked to different permissions (for example 'visible' => 'moodle/category:manage', 'moodle/category:viewhiddencategories'). Here in our current github/gist implementation, the user can only know the theme if he has 'moodle/category:manage' capability on the categoru context. However if we let users search on 'theme' then they would guess what the theme without the capability. I don't think it's a big deal to be worry here for category theme, but it's important to remember when implementing other functions with sensitive data. Thanks Juan.
          Hide
          Juan Leyva added a comment -

          Ok, you are right about permissions

          So, the expected keys are only

          "id", "name", "parent", "visible", "theme"?

          What about idnumber?

          Regards

          Show
          Juan Leyva added a comment - Ok, you are right about permissions So, the expected keys are only "id", "name", "parent", "visible", "theme"? What about idnumber? Regards
          Hide
          Juan Leyva added a comment -

          Code upgraded with the changes requested:

          https://gist.github.com/2404209

          Notes:

          I restrict these keys search: idnumber and theme to users with system capability moodle/category:manage allowed

          visible is not restricted because there is further checks in this capability when showing the results, so only visible categories for the current user are displayed

          I haven't find a unexpected value lang string, so I've used invalidextparam in webservice

          Show
          Juan Leyva added a comment - Code upgraded with the changes requested: https://gist.github.com/2404209 Notes: I restrict these keys search: idnumber and theme to users with system capability moodle/category:manage allowed visible is not restricted because there is further checks in this capability when showing the results, so only visible categories for the current user are displayed I haven't find a unexpected value lang string, so I've used invalidextparam in webservice
          Hide
          Jérôme Mouneyrac added a comment -

          Minor type fix to apply (this is what to paste):
          'The category column to search, expected keys (value format) are:
          "id" (int) the category id,
          "name" (string) the category name,
          "parent" (int) the parent category id,
          "idnumber" (string) category idnumber,
          "visible" (int) whether the category is visible or not,
          "theme" (string) category theme'

          All other changes look good to me. You can add your branch to the issue and I'll submit our team work It's the first time I see a three contributions issue

          Show
          Jérôme Mouneyrac added a comment - Minor type fix to apply (this is what to paste): 'The category column to search, expected keys (value format) are: "id" (int) the category id, "name" (string) the category name, "parent" (int) the parent category id, "idnumber" (string) category idnumber, "visible" (int) whether the category is visible or not, "theme" (string) category theme' All other changes look good to me. You can add your branch to the issue and I'll submit our team work It's the first time I see a three contributions issue
          Hide
          Jérôme Mouneyrac added a comment -

          Submitting. THanks Juan.

          Show
          Jérôme Mouneyrac added a comment - Submitting. THanks Juan.
          Hide
          Jérôme Mouneyrac added a comment -

          Ah I noticed Fabio's commit and my commit disappeared from the history. Juan can you put them back into your branch? Fabio and I spent a lot of time on them so it's important to keep it

          PS: I can not reject this issue from integration, I don't have the permissions, but you still can do the change to your branch before it gets reviewed (integrators will not know).

          Show
          Jérôme Mouneyrac added a comment - Ah I noticed Fabio's commit and my commit disappeared from the history. Juan can you put them back into your branch? Fabio and I spent a lot of time on them so it's important to keep it PS: I can not reject this issue from integration, I don't have the permissions, but you still can do the change to your branch before it gets reviewed (integrators will not know).
          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 - - edited

          Integrator blames (positively): grrr, it's like the 6th integration, each time adding more and more problems Now:

          • git conflict.
          • formatting.
          • using array_merge() for int-indexed arrays => lead to duplicates (use "+" instead).
          • troublemaker 0 != '' evaluation. (set it initially to null and check later with isset($value).
          • not ordering by path in ALL queries => causes the exclusion to work wrongly.
          • code checker warns and errors.
          • the final array, depending of the criteria used can be 100% disordered, so surely sorting the whole resulting array (by path, or by sortorder) could be necessary. This problem has been introduced by the current possibility of having N top categories.

          I was trying to fix all them with one extra commit. But finally have abandoned after 2h, sorry. Reopening.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Integrator blames (positively): grrr, it's like the 6th integration, each time adding more and more problems Now: git conflict. formatting. using array_merge() for int-indexed arrays => lead to duplicates (use "+" instead). troublemaker 0 != '' evaluation. (set it initially to null and check later with isset($value). not ordering by path in ALL queries => causes the exclusion to work wrongly. code checker warns and errors. the final array, depending of the criteria used can be 100% disordered, so surely sorting the whole resulting array (by path, or by sortorder) could be necessary. This problem has been introduced by the current possibility of having N top categories. I was trying to fix all them with one extra commit. But finally have abandoned after 2h, sorry. Reopening.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          You can get your 3 original commits (cherry-picked to fix conflicts) and my commit (fixing some of the points above, but NOT all) here:

          https://github.com/stronk7/moodle/commits/MDL-30084

          just in case you want to continue based on that...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - You can get your 3 original commits (cherry-picked to fix conflicts) and my commit (fixing some of the points above, but NOT all) here: https://github.com/stronk7/moodle/commits/MDL-30084 just in case you want to continue based on that...ciao
          Hide
          Martin Dougiamas added a comment -

          sigh

          Show
          Martin Dougiamas added a comment - sigh
          Hide
          Juan Leyva added a comment -

          Hi Jeromee,

          how are you thinking in solving this?

          Eloy already solved these:

          • Multiple coding violations.
          • Better detection of criteria, avoiding troublemaker 0 != ''
          • Minor formatting.
          • Ordering by path always to cause excludes to work ok.

          But since I only added the conditions part of the function (sorry for some of the coding violations, I submitted the non code checker final version) I don't know the rest of the code

          I think that the next step should be that Fabio cherry pick all the commits in an updated new branch and fix the rest of Eloy's comments

          What do you think?

          Regards

          Show
          Juan Leyva added a comment - Hi Jeromee, how are you thinking in solving this? Eloy already solved these: Multiple coding violations. Better detection of criteria, avoiding troublemaker 0 != '' Minor formatting. Ordering by path always to cause excludes to work ok. But since I only added the conditions part of the function (sorry for some of the coding violations, I submitted the non code checker final version) I don't know the rest of the code I think that the next step should be that Fabio cherry pick all the commits in an updated new branch and fix the rest of Eloy's comments What do you think? Regards
          Hide
          Jérôme Mouneyrac added a comment - - edited

          working on it... There was a wrong cheery-pick on the way, here is the current correct work in progress branch now: https://github.com/mouneyrac/moodle/commits/MDL-30084-good-commits

          Show
          Jérôme Mouneyrac added a comment - - edited working on it... There was a wrong cheery-pick on the way, here is the current correct work in progress branch now: https://github.com/mouneyrac/moodle/commits/MDL-30084-good-commits
          Hide
          Eloy Lafuente (stronk7) added a comment -

          two supper minor things:

          1) as far as we are ordering by path before processing excluded... the "Retrieve all categories in the database" does not need to have the ORDER BY path anymore.

          2) the ability to search by visible/not visible categories... perhaps it only has sense to people having 'moodle/category:viewhiddencategories' @ system or 'moodle/category:manage' @ context, because in fact they are not going to get those categories without those permissions. So I think it requires some capability check in the criteria "switch" code.

          With those implemented... if testing is covering visible/invisible combinations and other cases... I think this can be sent to integration again, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - two supper minor things: 1) as far as we are ordering by path before processing excluded... the "Retrieve all categories in the database" does not need to have the ORDER BY path anymore. 2) the ability to search by visible/not visible categories... perhaps it only has sense to people having 'moodle/category:viewhiddencategories' @ system or 'moodle/category:manage' @ context, because in fact they are not going to get those categories without those permissions. So I think it requires some capability check in the criteria "switch" code. With those implemented... if testing is covering visible/invisible combinations and other cases... I think this can be sent to integration again, thanks!
          Hide
          Jérôme Mouneyrac added a comment -

          Submitting for integration review.

          We'll still have to properly deal with exceptions. In the all Moodle code we have different exception everywhere: some translated, some not translated, core error == exception name ... but really can't we have more accurate exception code error when the exception come from the core lib function, some with debuginfo. You can participate to help defining http://docs.moodle.org/dev/Errors_handling_in_web_services so developers have a document to follow.

          Show
          Jérôme Mouneyrac added a comment - Submitting for integration review. We'll still have to properly deal with exceptions. In the all Moodle code we have different exception everywhere: some translated, some not translated, core error == exception name ... but really can't we have more accurate exception code error when the exception come from the core lib function, some with debuginfo. You can participate to help defining http://docs.moodle.org/dev/Errors_handling_in_web_services so developers have a document to follow.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Please have a look to http://docs.moodle.org/dev/Talk:Errors_handling_in_web_services for talking about exception.

          Show
          Jérôme Mouneyrac added a comment - - edited Please have a look to http://docs.moodle.org/dev/Talk:Errors_handling_in_web_services for talking about exception.
          Hide
          Jérôme Mouneyrac added a comment -

          I removed web service exception translation. Eloy can you check the Moodledocs and tell me what you think of both exception and warnings format.

          Show
          Jérôme Mouneyrac added a comment - I removed web service exception translation. Eloy can you check the Moodledocs and tell me what you think of both exception and warnings format.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Jerome... about exceptions I really think that WS could be a bit clever and always differentiate "errorcode" and "message", with the former being always the "stringkey" passed to the exception, and the later, "message", the one calculated by get_exception_info(), that can be translated or no.

          That way, WS client developers would be able to, always, rely in the "errorcode" for handling wrong situation as far as it's an untranslated code 100% guarantied. In the other side, message may be translated or may be not, so it's not reliable for any process but, perhaps, for displaying it when the expected behavior is that (show the error). Ignore else.

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Jerome... about exceptions I really think that WS could be a bit clever and always differentiate "errorcode" and "message", with the former being always the "stringkey" passed to the exception, and the later, "message", the one calculated by get_exception_info(), that can be translated or no. That way, WS client developers would be able to, always, rely in the "errorcode" for handling wrong situation as far as it's an untranslated code 100% guarantied. In the other side, message may be translated or may be not, so it's not reliable for any process but, perhaps, for displaying it when the expected behavior is that (show the error). Ignore else.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          About this issue, I think it looks perfect.

          Although I can imagine that there will be huge conflicts with the rest of course_category webservices (create, update, delete and this).

          So I'd propose to:

          1) create one unique patch with the 4 webservices (MDL-30081, MDL-30082, MDL-30083 and MDL-30084), and the 4 testing needed explained (note right now I don't know if the testclient web interface is something expected to work or no coz I've not get it working). Provide it here.

          2) link the other 3 issues with this ("will be fixed by MDL-30084"). And add one comment: "For integrators: code and testing for this is @ MDL-30084") on them.

          3) Submit the 4 issues for integration.

          Then:

          A) The 4 issues will be integrated and tested as a whole.

          B) It will land before freeze (next week is the last chance before freeze).

          How does it sound (I really think that the four together is the only way we have to land all this next week).

          So, I'm reopening this in order to get everything above done... ciao

          Show
          Eloy Lafuente (stronk7) added a comment - About this issue, I think it looks perfect. Although I can imagine that there will be huge conflicts with the rest of course_category webservices (create, update, delete and this). So I'd propose to: 1) create one unique patch with the 4 webservices ( MDL-30081 , MDL-30082 , MDL-30083 and MDL-30084 ), and the 4 testing needed explained (note right now I don't know if the testclient web interface is something expected to work or no coz I've not get it working). Provide it here. 2) link the other 3 issues with this ("will be fixed by MDL-30084 "). And add one comment: "For integrators: code and testing for this is @ MDL-30084 ") on them. 3) Submit the 4 issues for integration. Then: A) The 4 issues will be integrated and tested as a whole. B) It will land before freeze (next week is the last chance before freeze). How does it sound (I really think that the four together is the only way we have to land all this next week). So, I'm reopening this in order to get everything above done... ciao
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks Eloy. I'll work on a merge of all these commits.

          About exception, here it's how I came to the specs:
          many exceptions are thrown by core lib functions. So we only can stick with "class name"/"message"/"debuginfo". I don't think we are going to try catch everything from the server side, it seems wrong to overwrite exception => changing current structure no possible.

          So I wondered: "how we could get an error code between these three infos that are returned to the client". "class name" does work as a error code but it is super generic. Many 'moodle_exception', it doesn't help much the client. "message" is described as a untranslated and stable string. However many exceptions are thrown by core lib functions and they are all translated. So the client can't rely on them yet. However I think is the way to go, as it seems to be more a core exception issue which should not automatically translate the exception.

          Finally "debuginfo" is not sent in production site. Which mean the client code will not rely on it at all. It should be a untranslated message with some helpful information for the dev.

          In conclusion: if I'm writing a client I would not tweak my code following the returned exceptions. I'll try to avoid all of them, are they should be exceptional anyway. If you must tweak you client, it should be on warnings which are returned with other correct returned values.

          Show
          Jérôme Mouneyrac added a comment - Thanks Eloy. I'll work on a merge of all these commits. About exception, here it's how I came to the specs: many exceptions are thrown by core lib functions. So we only can stick with "class name"/"message"/"debuginfo". I don't think we are going to try catch everything from the server side, it seems wrong to overwrite exception => changing current structure no possible. So I wondered: "how we could get an error code between these three infos that are returned to the client". "class name" does work as a error code but it is super generic. Many 'moodle_exception', it doesn't help much the client. "message" is described as a untranslated and stable string. However many exceptions are thrown by core lib functions and they are all translated. So the client can't rely on them yet. However I think is the way to go, as it seems to be more a core exception issue which should not automatically translate the exception. Finally "debuginfo" is not sent in production site. Which mean the client code will not rely on it at all. It should be a untranslated message with some helpful information for the dev. In conclusion: if I'm writing a client I would not tweak my code following the returned exceptions. I'll try to avoid all of them, are they should be exceptional anyway. If you must tweak you client, it should be on warnings which are returned with other correct returned values.
          Hide
          Jérôme Mouneyrac added a comment -

          After reading Eloy comment again, I think you suggest to change the servers to return the exception 'errorcode' too. I don't think it would break many REST clients. Only the REST clients that validate the current Moodle exception format would fail. As for SOAP/XMLRPC we most likely need to change moodle_zend_soap/xmlrpc_server::fault().

          I suppose we could add like a serverversion param to each web service call?

          However for the moment, I stand on my previous conclusion => no change in our code, all external function exception are fully untranslated, the client dev does not expect exceptions to occur and does not implement behavior on returned exception. (S)He implements behavior on warnings.

          Show
          Jérôme Mouneyrac added a comment - After reading Eloy comment again, I think you suggest to change the servers to return the exception 'errorcode' too. I don't think it would break many REST clients. Only the REST clients that validate the current Moodle exception format would fail. As for SOAP/XMLRPC we most likely need to change moodle_zend_soap/xmlrpc_server::fault(). I suppose we could add like a serverversion param to each web service call? However for the moment, I stand on my previous conclusion => no change in our code, all external function exception are fully untranslated, the client dev does not expect exceptions to occur and does not implement behavior on returned exception. (S)He implements behavior on warnings.
          Hide
          Jérôme Mouneyrac added a comment -

          Keep going web service function discussion on MDL-32941

          Show
          Jérôme Mouneyrac added a comment - Keep going web service function discussion on MDL-32941
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This has been integrated and tested @ MDL-32941. Yay!

          Show
          Eloy Lafuente (stronk7) added a comment - This has been integrated and tested @ MDL-32941 . Yay!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: