Moodle
  1. Moodle
  2. MDL-32581

All textarea/DB TEXT should be joined by format field (params and return)

    Details

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

      Take the REST web service demo client. Your goal is to test all the description/summary fields in these functions:

      • core_course_get_categories()
      • core_course_create_categories()

      All the description/summary should be returned with an additional format field. Format field should be default on INPUT so you don't have to set them if you don't want.

      You also should be able to call the REST server with additional parameters. See http://docs.moodle.org/dev/Creating_a_web_service_client#Moodle_2.3_and_later. Check that core_course_get_categories() can return raw text, can not rewrite file url, or even can filter the text.

      Show
      Take the REST web service demo client. Your goal is to test all the description/summary fields in these functions: core_course_get_categories() core_course_create_categories() All the description/summary should be returned with an additional format field. Format field should be default on INPUT so you don't have to set them if you don't want. You also should be able to call the REST server with additional parameters. See http://docs.moodle.org/dev/Creating_a_web_service_client#Moodle_2.3_and_later . Check that core_course_get_categories() can return raw text, can not rewrite file url, or even can filter the text.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
    • Rank:
      39497

      Description

      Write documentation specs: http://docs.moodle.org/dev/How_to_contribute_a_web_service_function_to_core#Create_a_tracker_issue

      Fix all the existing.

      This concern 2.3 only.

        Issue Links

          Activity

          Hide
          Jérôme Mouneyrac added a comment - - edited

          I updated the documentation and pushed the fix. Currently testing.

          Note: for the message and note functions I was not sure if I should fix the fact that we return not standard and wrong warnings (they should be exception). I mentioned it in the phpdoc. I don't think it would break much client to change it for exception. At the end it's what they should be, exceptional. => I'm most likely going to do the change in another issue as I really think it would be good to be consistent.

          Show
          Jérôme Mouneyrac added a comment - - edited I updated the documentation and pushed the fix. Currently testing. Note: for the message and note functions I was not sure if I should fix the fact that we return not standard and wrong warnings (they should be exception). I mentioned it in the phpdoc. I don't think it would break much client to change it for exception. At the end it's what they should be, exceptional. => I'm most likely going to do the change in another issue as I really think it would be good to be consistent.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          core_course_get_contents: course module descriptions are automatically filtered by format_module_intro (called by their respective COURSEMODULE_get_coursemodule_info). The introformat is never returned/known. This could deserve it's own issue. Maybe need some $cm->get_content_format();

          Show
          Jérôme Mouneyrac added a comment - - edited core_course_get_contents: course module descriptions are automatically filtered by format_module_intro (called by their respective COURSEMODULE_get_coursemodule_info). The introformat is never returned/known. This could deserve it's own issue. Maybe need some $cm->get_content_format();
          Hide
          Jérôme Mouneyrac added a comment -

          Note I switch two default value from FORMAT_MOODLE to FORMAT_HTML.

          Show
          Jérôme Mouneyrac added a comment - Note I switch two default value from FORMAT_MOODLE to FORMAT_HTML.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Jerome,

          perhaps, instead of using $CFG for storage we could implement some sort of external_settings class (static, singleton or whatever) so we can perform simple calls like:

          external_settings::return_raw(), ::process_files(), ::apply_filters() and then use them in the external functions?

          That way, the API (the 3 functions and any other coming in the future) will be well-defined and self-contained in that class, in case we want to change the login later (imagine we decide to make them user preferences, or part of the service definition...). By providing one object to handle them, we won't need to recode and recode all the functions.

          Also, I hope (I really don't know) it it's "legal" to pass those extra post/get parameters in all the servers, is it? Are we free to do that in SOAP/XMLRPC ?

          Of course, I assume that you are keeping BC in case no setting is passed.

          One more little detail... do we need really those long names? Perhaps we could use the name of the methods above ('return_raw', ....) as shorter and clearer alternatives? Or simpler yet ('raw', ...) ? For your consideration.

          And finally, surely we should be validating those GET/POST params, why not use optional_param() and PARAM_BOOL or so? Perhaps it's not allowed there, but that's exactly what we should be looking for.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Jerome, perhaps, instead of using $CFG for storage we could implement some sort of external_settings class (static, singleton or whatever) so we can perform simple calls like: external_settings::return_raw(), ::process_files(), ::apply_filters() and then use them in the external functions? That way, the API (the 3 functions and any other coming in the future) will be well-defined and self-contained in that class, in case we want to change the login later (imagine we decide to make them user preferences, or part of the service definition...). By providing one object to handle them, we won't need to recode and recode all the functions. Also, I hope (I really don't know) it it's "legal" to pass those extra post/get parameters in all the servers, is it? Are we free to do that in SOAP/XMLRPC ? Of course, I assume that you are keeping BC in case no setting is passed. One more little detail... do we need really those long names? Perhaps we could use the name of the methods above ('return_raw', ....) as shorter and clearer alternatives? Or simpler yet ('raw', ...) ? For your consideration. And finally, surely we should be validating those GET/POST params, why not use optional_param() and PARAM_BOOL or so? Perhaps it's not allowed there, but that's exactly what we should be looking for. Ciao
          Hide
          Jérôme Mouneyrac added a comment -

          Arh Jira session expired... lost my comment... in summary:

          • parameters are optional (most of the time the client devs will not use them)
          • BC
          • SOAP/XMLRPC servers already receive the wstoken parameter in GET/POST, so additional GET/POST parameters will not change much more about being standard or not.
          • I used long name to avoid conflict with external function parameters. It's most likely that someone will create a web service with a param named 'return_raw'. However I agree they are a tiny bit long

          My main concern is about the object thing (external_settings), where do you keep it in memory (passing it as parameter doesn't seems to be a good idea to me as it complicates all the design)?

          Show
          Jérôme Mouneyrac added a comment - Arh Jira session expired... lost my comment... in summary: parameters are optional (most of the time the client devs will not use them) BC SOAP/XMLRPC servers already receive the wstoken parameter in GET/POST, so additional GET/POST parameters will not change much more about being standard or not. I used long name to avoid conflict with external function parameters. It's most likely that someone will create a web service with a param named 'return_raw'. However I agree they are a tiny bit long My main concern is about the object thing (external_settings), where do you keep it in memory (passing it as parameter doesn't seems to be a good idea to me as it complicates all the design)?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Aha, reviewing last version, implementing external_settings storage and request interception:

          1) Take a look to http://stronk7.doesntexist.com/job/Precheck%20remote%20branch/238/artifact/work/smurf.xml . Apart from the warnings (weight = 1), it includes a bunch of things that should be fixed.

          2) The format "helpstring" (embed in the xxx_parameters methods) is a bit repetitive. Perhaps it would be worth having it defined somewhere and reuse such define like crazy. Not critical, anyway.

          3) Note I've not looked to all changes @ detail but overall they have sense.

          4) We need to spend some time covering all these external functions with some phpunit tests asasp. Keeping the WS layer apart, just the external stuff. Let's prospect/discuss about that once this has landed.

          +1 to send it to integration. Thanks for all the iterations! Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Aha, reviewing last version, implementing external_settings storage and request interception: 1) Take a look to http://stronk7.doesntexist.com/job/Precheck%20remote%20branch/238/artifact/work/smurf.xml . Apart from the warnings (weight = 1), it includes a bunch of things that should be fixed. 2) The format "helpstring" (embed in the xxx_parameters methods) is a bit repetitive. Perhaps it would be worth having it defined somewhere and reuse such define like crazy. Not critical, anyway. 3) Note I've not looked to all changes @ detail but overall they have sense. 4) We need to spend some time covering all these external functions with some phpunit tests asasp. Keeping the WS layer apart, just the external stuff. Let's prospect/discuss about that once this has landed. +1 to send it to integration. Thanks for all the iterations! Ciao
          Hide
          Jérôme Mouneyrac added a comment - - edited

          0) I switch the external_warnings function for a external_warnings class, it will be much cleaner/maintainable.
          1) done
          2) I add a external_format_value extending external_value that prefills everything for text format.
          4) definitively in my list of TODO very soon

          Show
          Jérôme Mouneyrac added a comment - - edited 0) I switch the external_warnings function for a external_warnings class, it will be much cleaner/maintainable. 1) done 2) I add a external_format_value extending external_value that prefills everything for text format. 4) definitively in my list of TODO very soon
          Hide
          Aparup Banerjee added a comment -

          attaching a smurf.xml run on current patch (Jerome's request)

          Show
          Aparup Banerjee added a comment - attaching a smurf.xml run on current patch (Jerome's request)
          Hide
          Jérôme Mouneyrac added a comment -

          Maybe I should not have edited my previous comment but put a new one to make it more clear. Change done Eloy Writing the PHPUnit tests (MDL-33478)

          Show
          Jérôme Mouneyrac added a comment - Maybe I should not have edited my previous comment but put a new one to make it more clear. Change done Eloy Writing the PHPUnit tests ( MDL-33478 )
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Dan Poltawski added a comment -

          Thanks Jerome, looks good and integrated.

          Show
          Dan Poltawski added a comment - Thanks Jerome, looks good and integrated.
          Hide
          Jérôme Mouneyrac added a comment -

          Rosie detected an error:

          Undefined variable: type in /integration/master/lib/externallib.php on line 647

          which is quite bad. I need to change the constructor of external_format_value class.

          Sorry.

          Show
          Jérôme Mouneyrac added a comment - Rosie detected an error: Undefined variable: type in /integration/master/lib/externallib.php on line 647 which is quite bad. I need to change the constructor of external_format_value class. Sorry.
          Hide
          Aparup Banerjee added a comment -

          Jerome is working on a fix for this atm.

          Show
          Aparup Banerjee added a comment - Jerome is working on a fix for this atm.
          Hide
          Aparup Banerjee added a comment -

          https://github.com/mouneyrac/moodle/commit/6a709920a58d8dca458b8d2315e2dca0677c2ce0 has been integrated into master now. reopening for testing.

          Show
          Aparup Banerjee added a comment - https://github.com/mouneyrac/moodle/commit/6a709920a58d8dca458b8d2315e2dca0677c2ce0 has been integrated into master now. reopening for testing.
          Hide
          Rossiani Wijaya added a comment -

          This looks good Jerome.

          Test passed.

          Show
          Rossiani Wijaya added a comment - This looks good Jerome. Test passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          We could celebrate it today... but better if we perform a bigger party after releasing Moodle 2.3.

          Print this message and come to Perth that day, it's valid for one beer, wine, coke or... water, as you wish.

          Many thanks for your collaboration!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - We could celebrate it today... but better if we perform a bigger party after releasing Moodle 2.3. Print this message and come to Perth that day, it's valid for one beer, wine, coke or... water, as you wish. Many thanks for your collaboration! Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: