08 December 2010, 12:41

I am wondering if the way I'm intenting to add extension points to Mahara would be welcome in the core.

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

13 December 2010, 1:06

Hi Alexandre,

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

21 January 2011, 16:53

I worked on improving the patch, according to your ideas. It looks like this:

-------------------------------- 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?

26 January 2011, 4:08

I've uploaded a suggestion for this and created a merge request with master at http://gitorious.org/mahara/mahara/merge_requests/14 if you want to have a try,

Andrew

28 January 2011, 5:49

With Andrew's feature it is potentially possible to add inline javascript using:

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

13 February 2011, 22:36

Andrew, Ruslan,

It looks to me like merge request 14 only allows plugins to mangle the form when they have the same plugintype as the one in the form definition, but Alexandre needs a random plugin to be able to mangle a form which has a definition like plugintype=core, pluginname=admin, name=edituser_site.

We might as well let all the installed, active plugins have a go at mangling the form.  The plugintype, pluginname properties in the form definitions are pretty much just the names of directories in which to find help files, but they're probably useful to disambiguate forms with the same name, so there's no harm in adding them.  What if we just hack something similar to the contents of plugins_extension_points/pieform_element_callback into the pieform constructor, kind of like this (completely untested):

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

16 February 2011, 9:15

Hello Richard,

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.

.

16 February 2011, 16:44

Hi Ruslan,

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.

16 February 2011, 17:07

... or maybe, we could keep $JS and also make a more general, non-pieform-specific global (like $JS but for all head elements), and then make pieform_element_*_get_headdata() add stuff to that instead of  $GLOBALS['_PIEFORM_REGISTRY']

17 February 2011, 9:34

Hello Richard,

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?