Details

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

      Get SOAP and XMLRPC demo clients: https://github.com/moodlehq/sample-ws-clients

      Run one a first time. It will create two dummy users.
      Run them again. An exception is returned. If Debug is ON, it is saying the username is already used. If OFF it is just a generic error message.
      Test ON/OFF for both protocols.

      Show
      Get SOAP and XMLRPC demo clients: https://github.com/moodlehq/sample-ws-clients Run one a first time. It will create two dummy users. Run them again. An exception is returned. If Debug is ON, it is saying the username is already used. If OFF it is just a generic error message. Test ON/OFF for both protocols.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull Master Branch:

      Description

      We implemented set_exception_handler, but Zend servers actually catch all exceptions before that. We need to find a way to return the $debuginfo in the Zend servers.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Jérôme Mouneyrac added a comment -

            I extended the Zend servers and overwrote their fault() functions: https://github.com/mouneyrac/moodle/compare/master...MDL-29435

            I'll clean the previous set_exception_handler code (in our moodle_zend_servers) that is never called. I'll check if removing it is compatible with the AMF Zend server.

            Show
            Jérôme Mouneyrac added a comment - I extended the Zend servers and overwrote their fault() functions: https://github.com/mouneyrac/moodle/compare/master...MDL-29435 I'll clean the previous set_exception_handler code (in our moodle_zend_servers) that is never called. I'll check if removing it is compatible with the AMF Zend server.
            Hide
            Jérôme Mouneyrac added a comment -

            I checked the AMF server, it's returning an exception. The fault() function is not implemented in AMF server. So we should keep the previous code related to set_exception_handler, and anyway it would manage exception for any new Zend servers.

            Show
            Jérôme Mouneyrac added a comment - I checked the AMF server, it's returning an exception. The fault() function is not implemented in AMF server. So we should keep the previous code related to set_exception_handler, and anyway it would manage exception for any new Zend servers.
            Hide
            Petr Skoda added a comment -

            I do not think the use of moodle_exception is valid, isset() for debug info seems inappropriate too.

            Show
            Petr Skoda added a comment - I do not think the use of moodle_exception is valid, isset() for debug info seems inappropriate too.
            Hide
            Jérôme Mouneyrac added a comment -

            I guess you are saying it's not valid because any Zend exceptions would become Moodle exceptions? Are you suggesting to create a new exception? It would be okay to me, but what is the issue with using moodle_exception? The exception information stay the same which is what people who debug want to know.

            The moodle_exception is a lot better than the no debug info returned by our current Zend servers.

            For the isset(), you did the same in the REST server That seemed appropriate to me. (https://github.com/mouneyrac/moodle/commit/cc93c7da15bcd669959c7e554faab7c0b5c364ed#L21R94)

            Show
            Jérôme Mouneyrac added a comment - I guess you are saying it's not valid because any Zend exceptions would become Moodle exceptions? Are you suggesting to create a new exception? It would be okay to me, but what is the issue with using moodle_exception? The exception information stay the same which is what people who debug want to know. The moodle_exception is a lot better than the no debug info returned by our current Zend servers. For the isset(), you did the same in the REST server That seemed appropriate to me. ( https://github.com/mouneyrac/moodle/commit/cc93c7da15bcd669959c7e554faab7c0b5c364ed#L21R94 )
            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
            Petr Skoda added a comment -

            hmm:

                /**
                 * Constructor
                 * @param string $errorcode The name of the string from error.php to print
                 * @param string $module name of module
                 * @param string $link The url where the user will be prompted to continue. If no url is provided the user will be directed to the site index page.
                 * @param object $a Extra words and phrases that might be required in the error string
                 * @param string $debuginfo optional debugging information
                 */
                function __construct($errorcode, $module='', $link='', $a=NULL, $debuginfo=null) {
            

            and your:

            $fault = new moodle_exception($fault->getMessage() . ' | DEBUG INFO: ' . $fault->debuginfo);
            

            I did not try it myself, but it looks like it can not work. I suppose you did not test it, right?

            Show
            Petr Skoda added a comment - hmm: /** * Constructor * @param string $errorcode The name of the string from error.php to print * @param string $module name of module * @param string $link The url where the user will be prompted to continue. If no url is provided the user will be directed to the site index page. * @param object $a Extra words and phrases that might be required in the error string * @param string $debuginfo optional debugging information */ function __construct($errorcode, $module='', $link='', $a=NULL, $debuginfo=null) { and your: $fault = new moodle_exception($fault->getMessage() . ' | DEBUG INFO: ' . $fault->debuginfo); I did not try it myself, but it looks like it can not work. I suppose you did not test it, right?
            Hide
            Jérôme Mouneyrac added a comment -

            No I never test anything before to push, it's more fun. Of course I tested.

            Show
            Jérôme Mouneyrac added a comment - No I never test anything before to push, it's more fun. Of course I tested.
            Hide
            Petr Skoda added a comment -

            Then how is it possible that the constructor worked for you? The first parameter in moodle_exception constructor is "string $errorcode The name of the string" and you are using there some random text with extra debug info. Am I missing something?

            Show
            Petr Skoda added a comment - Then how is it possible that the constructor worked for you? The first parameter in moodle_exception constructor is "string $errorcode The name of the string" and you are using there some random text with extra debug info. Am I missing something?
            Hide
            Jérôme Mouneyrac added a comment -

            Because when the moodle_exception can not find the string, then it displays the text, that's all your are missing. From what I remember.

            Show
            Jérôme Mouneyrac added a comment - Because when the moodle_exception can not find the string, then it displays the text, that's all your are missing. From what I remember.
            Hide
            Petr Skoda added a comment -

            Are you serious?

            Show
            Petr Skoda added a comment - Are you serious?
            Hide
            Petr Skoda added a comment -

            to integrators: this is not suitable for integration into any branch

            Show
            Petr Skoda added a comment - to integrators: this is not suitable for integration into any branch
            Hide
            Sam Hemelryk added a comment -

            Certainly we are at a point where these debug improvements need to be discussed.

            Show
            Sam Hemelryk added a comment - Certainly we are at a point where these debug improvements need to be discussed.
            Hide
            Jérôme Mouneyrac added a comment -

            Running the code I pushed, with xmlrpc (but works also with SOAP):

            Array
            (
            [faultCode] => 0
            [faultString] => error/Invalid parameter value detected, execution can not continue. | DEBUG INFO: Username already exists: testusername1
            )

            As you can see, the error message is explicit to the user and the debug info is displayed. If debug info is off, the code will go through the current way the Zend server handle exception, so without debug info.

            It seemed to me a good solution, I'm sorry to not understand what Petr you are saying, please could you clarify why you don't like it.

            Show
            Jérôme Mouneyrac added a comment - Running the code I pushed, with xmlrpc (but works also with SOAP): Array ( [faultCode] => 0 [faultString] => error/Invalid parameter value detected, execution can not continue. | DEBUG INFO: Username already exists: testusername1 ) As you can see, the error message is explicit to the user and the debug info is displayed. If debug info is off, the code will go through the current way the Zend server handle exception, so without debug info. It seemed to me a good solution, I'm sorry to not understand what Petr you are saying, please could you clarify why you don't like it.
            Hide
            Petr Skoda added a comment -

            error/stringid is just a fallback, you must not send rubbish texts to our exception constructors that expect strinids, it would be the same like get_string('Some nice text message'). There is absolutely no reason to do hack like this when you can create new non-moodle exception that encapsulates the original moodle exception. There might be some other solution too...

            Show
            Petr Skoda added a comment - error/stringid is just a fallback, you must not send rubbish texts to our exception constructors that expect strinids, it would be the same like get_string('Some nice text message'). There is absolutely no reason to do hack like this when you can create new non-moodle exception that encapsulates the original moodle exception. There might be some other solution too...
            Hide
            Jérôme Mouneyrac added a comment -

            I'll create a new moodle_zend_exception where the debug info is added to the message, so Zend servers can return some debug infos.

            Show
            Jérôme Mouneyrac added a comment - I'll create a new moodle_zend_exception where the debug info is added to the message, so Zend servers can return some debug infos.
            Hide
            Sam Hemelryk added a comment -

            Hi guys,

            I've been studying our webservice code and the Zend server code for the past couple of hours in regards to these changes.

            I agree that abusing moodle_exception in such a way is not great, and that using another exception (perhaps a new one) is a solution. However I think we should try to keep things as close to the existing (parent) calls as possible.

            For the Soap server what if we use the SoapFault Exception class? This is the same Exception class returned by the parent fault method, we can use it in the exact way it is being used presently, its core PHP separate of both Moodle and Zend, and its the native exception class for the underlying calls.

            For the XMLRPC it's not so clear - obviously this doesn't have native support like Soap and as such Zend have there own custom exception class, I think abusing their exception class is about as incorrect as abusing our exception class. For this it'd get my +1 to use the closest to the expected exception type as possible so probably just a plain Jane Exception. What ever you do here you will need to register the exception class you use with the Zend XMLRPC Server.

            In general I'm not a huge fan of the solution - however given Zend don't support this debug info in their exception classes I think if we really do want to beef up the content of our exceptions then this is as good as it gets.

            Of course as Petr says we really shouldn't be abusing exceptions - exceptions should be exceptional things. Because everything in regards to the web services is laid out so well things like invalid param's only represent significant errors with the calling client. In the future it'd be great to try and move away from this sort of information exception and find a better solution whether its proper checking or whatever.

            As a side thought we could of course make proper use of the error code parameter and then log errors and provide that information through a report or subsequent request.
            Just ideas but that would allow us to take full debug information, provide and access key, and then require to user to authernticate/hold caps to access the information.

            Anyway it's 10pm here so I'm off.
            Cya

            Show
            Sam Hemelryk added a comment - Hi guys, I've been studying our webservice code and the Zend server code for the past couple of hours in regards to these changes. I agree that abusing moodle_exception in such a way is not great, and that using another exception (perhaps a new one) is a solution. However I think we should try to keep things as close to the existing (parent) calls as possible. For the Soap server what if we use the SoapFault Exception class? This is the same Exception class returned by the parent fault method, we can use it in the exact way it is being used presently, its core PHP separate of both Moodle and Zend, and its the native exception class for the underlying calls. For the XMLRPC it's not so clear - obviously this doesn't have native support like Soap and as such Zend have there own custom exception class, I think abusing their exception class is about as incorrect as abusing our exception class. For this it'd get my +1 to use the closest to the expected exception type as possible so probably just a plain Jane Exception. What ever you do here you will need to register the exception class you use with the Zend XMLRPC Server. In general I'm not a huge fan of the solution - however given Zend don't support this debug info in their exception classes I think if we really do want to beef up the content of our exceptions then this is as good as it gets. Of course as Petr says we really shouldn't be abusing exceptions - exceptions should be exceptional things. Because everything in regards to the web services is laid out so well things like invalid param's only represent significant errors with the calling client. In the future it'd be great to try and move away from this sort of information exception and find a better solution whether its proper checking or whatever. As a side thought we could of course make proper use of the error code parameter and then log errors and provide that information through a report or subsequent request. Just ideas but that would allow us to take full debug information, provide and access key, and then require to user to authernticate/hold caps to access the information. Anyway it's 10pm here so I'm off. Cya
            Hide
            Sam Hemelryk added a comment -

            Thanks Jerome - gets my +1.
            The only thing I noted was the new includes are relative paths.
            However I inspected the rest of WS code and see thats quite common.

            Cheers Sam

            Show
            Sam Hemelryk added a comment - Thanks Jerome - gets my +1. The only thing I noted was the new includes are relative paths. However I inspected the rest of WS code and see thats quite common. Cheers Sam
            Hide
            Jérôme Mouneyrac added a comment - - edited

            Thanks Sam.

            The Zend include is quite old. When I started using the Zend web service classes (I guess 2 years ago), the other Zend libraries were included this way in Moodle. In fact if I remember in the Zend framework tutorial, they include the library the same way. So I suppose that Zend library needs to be included this way, and we implemented some hook to do it. Being consistent I implemented the same way I submit for integration.

            Show
            Jérôme Mouneyrac added a comment - - edited Thanks Sam. The Zend include is quite old. When I started using the Zend web service classes (I guess 2 years ago), the other Zend libraries were included this way in Moodle. In fact if I remember in the Zend framework tutorial, they include the library the same way. So I suppose that Zend library needs to be included this way, and we implemented some hook to do it. Being consistent I implemented the same way I submit for integration.
            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
            Sam Hemelryk added a comment -

            Thanks Jerome, this has been integrated now

            Show
            Sam Hemelryk added a comment - Thanks Jerome, this has been integrated now
            Hide
            Jérôme Mouneyrac added a comment -

            Cool, thanks

            Show
            Jérôme Mouneyrac added a comment - Cool, thanks
            Hide
            Aparup Banerjee added a comment - - edited

            works for me, DEBUG INFO is left out when not in debug mode for RESt and XMLRPC and SOAP too!

            btw, when SOAP protocol wasn't allowed - SOAP couldn't return the access control exception - instead SOAPfaults started showing up in error log about WSDL link. following the WSDL link showed the access control exception - i'll make a separate issue for this.

            Show
            Aparup Banerjee added a comment - - edited works for me, DEBUG INFO is left out when not in debug mode for RESt and XMLRPC and SOAP too! btw, when SOAP protocol wasn't allowed - SOAP couldn't return the access control exception - instead SOAPfaults started showing up in error log about WSDL link. following the WSDL link showed the access control exception - i'll make a separate issue for this.
            Hide
            Aparup Banerjee added a comment -

            linking WSDL issue.

            Show
            Aparup Banerjee added a comment - linking WSDL issue.
            Hide
            Jérôme Mouneyrac added a comment - - edited

            Thanks Apu. Good catch, it's a missing part of the fix (we should also have extended the WSDL discovery class to return the same debug info). This issue can be closed as we created another issue for the missing part (MDL-29825).

            Show
            Jérôme Mouneyrac added a comment - - edited Thanks Apu. Good catch, it's a missing part of the fix (we should also have extended the WSDL discovery class to return the same debug info). This issue can be closed as we created another issue for the missing part ( MDL-29825 ).
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Many thanks for all the hard work. This is now part of Moodle, your favorite LMS.

            Closing as fixed, ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Many thanks for all the hard work. This is now part of Moodle, your favorite LMS. Closing as fixed, ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: