Moodle
  1. Moodle
  2. MDL-29276 META- Web service improvements for 2.2
  3. MDL-29279

Making the REST server return JSON values if the new "alt" param is set to "json".

    Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0, 2.1, 2.2
    • Fix Version/s: 2.2
    • Component/s: Web Services
    • Labels:
    • Testing Instructions:
      Hide

      Download Moodle web service clients: https://github.com/moodlehq/sample-ws-clients
      Edit the REST client to support json
      Run the REST client

      Note: these test clients are very new. So if anything isn't straight forward, please improve them and add more information in their phpdoc.

      Show
      Download Moodle web service clients: https://github.com/moodlehq/sample-ws-clients Edit the REST client to support json Run the REST client Note: these test clients are very new. So if anything isn't straight forward, please improve them and add more information in their phpdoc.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull Master Branch:
    • Rank:
      19042

      Description

      Making the REST server return JSON values if the new "alt" param is set to "json". Otherwise the REST server still keep returning the current XML.

        Activity

        Hide
        Dongsheng Cai added a comment -

        Peer reviewed, looks good to me, just need to format the if-else statement correctly:
        https://github.com/mouneyrac/moodle/compare/master...MDL-29279#L0R94

        Show
        Dongsheng Cai added a comment - Peer reviewed, looks good to me, just need to format the if-else statement correctly: https://github.com/mouneyrac/moodle/compare/master...MDL-29279#L0R94
        Hide
        Jérôme Mouneyrac added a comment -

        Thanks Dongsheng. I fixed the indentation.

        Show
        Jérôme Mouneyrac added a comment - Thanks Dongsheng. I fixed the indentation.
        Hide
        Petr Škoda added a comment -

        where is the code?

        Show
        Petr Škoda added a comment - where is the code?
        Hide
        Jérôme Mouneyrac added a comment -

        Code is back

        Show
        Jérôme Mouneyrac added a comment - Code is back
        Hide
        Petr Škoda added a comment -

        Why are you removing only

        unset($_REQUEST['restformat']);

        ? Why not also _GET and _POST?
        Are you sure no custom external lib function ever used 'restformat' parameter? Would it be possible to use something more unique?

        Show
        Petr Škoda added a comment - Why are you removing only unset($_REQUEST['restformat']); ? Why not also _GET and _POST? Are you sure no custom external lib function ever used 'restformat' parameter? Would it be possible to use something more unique?
        Hide
        Sam Hemelryk added a comment -

        Hi Jerome,

        I've been having a look at this now.
        In regards to Petr's comments I certainly agree that restformat should be more unique, what about `wsrestformat` at a glance that looks like it would be more inline with the other vars used by the rest server. In fact would it be a better idea if that was collected by parse_request like the other params?
        As for the unsetting of $_REQUEST I agree with Petr in that it would be best to unset it from post and get as well, however I see looking at the webservice code that it routinely uses and then deletes from $_REQUEST so I would be happy for that to go in as it is presently providing a new bug is created to clean that up throughout the webservice code (which may be a better idea changing that everywhere at once).

        Let me know what you think - I'll leave this in the current sprint for the time being and if you clean up the naming of that string it'd get my +1.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Jerome, I've been having a look at this now. In regards to Petr's comments I certainly agree that restformat should be more unique, what about `wsrestformat` at a glance that looks like it would be more inline with the other vars used by the rest server. In fact would it be a better idea if that was collected by parse_request like the other params? As for the unsetting of $_REQUEST I agree with Petr in that it would be best to unset it from post and get as well, however I see looking at the webservice code that it routinely uses and then deletes from $_REQUEST so I would be happy for that to go in as it is presently providing a new bug is created to clean that up throughout the webservice code (which may be a better idea changing that everywhere at once). Let me know what you think - I'll leave this in the current sprint for the time being and if you clean up the naming of that string it'd get my +1. Cheers Sam
        Hide
        Sam Hemelryk added a comment -

        Hi Jerome - just reopening this issue so that when you're done with the changes you can put it back up and we will know to look at it again

        Show
        Sam Hemelryk added a comment - Hi Jerome - just reopening this issue so that when you're done with the changes you can put it back up and we will know to look at it again
        Hide
        Jérôme Mouneyrac added a comment -

        I did the change Sam, you can integrate. Thanks both of you for reviewing

        Show
        Jérôme Mouneyrac added a comment - I did the change Sam, you can integrate. Thanks both of you for reviewing
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Integrated (master only).

        I must recognize that:

        • I accepted it in "blind" mode. Not sure how that works.
        • I don't like having to unset those RPGC variables that way. Cannot we have some central place to ban all them following some rule before arriving to lower levels (Zend?). Or doesn't Zend provide some facility to keep out some RPGC ?
        Show
        Eloy Lafuente (stronk7) added a comment - Integrated (master only). I must recognize that: I accepted it in "blind" mode. Not sure how that works. I don't like having to unset those RPGC variables that way. Cannot we have some central place to ban all them following some rule before arriving to lower levels (Zend?). Or doesn't Zend provide some facility to keep out some RPGC ?
        Hide
        Sam Hemelryk added a comment -

        Thanks Jerome this has been tested now.
        Please note that you'll need to update the test client as some point now as param restformat has of course being changed.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Thanks Jerome this has been tested now. Please note that you'll need to update the test client as some point now as param restformat has of course being changed. Cheers Sam
        Hide
        Jérôme Mouneyrac added a comment -

        Eloy:
        a) server.php level wants to know about 'moodlewsrestformat'
        b) authentication level wants to know about 'wsfunction' / 'wstoken'
        c) Zend server read all the params
        d) valid_param() checks all params are valid against the function description (so we don't want 'moodlewsrestformat' / 'wsfunction' / 'wstoken')

        What are the benefits to introduce a banning rule system? Wouldn't it make the web service structure more complex? I'm not against it, I just don't know.

        Sam:
        it's done.

        Thank you all

        Show
        Jérôme Mouneyrac added a comment - Eloy: a) server.php level wants to know about 'moodlewsrestformat' b) authentication level wants to know about 'wsfunction' / 'wstoken' c) Zend server read all the params d) valid_param() checks all params are valid against the function description (so we don't want 'moodlewsrestformat' / 'wsfunction' / 'wstoken') What are the benefits to introduce a banning rule system? Wouldn't it make the web service structure more complex? I'm not against it, I just don't know. Sam: it's done. Thank you all
        Hide
        Eloy Lafuente (stronk7) added a comment -

        And this code has been spread to all Moodle git and cvs repositories. Many thanks! Closing.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - And this code has been spread to all Moodle git and cvs repositories. Many thanks! Closing. Ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: