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:

      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.

        Gliffy Diagrams

          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: