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:
    • Rank:
      19043

      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.

        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 Škoda added a comment -

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

          Show
          Petr Škoda 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 Škoda 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 Škoda 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 Škoda 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 Škoda 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 Škoda added a comment -

          Are you serious?

          Show
          Petr Škoda added a comment - Are you serious?
          Hide
          Petr Škoda added a comment -

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

          Show
          Petr Škoda 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 Škoda 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 Škoda 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: