Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-30126

Our rest client should be updated fo json support

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2, 2.3
    • Fix Version/s: 2.2.2
    • Component/s: Web Services
    • Labels:

      Gliffy Diagrams

        Attachments

          Issue Links

            Activity

            Hide
            nebgor Aparup Banerjee added a comment -

            hi, this looks fine to me.
            I have noted however that we're back to building urls manually when we have the moodle_url to build urls with. since you're in the area, Jerome, you might want to consider cleaning that up.

            Show
            nebgor Aparup Banerjee added a comment - hi, this looks fine to me. I have noted however that we're back to building urls manually when we have the moodle_url to build urls with. since you're in the area, Jerome, you might want to consider cleaning that up.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Hi Apu, thanks for the peer-review. What do you mean by clean up the aera? In the meantime I submit this issue for integration. If something else need to be done we can still open a new issue. I would need this issue integrated to make the web service unit test work with the REST server

            Show
            jerome Jérôme Mouneyrac added a comment - Hi Apu, thanks for the peer-review. What do you mean by clean up the aera? In the meantime I submit this issue for integration. If something else need to be done we can still open a new issue. I would need this issue integrated to make the web service unit test work with the REST server
            Hide
            nebgor Aparup Banerjee added a comment -

            ah i meant in the constructor, $serverurl is a moodle_url so

            $result = download_file_content($this->serverurl. '?wstoken='.$this->token. $formatparam . '&wsfunction='. $functionname, null, $params);

            could be

             if ($this->format == 'json') {
              $this->serverurl->param('moodlewsrestformat','json');
             }
             $this->serverurl->param('wstoken',$this->token);
             $this->serverurl->param('wsfunction',$functionname); //you could also use params().
             
             $result = download_file_content($this->serverurl, null, $params);

            Show
            nebgor Aparup Banerjee added a comment - ah i meant in the constructor, $serverurl is a moodle_url so $result = download_file_content($this->serverurl. '?wstoken='.$this->token. $formatparam . '&wsfunction='. $functionname, null, $params); could be if ($this->format == 'json') { $this->serverurl->param('moodlewsrestformat','json'); } $this->serverurl->param('wstoken',$this->token); $this->serverurl->param('wsfunction',$functionname); //you could also use params().   $result = download_file_content($this->serverurl, null, $params);
            Hide
            stronk7 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
            stronk7 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
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Ho, I'm integrating this but agree with Aparup that the urls should be constructed using our moodle_url class. Please consider filling one issue about that.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Ho, I'm integrating this but agree with Aparup that the urls should be constructed using our moodle_url class. Please consider filling one issue about that.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Big Q!

            This seems to be one improvement for master only, but is blocking currently to MDL-30210 (where both the 22 and master patches are the same).

            So... should this also be backported to 22 ? Plz, illustrate us, TIA and ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Big Q! This seems to be one improvement for master only, but is blocking currently to MDL-30210 (where both the 22 and master patches are the same). So... should this also be backported to 22 ? Plz, illustrate us, TIA and ciao
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Ah yes it should be in 2.2. I'll backport it and apply the suggested fix during my next ws week

            Show
            jerome Jérôme Mouneyrac added a comment - Ah yes it should be in 2.2. I'll backport it and apply the suggested fix during my next ws week
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Sorry but... does that mean that we must halt integration of both this and MDL-30210 till the next ws week ?

            Or that we should apply both this and MDL-30210 only to master?

            Or apply this only to master and MDL-30210 to all the branches (knowing in will fail-testing under 22_STABLE)?

            Please clarify, I'm a bit lost right now (surely a good moment to go sleep a bit, hehe). TIA!

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Sorry but... does that mean that we must halt integration of both this and MDL-30210 till the next ws week ? Or that we should apply both this and MDL-30210 only to master? Or apply this only to master and MDL-30210 to all the branches (knowing in will fail-testing under 22_STABLE)? Please clarify, I'm a bit lost right now (surely a good moment to go sleep a bit, hehe). TIA! Ciao
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Ah sorry Eloy Send me back both of them to development, I'll send them back to you next week. It's easier for you I guess.

            Show
            jerome Jérôme Mouneyrac added a comment - Ah sorry Eloy Send me back both of them to development, I'll send them back to you next week. It's easier for you I guess.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            (sending back to integration, as agreed, thanks!)

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - (sending back to integration, as agreed, thanks!)
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Fixed resubmitted.

            Show
            jerome Jérôme Mouneyrac added a comment - Fixed resubmitted.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            The rest server doesn't support JSON in 2.1. Anyway only if a developer set the REST protocol to true in the web service unit test file, then the tests will fail. When settings it up a message indacte this issue and the dev will understand that REST unit test doesn't work on 2.1.

            Show
            jerome Jérôme Mouneyrac added a comment - The rest server doesn't support JSON in 2.1. Anyway only if a developer set the REST protocol to true in the web service unit test file, then the tests will fail. When settings it up a message indacte this issue and the dev will understand that REST unit test doesn't work on 2.1.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Oki, integrating this for 22 and master... thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Oki, integrating this for 22 and master... thanks!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated, thanks (22 and master).

            Now I'm going to integrate and test MDL-30210 that will cause this to pass (or no).

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated, thanks (22 and master). Now I'm going to integrate and test MDL-30210 that will cause this to pass (or no). Ciao
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Uhm,

            I've got some problems running MDL-30210 (all protocols and functions enabled).

            Then I've edited webservice/simpletest/testwebservice.php, enabling only REST and 3 functions (moodle_course_get_courses, moodle_user_get_users_by_id, core_enrol_get_enrolled_users).

            And running unit-tests under /webservice I get:

            1/1 test cases complete: 53 passes, 0 fails and 0 exceptions.

            Is that enough to consider the test passed?

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Uhm, I've got some problems running MDL-30210 (all protocols and functions enabled). Then I've edited webservice/simpletest/testwebservice.php, enabling only REST and 3 functions (moodle_course_get_courses, moodle_user_get_users_by_id, core_enrol_get_enrolled_users). And running unit-tests under /webservice I get: 1/1 test cases complete: 53 passes, 0 fails and 0 exceptions. Is that enough to consider the test passed?
            Hide
            jerome Jérôme Mouneyrac added a comment - - edited

            Sorry Eloy for making you test the unit test. I changed the testing instruction to make the issue not depend of the clunky unit test

            Show
            jerome Jérôme Mouneyrac added a comment - - edited Sorry Eloy for making you test the unit test. I changed the testing instruction to make the issue not depend of the clunky unit test
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Nice, thanks, that will help to close this... trying...

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Nice, thanks, that will help to close this... trying...
            Hide
            jerome Jérôme Mouneyrac added a comment -

            If it's too long to test you can send it to someone else for testing, or close it. I tested it multiple times with multiple functions.

            Show
            jerome Jérôme Mouneyrac added a comment - If it's too long to test you can send it to someone else for testing, or close it. I tested it multiple times with multiple functions.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Passed, I've tested json rest with both:

            • admin/webservice/testclient.php: Hacking it a bit to specify json format, and 3-4 random functions (there are a bunch missing, surely because of they missing its "form" definition or so). All them returned proper json.
            • sample-ws-clients/PHP-REST: By configuring it and execute some "core" functions, it also worked ok, both with formats being xml and json.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Passed, I've tested json rest with both: admin/webservice/testclient.php: Hacking it a bit to specify json format, and 3-4 random functions (there are a bunch missing, surely because of they missing its "form" definition or so). All them returned proper json. sample-ws-clients/PHP-REST: By configuring it and execute some "core" functions, it also worked ok, both with formats being xml and json. Ciao
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            It is late here and I'm very tired but I didn't want to go to sleep before expressing my admiration for your amazing collaboration. Thanks!

            Closing as fixed, heading to zzzZZZzzz, niao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - It is late here and I'm very tired but I didn't want to go to sleep before expressing my admiration for your amazing collaboration. Thanks! Closing as fixed, heading to zzzZZZzzz, niao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  12/Mar/12