Hi, sorry Paul, I actually forgot about this issue. Thanks for reminding.
And sorry again but I completely disagree with your approach. Though your code looks very neat and easy to read.
Maybe your code works for rubrics and marking guide and mod_assign but will fail completely for any other module supporting advanced grading or any other contributed advanced grading plugin.
Your check for capability 'assign:view' makes code not working for mod_assignment (and all non-core modules supporting advanced grading). Also you query table 'gradingform_'.$def->method.'_criteria' and it is completely up to grading method which table to use for storing data.
There MUST be function(s) inside grading methods plugins to return such information for external queries. Externallib should ONLY be calling API functions and make no direct queries to DB.
There are already functions get_definition_copy() and get_definition_for_editing() that return the definition. If for externallib we need definition with the original ids and not processed file links, feel free to add a new function to the parent class and overwrite it in rubrics and marking guide. This way contributed plugins that need to support externallib will implement this method as well.
The same with the capability to view the module (or definition) - it must be generic. Also please don't forget that 'mod:xxx/view' is not even a standard capability. Module may not define it. Besides this is not the only capability for user to view the advanced grading definition.
In case of both rubrics and marking guide user must have either capability 'moodle/grade:managegradingforms' or capability to view module but only in case an option 'alwaysshowdefinition' is on and definition is ready. Students can never see not active definition.
Well, the resume:
core_grade_external::get_definition() should not define structure for criteria and levels because this is rubric/guide - specific structure. This function should only define fields that are the same for ANY grading method (including non-core ones). Afaik externallib does not support free format in returned xml. In this case additional method-specific data (such as criteria/levels) should be returned in some serialised string.
core_grade_external::get_definitions() should call functions from grading methods and not do DB queries. Also capabilities check should be more generic and most likely be a callback to grading method again.
Actually I'm pretty sure we discussed it already.
I've spent some time recently looking for bugs in course categories creation in externallib. The result was one-line fix (see
MDL-38144) but it took a while to find out why. And this could be avoided if externallib did not copy the code but rather use the same functions as core (and created those functions if they did not exist).
Well sorry again to fail your work and that it took me a while to start reviewing.