Moodle

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.

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.

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.

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

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.

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

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

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

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.

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

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

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

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

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).

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

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()

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