Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0
    • Component/s: Web Services
    • Labels:
      None
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE

      Description

      Create a documentation generator for every protocol.
      The code should use a similar algo to get_virtual_method_code() from webservice/lib.php

        Gliffy Diagrams

          Activity

          Hide
          skodak Petr Skoda added a comment -

          I personally do not like the access control in the new code at all, it should imo use standard forms+session with new capability which allows user to list any function description. There are multiple technical and usability issues...

          The current code is imo abusing the new html_forms class which is intended only for simple buttons and single select boxes.

          Show
          skodak Petr Skoda added a comment - I personally do not like the access control in the new code at all, it should imo use standard forms+session with new capability which allows user to list any function description. There are multiple technical and usability issues... The current code is imo abusing the new html_forms class which is intended only for simple buttons and single select boxes.
          Hide
          skodak Petr Skoda added a comment -

          Looking at the page structure it feels different from other admin pages, my +1 to integrate it fully into admin tree and refactor it to match the coding style of other admin pages. There should be imho some coding style proposal if we decide to do pages like this in a new way.

          Show
          skodak Petr Skoda added a comment - Looking at the page structure it feels different from other admin pages, my +1 to integrate it fully into admin tree and refactor it to match the coding style of other admin pages. There should be imho some coding style proposal if we decide to do pages like this in a new way.
          Hide
          skodak Petr Skoda added a comment -

          issues:
          1/ the docs page can not be disabled - custom login mechanism breaks $CFG->forcelogin (security)
          2/ can not be integrated with the admin settings tree because it does not use standard sessions
          3/ IP restrictions cause problems - browser does not have the same IP as the ext. system
          4/ protocol checks missing (security)
          5/ token auth missing - it might be very problematic when token is linked to current session
          6/ missing login fault logging and lockout (not implemented in standard WS yet too) (security)
          7/ this approach does not give access to docs if WS are not configured
          etc.

          The standard external page approach problem is that it would not list all available functions for particular user, but instead would only list all external functions.

          Show
          skodak Petr Skoda added a comment - issues: 1/ the docs page can not be disabled - custom login mechanism breaks $CFG->forcelogin (security) 2/ can not be integrated with the admin settings tree because it does not use standard sessions 3/ IP restrictions cause problems - browser does not have the same IP as the ext. system 4/ protocol checks missing (security) 5/ token auth missing - it might be very problematic when token is linked to current session 6/ missing login fault logging and lockout (not implemented in standard WS yet too) (security) 7/ this approach does not give access to docs if WS are not configured etc. The standard external page approach problem is that it would not list all available functions for particular user, but instead would only list all external functions.
          Hide
          skodak Petr Skoda added a comment -

          we could also add a flag into db/external.php which hides functions in this UI, could be useful for hiding of local hacks, the standard plugins do not need this because we are opensource

          Show
          skodak Petr Skoda added a comment - we could also add a flag into db/external.php which hides functions in this UI, could be useful for hiding of local hacks, the standard plugins do not need this because we are opensource
          Hide
          jerome Jérôme Mouneyrac added a comment -

          Thanks Petr for your review

          0/ "The standard external page approach problem is that it would not list all available functions for particular user, but instead would only list all external functions."
          >> I think it's a issue as we don't want to let some developers know what external functions are activated. So they don't complain that other entities have access to these functions.

          "1/ the docs page can not be disabled - custom login mechanism breaks $CFG->forcelogin (security)"
          >> it seems to me less important than 0/. Can you explain why it breaks and why it implies? The login page doesn't log into Moodle, just authenticate web service user (as the web services do)

          "2/ can not be integrated with the admin settings tree because it does not use standard sessions"
          >> I still think it is not a good idea to display any admin tree neither any links, or Moodle menus to an external developers.

          "3/ IP restrictions cause problems - browser does not have the same IP as the ext. system"
          >> right, I put it for protection thinking we could remove it easily. I'll remove it.

          "4/ protocol checks missing (security)"
          >>good catch too, I missed out this point

          "5/ token auth missing - it might be very problematic when token is linked to current session"
          >> I knew about that. When I first started writing the generator it was just extending the webservice_server abstract class (containing only authentication), and so had the exact same authentication. Any ws user has a password, so should be able to authenticate with ws.

          "6/ missing login fault logging and lockout (not implemented in standard WS yet too) (security)"
          >> another good point, 'll write another issue for that.

          "7/ this approach does not give access to docs if WS are not configured"
          >> I think it's a good approach. We can write an additional documentation page listing all web service (with PDF to download)

          in an additional 8/, we need an easy printable form.

          Show
          jerome Jérôme Mouneyrac added a comment - Thanks Petr for your review 0/ "The standard external page approach problem is that it would not list all available functions for particular user, but instead would only list all external functions." >> I think it's a issue as we don't want to let some developers know what external functions are activated. So they don't complain that other entities have access to these functions. "1/ the docs page can not be disabled - custom login mechanism breaks $CFG->forcelogin (security)" >> it seems to me less important than 0/. Can you explain why it breaks and why it implies? The login page doesn't log into Moodle, just authenticate web service user (as the web services do) "2/ can not be integrated with the admin settings tree because it does not use standard sessions" >> I still think it is not a good idea to display any admin tree neither any links, or Moodle menus to an external developers. "3/ IP restrictions cause problems - browser does not have the same IP as the ext. system" >> right, I put it for protection thinking we could remove it easily. I'll remove it. "4/ protocol checks missing (security)" >>good catch too, I missed out this point "5/ token auth missing - it might be very problematic when token is linked to current session" >> I knew about that. When I first started writing the generator it was just extending the webservice_server abstract class (containing only authentication), and so had the exact same authentication. Any ws user has a password, so should be able to authenticate with ws. "6/ missing login fault logging and lockout (not implemented in standard WS yet too) (security)" >> another good point, 'll write another issue for that. "7/ this approach does not give access to docs if WS are not configured" >> I think it's a good approach. We can write an additional documentation page listing all web service (with PDF to download) in an additional 8/, we need an easy printable form.
          Hide
          dougiamas Martin Dougiamas added a comment -

          +1 for jerome's comments. The page should show to external developers what functions they can access only. Comprehensive docs belong in Moodle docs.

          Show
          dougiamas Martin Dougiamas added a comment - +1 for jerome's comments. The page should show to external developers what functions they can access only. Comprehensive docs belong in Moodle docs.
          Hide
          skodak Petr Skoda added a comment -

          a/ at least split the display logic from the entry point so that others may print external function docs elsewhere
          b/ do not use define('NO_DEBUG_DISPLAY', true); in scripts that return human readable html - this is intended for binary file servings and machine readable XML
          c/ there should be some kill switch for this imo, defaulting to script disabled

          Show
          skodak Petr Skoda added a comment - a/ at least split the display logic from the entry point so that others may print external function docs elsewhere b/ do not use define('NO_DEBUG_DISPLAY', true); in scripts that return human readable html - this is intended for binary file servings and machine readable XML c/ there should be some kill switch for this imo, defaulting to script disabled
          Hide
          skodak Petr Skoda added a comment -

          d/ assigning directly to $USER is often very tricky and is generally discouraged, it might be better to work around it here because it is not necessary due to the fact that the function is not executed

          Show
          skodak Petr Skoda added a comment - d/ assigning directly to $USER is often very tricky and is generally discouraged, it might be better to work around it here because it is not necessary due to the fact that the function is not executed
          Hide
          jerome Jérôme Mouneyrac added a comment -

          agree from a to d
          I removed all the $USER reference and debug define. Working on splitting the code and a switch.
          Thanks for reviewing Petr.

          Show
          jerome Jérôme Mouneyrac added a comment - agree from a to d I removed all the $USER reference and debug define. Working on splitting the code and a switch. Thanks for reviewing Petr.
          Hide
          skodak Petr Skoda added a comment -

          ehm, the use of renderer is not correct because wsdoc is neither plugin nor core, it is pure luck that it actually finds the renderer
          $renderer = $PAGE->theme->get_renderer('core_wsdoc',$OUTPUT);

          see MDL-21143

          Show
          skodak Petr Skoda added a comment - ehm, the use of renderer is not correct because wsdoc is neither plugin nor core, it is pure luck that it actually finds the renderer $renderer = $PAGE->theme->get_renderer('core_wsdoc',$OUTPUT); see MDL-21143
          Hide
          jerome Jérôme Mouneyrac added a comment -

          I am trying to use at best the renderer system, it is fun I do understand that there is no best practice documentation because the renderer system is still in construction. And I'll be happy to improve the generator code once I know what is the correct way to do

          Sam explained me what is the current problem with the way I used the renderer into the documentation:
          The problem with the current documentation renderer is not that I am purely lucky (the renderer is manually included) but the problem is that we can not override the renderer (because the renderer is manually included).

          A solution (knowing that I don't know much about renderer yet ) could be that get_renderer() searches for some renderer class name containing the word override (the rest of the name being the same name as the class we tried to override).

          I stay tunes on the evolution of MDL-21143

          Show
          jerome Jérôme Mouneyrac added a comment - I am trying to use at best the renderer system, it is fun I do understand that there is no best practice documentation because the renderer system is still in construction. And I'll be happy to improve the generator code once I know what is the correct way to do Sam explained me what is the current problem with the way I used the renderer into the documentation: The problem with the current documentation renderer is not that I am purely lucky (the renderer is manually included) but the problem is that we can not override the renderer (because the renderer is manually included). A solution (knowing that I don't know much about renderer yet ) could be that get_renderer() searches for some renderer class name containing the word override (the rest of the name being the same name as the class we tried to override). I stay tunes on the evolution of MDL-21143
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Hi Jerome,
          I've just been pulling through the output code (yet again) and have misinformed you about how theme's override renderers.

          The theme achieves it by first defining its own render factory (within a lib.php file) which gets called by the theme class and set by $THEME->rendererfactory in the themes config.
          The render factory can then override the method in which the renderers are found, instantiated and returned. This method could implement any process it desires to override renderers, and allows for exactly as you suggested above. Should you choose to implement a scheme whereby overridden renderers prefix the word `override_` then you could do so here.

          If you would like to see an example of how this is implemented check out the custom corners theme.

          Cheers
          Sam

          Show
          samhemelryk Sam Hemelryk added a comment - Hi Jerome, I've just been pulling through the output code (yet again) and have misinformed you about how theme's override renderers. The theme achieves it by first defining its own render factory (within a lib.php file) which gets called by the theme class and set by $THEME->rendererfactory in the themes config. The render factory can then override the method in which the renderers are found, instantiated and returned. This method could implement any process it desires to override renderers, and allows for exactly as you suggested above. Should you choose to implement a scheme whereby overridden renderers prefix the word `override_` then you could do so here. If you would like to see an example of how this is implemented check out the custom corners theme. Cheers Sam
          Hide
          jerome Jérôme Mouneyrac added a comment -

          Thanks, Sam no problem
          I keep listening when a better solution is proposed to $renderer = $PAGE->theme->get_renderer('core_wsdoc',$OUTPUT);

          Show
          jerome Jérôme Mouneyrac added a comment - Thanks, Sam no problem I keep listening when a better solution is proposed to $renderer = $PAGE->theme->get_renderer('core_wsdoc',$OUTPUT);
          Hide
          jerome Jérôme Mouneyrac added a comment -

          Added a Print button that trigger the print browser operation.
          The only thing I didn't do in this issue is that I didn't split the code for reuse. I'll do it later if ever we need it (but I don't think we really need it).

          Show
          jerome Jérôme Mouneyrac added a comment - Added a Print button that trigger the print browser operation. The only thing I didn't do in this issue is that I didn't split the code for reuse. I'll do it later if ever we need it (but I don't think we really need it).
          Hide
          jerome Jérôme Mouneyrac added a comment -

          Following the last javascript and theme changes in Moodle, the documentation had been broken. Btw add a token authentication method too. Thanks.

          Show
          jerome Jérôme Mouneyrac added a comment - Following the last javascript and theme changes in Moodle, the documentation had been broken. Btw add a token authentication method too. Thanks.
          Hide
          jerome Jérôme Mouneyrac added a comment -

          css is now broken:
          Coding error detected, it must be fixed by a programmer: Invalid stylesheet parameter.

          More information about this error
          Debug info: webservice/wsdoc.css
          Stack trace:

          • line 418 of /lib/ajax/ajaxlib.php: coding_exception thrown
          • line 275 of /webservice/wsdoc.php: call to page_requirements_manager->css()
          • line 78 of /webservice/wsdoc.php: call to webservice_documentation_generator->display_login_page_html()
          • line 300 of /webservice/wsdoc.php: call to webservice_documentation_generator->run()
          Show
          jerome Jérôme Mouneyrac added a comment - css is now broken: Coding error detected, it must be fixed by a programmer: Invalid stylesheet parameter. More information about this error Debug info: webservice/wsdoc.css Stack trace: line 418 of /lib/ajax/ajaxlib.php: coding_exception thrown line 275 of /webservice/wsdoc.php: call to page_requirements_manager->css() line 78 of /webservice/wsdoc.php: call to webservice_documentation_generator->display_login_page_html() line 300 of /webservice/wsdoc.php: call to webservice_documentation_generator->run()
          Hide
          skodak Petr Skoda added a comment -

          there should not be any css files laying around any more, everything should go into theme css or plugins, the requires->css() is intended solely for user submitted stylesheets such as the mod/data module, please move all css to the standardold theme for now

          Show
          skodak Petr Skoda added a comment - there should not be any css files laying around any more, everything should go into theme css or plugins, the requires->css() is intended solely for user submitted stylesheets such as the mod/data module, please move all css to the standardold theme for now

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                24/Nov/10