> the only problem is that we can't really break the smarty function because too many non-core plugins will be using it.
I did not mean breaking it, rather changing the way how header related stuff is defined, which will make it cleaner and preserve all existing functionality, i.e.
$stringjs = '<script type="text/javascript">';
$stringjs .= 'var strings = ' . json_encode($strings) . ';';
$stringjs .= '</script>';
will become
$HEADER->add_js_string('var strings = ' . json_encode($strings) . ';')
another example
$stylesheets = array_merge($stylesheets, array_reverse($pluginsheets));
will become
$HEADER->add_css_link(array_reverse($pluginsheets));
Your dwoo function idea will basically suggest the same but another way, through creating a proper dwoo function with all new stuff and turning smarty as a wrapper for it, which sounds good. Shall we aim this to 1.4 then?
]]>But maybe this is a good time to write a dwoo() function, that takes a sensible, less ugly set of arguments, and leave smarty() there, but change it into a wrapper around dwoo() that sets the appropriate globals.
It'd also be a good time to fix one other thing that's always bugged me, adding some kind of warning whenever a programmer calls pieform() after the dwoo/smarty function was already called. Sometimes that means the $GLOBALS['_PIEFORM_REGISTRY'] isn't set, so the header js doesn't get included, and the form doesn't work.
]]>I have to admit, I have not come accross pieform_element_<elementname>_get_headdata() before, it was definitely possible using it to change the header in RPX plugin, the only problem with it is that loginform should be aware of rpx plugin (in configdir) which is a bit durty. Perhaps we can make 'constructorcallback' as an array with a list of plugin functions that needs to be called, one of these functions will be mangling $GLOBALS['_PIEFORM_REGISTRY'] and changing 'configdir' by adding required location. But this is also complicated and difficult to maintain.
Your idea about global variable sounds interesting and could make the code inside pieform_element_*_get_headdata() more tidier. What if we get rid of $JS completely and use $HEADER global instead that would contain all possible types of head elements (js, css), then we could cleanup smarty function to get rid of $javascript_array, $stringjs, $stylesheets, $headers and replacing them with corresponding methods?
]]>Yeah, you're absolutely right, that proposal would put way too many safe_requires in each page, and still doesn't solve the problem of letting plugins add things into the validate/submit callbacks, which we haven't talked about much. So maybe we should continue this discussion and work towards a nicer general solution, maybe with the extra table you suggested containing form names and plugins.
In the meantime we'll add something into core for the login form that'll fix your immediate problem with the janrain engage auth.
What if we add Andrew's stuff, initially just the get_plugin_elements hook that looks at the form's plugintype plugins (later on we could make it look at the extra table instead, and add hooks for validate/submit_plugin_elements too).
Like you, I've got misgivings about the $JS global variable - it initially looks like it'd be a nice general solution, but I can't think of anywhere other than the login form where it'd be needed; login is probably the only form that gets added by js like that. I think it might be nicer to use a new custom pieform element instead, inside auth/rpx, which adds js to the header using the pieform_element_<elementname>_get_headdata() function - would that work? I guess you'd also need to use the 'configdirs' property of the login form to include elements from auth plugins.
]]>The only probem with yours/Alexandre's solution is that it adds an extreme overhead. Imagine, for each pieform constructor call (we have them nearly on every Mahara page) it will be needed to "safe_require" all possible plugins (one of the heaviest things in terms of performance) and check whether particular method exists in each of them.
The advantage of Andrew's idea was to limit such calls to the forms that have 'constructorcallback' parameter and check the plugins of 'plugintype' parameter only making very little impact on perfomance.
Of course this solution is not as flexible as yours and Alexandre's one, but we might expect which forms are likely to be modified and add callback parameters to them in the core. Alternatively, we should think about something smarter to avoid numerous "safe_require" - perhaps some table with plugin list and which form they intend to modify, so that pieform constructor will only call the required method for the particular plugin.
.
]]>$elementcallback = 'pieform_';
if (!empty($this->data['plugintype'])) {
$elementcallback .= $this->data['plugintype'] . '_' . $this->data['pluginname'] . '_';
}
$elementcallback .= $this->name . '_elements';
foreach (plugin_types_installed() as $plugintype) {
foreach (plugins_installed($plugintype) as $plugin) {
safe_require($plugintype, $plugin['name']);
$classname = generate_class_name($plugintype, $plugin['name']);
if (method_exists($classname, $elementcallback)) {
call_static_method($classname, $elementcallback, $this->data);
}
}
}
That would mean that plugin authors could mangle forms to their hearts' content without the need to modify the form definition to add a constructorcallback. I think it'd also do what you need for your auth plugin? You'd just have to define something like PluginAuthRpx::pieform_auth_internal_login_elements(&$data)
We still might have forms with the same name, plugintype, and pluginname floating around, but I guess we can rename them in core as we find them.
$elements['inlinejs'] = array(
'value' => '<script type="text/javascript">alert('Hello');</script>';
);
But it will not work for login form for instance, because the pieform html is added through javascript in that case (see get_login_form_js).
So, to solve this I suggest a globally accessible Javascript class that allows manipulating javascript content in html header. See http://gitorious.org/mahara/mahara/merge_requests/15 for details. It is likely needs some discussion and improvement, perhaps there is a smarter way of substituting JS from plugins.
]]>Andrew
]]>
-------------------------------- lib/mahara.php --------------------------------
diff --git a/lib/mahara.php b/lib/mahara.phpindex 8f30229..56a91ba 100644--- a/lib/mahara.php+++ b/lib/mahara.php@@ -1173,6 +1173,15 @@ function fetch_installed_plugins() {
return $plugins;
}
+function hook_invoke($hook, &$elements) {
+
+ foreach(plugins_extension_points($hook) as $classname) {
+ $elements = array_merge(
+ $elements,
+ call_static_method ($classname, $hook, $elements)
+ );
+ }
+}
The hook_invoke function can be called in pieform constructor:
-------------------------------- lib/pieforms/pieform.php --------------------------------
diff --git a/lib/pieforms/pieform.php b/lib/pieforms/pieform.phpindex c5f8652..919c041 100644--- a/lib/pieforms/pieform.php+++ b/lib/pieforms/pieform.php
@@ -172,6 +172,9 @@ class Pieform {/*{{{*/
public function __construct($data) {/*{{{*/
$GLOBALS['_PIEFORM_REGISTRY'][] = $this;
+ // Invoke hook here
+ hook_invoke($data['plugintype'].'_'.$data['pluginname'].'_'.$data['name'].'_elements', $data['elements']);
+
if (!isset($data['name']) || !preg_match('/^[a-z_][a-z0-9_]*$/', $data['name'])) {
throw new PieformException('Forms must have a name, and that name must be valid (validity test: could you give a PHP function the name?)');
}
That works just fine for editing forms, even though there still is a problem for ordering form's elements. I just need to implement the right hook in my plugin to alter a form. But how could we implement hook support for submit and validation functions? What do you guys think?
]]>1- the "if (table_exists(...))" call is unnecessary because that's already checked in plugin_types_installed().
2- the "safe_require(...)" call may need to be cached via a static variable trick to avoid stat'ing each file more than once on every page load.
Security-wise, I didn't see anything wrong. Of course, any extension taking advantage of these hooks needs to be fully trusted, but that's obvious, being PHP code.
Cheers,
Francois
]]>We like your idea and it could definitely make it to core.
To reduce the number of places that need to have some code added, why not add the code directly into Pieform? That way, the Pieform constructor will take care of running the appropriate hooks.
Furthermore, in order to avoid having to tweak every form call to add the name of the function that will modify the elements array, you could just generate the name from the existing form attributes. Something like:
$name = "{$form['plugintype']}_{$form['pluginname']}_{$form['name']}_elements";
That way, if you want to hook any of the forms in Mahara, you just need to add a method with the right name to your plugin.
Does that make sense?
Cheers,
Francois
]]>Here is an example of what we've added so far:
admin/users/edit.php and admin/users/create.php:
----------------------------- admin/users/edit.php ----------------------------- index 81ef021..f4ff94b 100644 @@ -145,24 +145,30 @@ if (count($authinstances) > 1) { 'defaultvalue' => $un ? $un : $user->username, ); } } } $elements['submit'] = array( 'type' => 'submit', 'value' => get_string('savechanges','admin'), ); +$extpoint = "admin_users_modify_form_elements"; +foreach (plugins_extension_points($extpoint) as $classname) { + $elements = call_static_method($classname, $extpoint, + $elements, '', $user); +} + $siteform = pieform(array( 'name' => 'edituser_site', 'renderer' => 'table', 'plugintype' => 'core', 'pluginname' => 'admin', 'elements' => $elements, )); function edituser_site_validate(Pieform $form, $values) { global $SESSION; if (!$user = get_record('usr', 'id', $values['id'])) { return false;
as well as other places where it's requited.. where:
-------------------------------- lib/mahara.php -------------------------------- index b983b6b..f3a1c1a 100644 @@ -1142,24 +1142,79 @@ function plugin_types_installed() { */ function plugins_installed($plugintype, $all=false) { $sort = 'name'; if ($plugintype == 'blocktype') { $sort = 'artefactplugin,name'; } if ($all) { return get_records_array($plugintype . '_installed', '', '', $sort); } return get_records_array($plugintype . '_installed', 'active', 1, $sort); } + + +/** + * This returns the formatted plugin name that has a given method. + * Use this syntax to call installed plugins: + * + * $extpoint = "admin_users_modify_form_elements"; + * foreach (plugins_extension_points($extpoint) as $classname) { + * $elements = call_static_method($classname, $extpoint, + * $elements, '', $user); + * } + * + *@return array of class names that point to plugins that have the specified method + */ +function plugins_extension_points($method) { + $plugins = array(); + foreach (plugin_types_installed() as $plugintype) { + if (table_exists(new XMLDBTable($plugintype . '_installed'))) { + if ($installed = plugins_installed($plugintype, true)) { + foreach ($installed as $i) { + $plugin = $i->name; + safe_require($plugintype, $plugin); + $classname = generate_class_name($plugintype, $plugin); + if (method_exists($classname, $method)) { + $plugins[] = $classname; + } + } + } + } + } + return $plugins; +} + + /** * Helper to call a static method when you do not know the name of the class * you want to call the method on. PHP5 does not support $class::method(). */ function call_static_method($class, $method) { $args = func_get_args(); array_shift($args); array_shift($args); return call_user_func_array(array($class, $method), $args); } function generate_class_name() {
Would this way of doing be accepted in the core, do you have feedback or suggestions as to how we could have solutions to tweak the "Groups" form, and/or a generic way in which we could add all sorts of extension points to extend internal functionality ?
Would you have any concerns about performance and/or security ?
Thank you :)
Alexandre Bourget, Savoir-faire Linux inc., Montreal, Canada
]]>