Template options

[alexandru-elisei]

The plugin does not check anywhere if the recipe contains the options that a template needs and the plugin will generate files that are incorrect. You are expecting that the user is intimate with the template and will supply the correct options.

Missing recipe options can be spotted pretty easily because there will be an empty space where a value was supposed to be, but there are cases where the error is more subtle.

For example, if for one of the capabilities the user doesn't supply the 'name' attribute, mustache will look for it up the hierarchy and it will match the top level attribute 'name', which represents the value for the string id 'pluginname'. You will end up with something like this: 'mod/modplugin:Example plugin generated' => [

[alexandru-elisei]

A possible solution would be to have each file class have a list of options that will be checked when calling the set_data() function, similar to how you are doing it in the version_php_file class.

[mudrd8mz]

I'll look at details after the weekend. I can imagine that tool_pluginskel\local\skel\base::set_data() calls first self::validate_data() which every subclass could extend.

[mudrd8mz]

Details later, I believe a good way could be to declare expected data fields in at the beginning of the template. Something similar to what you had in $optional_options, but declared close to the place where it is used. Something like:

{{!
    db/access.php

    * component?
    * copyright?
    * capabilities? capabilities.name
}}

Also, can you please research the Mustache docs a bit, I roughly rememeber there was something like {{ .name }} that should not bubble up. thanks.

[alexandru-elisei]

Yes, we can use {{ .name }} to prevent looking up the context stack, but we would also need to use the pragma PRAGMA_ANCHORED_DOT when instantiating the Mustache engine.

Do you think all our template variables that have local scope (like array values) should be anchored? To make sure that Mustache only searches for the variables in the current context, and doesn't try to find them higher in the context scope.

[alexandru-elisei]

I think that the skeleton class is a good place to check for the template variables, we also need the names of those variables for the web interface.

[mudrd8mz]

I do like the anchored dot behaviour. For most places I've seen so far, I think it should be converted to {{ .name }} as we do not really expect the propagation.

I was hoping the skel classes would be quite general, such as that php_internal_file. I was thinking we would use a subclass just for special case like lang_file. But if you see it would work well for the recipe validation and the UI purposes, then ok.

[alexandru-elisei]

Anchored dot I was asking if ALL local variables should be anchored, to prevent edge cases where a variable isn't present in the local scope, but a variable with the same name is present in a higher context.

For example, in the db_events.mustache template, all the variables in the 'observers' context would use a dot: .eventname, .callback, .includefile, .priority, .internal.

I think this is the right way to do it because even though right now there isn't any situation where we have a variable like 'includefile' in a higher context, as we develop the plugin we might add such a variable and it's very easy to forget to modify the template.

Skel classes validating template variables I think this is the logical place to do the validation. The role of a skel class is to generate the template, it makes sense to also check here if the template variables are present.

As for how the skel classes will work for validating the UI: each skel class will have a function like get_template_options() which will return all the recipe options needed for the template. The manager will have a function like get_file_options($filename), which will return $this->files[$filename]->get_template_options(). This way the UI will have a list of variables and will ask the user for their values.

I think it's still a bit early to implement this. I propose we postpone this until we are actually working on the web interface (or some other suitable moment).

Implemented as I proposed above there would be insignificant changes to the current code (modify the skel type in prepare_files_skeletons()), but if we decide to use another method for validating the template options/getting those options for the web interface there would be a lot of code that I would have to throw away (all the skel classes).

[mudrd8mz]

Re anchored dots: Yes, I agree with you, sorry if it was not clear. Let us enable the pragma and let us start using the dot notation for all the variables.

Re validating: Firstly, I was confused by the terminology. By "template options" you actually mean template variables / placeholders, right? A method like get_template_variables() would make sense then. But i agree it may be a bit pre-mature to start with it now. I would suggest to deal with activity modules lib.php and other essential, not trivial bits first.

[alexandru-elisei]

Yeah, when I say template options I mean template variables.

I'll use template variables from now on to avoid the confustion.

[mudrd8mz]

Closing for inactivity.