mudrd8mz / moodle-tool_pluginskel

Generator of Moodle plugins skeletons
https://moodle.org/plugins/tool_pluginskel
Other
51 stars 46 forks source link

String for the capability title not created #66

Closed mudrd8mz closed 8 years ago

mudrd8mz commented 8 years ago

Via the UI, I define a capability like "block/helloworld:view". There should be a mandatory field like "Title" with the human readable name/title of the capability that would end as a string with the capability name.

alexandru-elisei commented 8 years ago

What exactly do you mean? The capabilities are defined in the UI exactly as they are defined in the recipe.

The name field is used to name the capability. So, for name: something and the component component: mod_new the generated capability would be mod/new:something. Are you suggesting that I should rename the name field to title?

As for making it mandatory.

Capabilities are not mandatory. If I make it mandatory in the web interface (like component) then I won't be able to submit the page without writing something there, making capabilities in fact mandatory.

I can throw an exception if the capabilities aren't correctly defined, yes, but how and where? The manager doesn't check if the recipe is correct before generating the plugin files, why should the web interface do it?

mudrd8mz commented 8 years ago

Sorry, I was not clear apparently. What I am saying is that for each capability, a string with its title is to be created - e.g. https://github.com/moodle/moodle/blob/1d17f51a8df2f7570c9b2fec95593b7c299139e5/mod/workshop/lang/en/workshop.php#L331-L349

If the string is missing, the permissions UI throws debugging warning. I think we should provide a new field (which I suggested to be named as "title") that could hold such string value. And the string would be created for each capability automatically.

alexandru-elisei commented 8 years ago
  1. An example of capability definition for the plugin block_html:
$capabilities = array(
    'block/html:addinstance = array(
    ...
    )
);

What information should the new title variable have?

mudrd8mz commented 8 years ago

What information should the new title variable have?

The human readable name of the capability, the one that goes into the string.

Why can't I simply check for the current name variable?

I understand the current name is the machine readable name of the capability - which is then used in the <componenttype>/<componentname>:<capabilityname>. This whole issue is about strings that are supposed to exist for each capability.

but not inside the manager

Sure it will be the manager feature. I was thinking the field would be mandatory if the capability is defined (if name is provided, then title must be too). But I now realised there is probably no support for it in the moodle forms library.

alexandru-elisei commented 8 years ago

Oh, I see now.

I could automatically generate a text for the lang string that would go something like this Please define in lang/en/block_html.php the human readable title for capability html:addinstance.

Another option would be to create a static function that, for a given recipe, verifies if the string text exists and if not will issue a warning. This function could be called during plugin generation and by the web interface to verify the recipe after submitting the form.

The problem I see with the second approach is how to implement the warning. In the monolog library I haven't found any stream handlers that can interact with the browser (except BrowserConsoleHandler), but I don't think that's what we need.

I can think of two solutions for this:

alexandru-elisei commented 8 years ago

In the meantime I've added to the manager a check for the missing strings, a logger warning is issued with the missing string - see #68.

alexandru-elisei commented 8 years ago

How about renaming title to description?

capabilities:
    - name: enrol 
    - description: Enrol users
...
mudrd8mz commented 8 years ago

Description in Moodle typically holds a longer text. These strings are supposed to be short one liners.

mudrd8mz commented 8 years ago

I assume this can be closed as implemented now?

alexandru-elisei commented 8 years ago

Yes, close it.