Florian Lagg wrote:

...

I hope I could provide a small patch on monday for testing.

I made it. The Patch is made with SVN Version 1626. I implemented it slightly different. I'll describe it here:

For me it looks good, but we need few "must have" features at start:

1. Auto-calling plugin actions for build-in tasks/actions, it means we

should add (in index.php) code for automatic calling of actions e.g. 'mail-compose-before', 'mail-compose-after', 'mail-sendmail-before', etc. and probably also 'mail-before', 'mail-after', etc. 2. Possibility to use own tasks/actions (I mean /?_task=settings&_action=myaction or /?_task=mytask) 3. Javascript part - for javascript functions/actions (in similar fasion) including point above. 4. Possibility to add UI part from plugin without skins modification. It's "must have", but I think, we can live without that for now.

And not exactly "must have", but my "would be nice" ideas for future:

5. Actions priority - if we have few plugins for the same action which

should be executed first? 6. Config option(s) for enabling/disabling plugin parts. Let's say we have a plugin with few features, but we need only one. Ok, we can have separate configuration for each plugin, but maybe something global? Let's say using regexp. It may be used for temporary disabling whole plugins without removing files from plugins/ dir. E.g.

$rcmail_config['disabled_plugins'] = 'plugin2 plugin3::mail-compose.* plugin3::settings.*';

Cor Bosman wrote:

I would do it the other way around. All plugins are disabled until you enable them in the config.

Yes, in future we'll need also possibility to enable/disable plugin actions via Settings.

Just to make sure. The plugin system we're talking about here doesnt just allow actions, but also small hooks everywhere throughout the code/templates?

Yes for me. I think templates engine should call plugin actions before/after each template object (objects, but also e.g. buttons and every template tag with specified name) and return content from those actions.

Hi,

• $this->prop['plugin_dir'] = $this->prop['plugin_dir'] ?

unslashify($this->prop['temp_dir']) : INSTALL_PATH . 'plugins';

• $this->prop['plugin_dir'] = $this->prop['plugin_dir'] ?

unslashify($this->prop['plugin_dir']) : INSTALL_PATH . 'plugins';

yes - this is defnitly against the 80-char lines rule if I write it in one line. I've just done copy&paste here - sorry. There are some other issues in my code which I will come after.

1. The plugin class, once instantiated, should register it's

hooks at the plugins repository (rcube_plugins class). No implicit calling of an object method. Otherwise hook names are restricted to naming conventions.

After careful thinking - yes there are some ideas in background why I coded it this way.

1. plugin development is easier if someone doesn't have to care for hook

registration. Now I thought again and there could be an issue without registering: It could be a big performance issue if we try to call any hook on any plugin. So we have to register hooks. 2. if you know the name of the hook you know when it is called, so you can understand 3rd party plugins faster. But with a hook registry you have one single place to look at (constructor of the plugin) so this shouldn't be an issue.

2. Same as in 1) for the rcube_plugins class. The application

should trigger a plugin-hook by calling one single method (like 'trigger_hook()') and pass the name of the hook as the first argument.

Yes. I agree. I first thought of an included API to help plugin developers this way - but I now think this does not make sence. This is just too much overhead if we get many hooks (and I think we will get really many hooks!)

3. Hooks need data and (maybe) return modified data.

Therefore I suggest to pack all data of a plugin-hook into a single argument which is an array (or object). The plugin method then returns the complete array (object) with modified data. Triggering a hook would then look like this:

$credentials = $plugins->trigger_hook('before-login', array('user' => $user, 'pass' => $pwd));

Yes, some hooks may need data. I personally prefer objects rather than arrays. I dont know if there is an object for any kind of data. If not we have to write them too. I think we have three types of hooks: (+ two special types)

1. hook needs no data: the hook after a user login e.g.: The user data could

be gathered from the session vars. But it does make sence to give the user data directly to the hook - if we change the way RC saves userdata we do not have to change the plugins, so this type should likely be non-existent. 2. hook with one "data row", like editing a contact: here the hook should get an object with the data. 2.1. special hook with one "data row" from different sources. For example the hook 'addressbook_contact_edit' which could need data from the user object and from the contact object 3. hook with multiple "data rows", like deleting multiple contacts at once. Shall we give an array of objects to the plugin or shall we call the hook with each data row? (Excuse me about talking in data rows - it's just easier to understand even if it's not 100% the right word for it) 3.1. special hook with multiple "data rows" and objects from different sources. 'addressbook_contact_delete'

How should we handle different types of hooks? I think we shall not use 1. (see reason above) The 2nd one is easy and most used. For the 3rd we have multiple options:

• Call the plugin method for each object
• Call the plugin once with an array of objects
• (my favourite) let the plugin decide how to call the method on hook

registry. For some plugins it could be essential to handle all hooks at once. For the special cases 2.1. and 3.1. we could again use an array() including all objects (or array of objects) Some code to show this cases: Case 1: no code because we shouldn't use it. Case 2: 1 data row, 1 data object $data_object = $plugins->trigger_hook('foo', $data_object); Case 2.1.: multiple data rows, 1 data object $array_of_data_objects = $plugins->trigger_hook('foo', array($data_object_1, $data_object_2)); If the plugin registered with the option to get single data rows the rcube_plugins class could handle to split it and reassemble it before/after calling the function as in 2. Case 3: single data row with data from more objects $array_of_data_objects = $plugins->trigger_hook('foo_user_and_contact', array($user_object, $contact_object)); Case 3.1.: multiple kinds of objects each and multiple data rows $array_of_data_objects = $plugins->trigger_hook('foo_user_and_multiple_contacts', array($user_object, array($contact_object_1, $contact_object_2))); Same as in 2.1. these array could be splitted in multiple calls like in 3.

Uh, gets complicated now. Is there an easier way? Don't think so, but im tired ;-)

After calling a hook we should take the returning object and write it back to the original sources, right?

Ah... btw: I prefer the naming convention 'user-login-after' instead of your 'after-login' for hook names because it groups things together. I havn't seen rules on this in the guidelines, is there any? It just groups together what should be togehter. Any hook on user begin with..: user Any hook on login begins with: user_login The hook after a login uses..: user_login_after Is this OK? (I chaged the dash with underline too meet the guidelines)

4. Please read the http://trac.roundcube.net/wiki/Dev_Guidelines,

especially the naming conventions.

Thanks.

We also have a wiki on http://trac.roundcube.net. Feel free to create pages and upload patches there.

I'm just used to my site and want to be able to make backups, ... Thanks anyway. I'll note the requests here on my site http://www.lagg.at/temp/todo-rc-plugin

Thanks for your work so far! Thomas

Thanks for your work so far! I use it - so I can also give something.

I want to point you again to this site: http://www.lagg.at/temp/todo-rc-plugin We have to decide which hooks we need right now. I have added all hooks which I eventually ever need for my funambol-plugin. If you have no special wishes for hooks right now I will implement these only at first. This will be the better approach than implementing all possible hooks and never use them.

...

Plugin or not... always debatable, I guess.

Take LDAP addressbook support, for example - isn't that something that could also be provided by a plugin? It also requires configuring a backend

system

...

(an LDAP directory).

Absolutely - if there is a flexible framework almost everything except the framework itself can be a plugin - with some plugins required (see Typo3 for a great sample of this!). But unless this Plugin system is done its hard to implement something like this without patching the sources.... so first job: plugin architecture!

Jeroen, could you provide me a patch for your plugin? You may have read that I want to implement a plugin architecture here - it would be great for me to see how you implemented this.

Your "plugin" is configureable on the web-view of RC, so it is a plugin involved in the backend and frontend.

Depending on this I can find out (hopefully) where you need hooks to implement this plugin without patching if you use my plugin architecture. We had some questions about this here on the list but inspecting code is always easier.

It couldn't be a solution to carry 3rd party products as diff patches to specific versions - this way you could never implement 2-3 plugins with ease.

Pls. send your patch to the list. Thanks.

List info: http://lists.roundcube.net/dev/

We could use stdClass to create any kind of objects. But objects are always passed as reference. This is good for the memory but dangerous for the application code. If passed by reference a plugin can (accidentally) change data in the argument object. Arrays are passed by value (but only copied on change). If a plugin changes some argument values it does not affect the original data. The plugin has to return the new data and even then the application can decide whether to use the changed data or not. With

arrays

we can have "read-only" arguments, with objects we don't.

The arrays aren't really read-only because the plugin can write on them. This could be a stumbling block for plugin development because the developer doesn't see the variable is read-only.

We can have read only arguments with classes. class foo { private $bar_ro; private $bar_rw;

public function getBar_ro()
{
    return $bar_ro;
}
public function getBar_rw()
{
    return $bar_rw;
}
public function setBar_rw($value)
{
    //check some things here
    if($value < 0)
        throw new Exception('setBar_rw bust be equal or more than 0')

    return true;
}

} There is no setter so a plugin which gets this class parameter could not set $bar_ro - but read it. The variable $bar_rw could be written also - but there are some checks if the values are correct. We can also control & check given data so we could write it back if the core code without any problems.

We just have to define data classes we can use in our plugins. Maybe we find a way to isolate plugins to these functions (and not to access roundcube objects directly e.g. using the main roundcube singleton (cannot remember the name right now)? Don't know.

...

I think we have three types of hooks: (+ two special types)

Hooks should always get one argument (could be null) which is an array.

In

case of the address book we have the rcube_result_set class which is some sort of an iterator for "data rows" or records.

Ok, than any hook should send them array(foo), where foo is a data class.

...

After calling a hook we should take the returning object and write it

back

...

to the original sources, right?

It depends on the implementation. The core application should have

control

of what data can be changed by a plugin and not the plugin itself.

Agree. With the above code (classes rather than arrays) we can delegate some data checking logic to the plugin-architecture.

...

Ah... btw: I prefer the naming convention 'user-login-after' instead of your 'after-login' for hook names because it groups things together. I havn't seen rules on this in the guidelines, is there any? It just groups together what should be togehter. Any hook on user begin with..: user Any hook on login begins with: user_login The hook after a login uses..: user_login_after Is this OK? (I chaged the dash with underline too meet the guidelines)

That's OK. Since hooks are not directly related to object methods but simple strings we're not bound to the guidelines here. Using dashes could help distinguish between hooks and method names...

Ok, let's use dashes.

I think this is the clearest definition we had so far. What do you think?

Of course they are writable for the plugin but it's just a local copy and not the original data. The plugin has to RETURN the altered data. That's simple data-in and data-out. Altering arguments that are passed by reference makes it hard to understand where data is changed when reading/debugging the code.

I agree that it might not be clear to the plugin if the modified data it returns will really be used by the app but I don't like to build a

complex

data structure before calling a hook. As an application developer I'd

like

to compose hook arguments inline like

$plugin->trigger_hook('a-hook', array('foo' => $bar, ...));

You can even use a class inline (using a constructor in the class), I'm describing it below. I think we should use a class because it's more secure.

because I think this is the common case. Let's move the specification which modified data will finally be used to the documentation of the plugin-hooks.

...

Ok, than any hook should send them array(foo), where foo is a data

class.

I suggest to always use a name => value map. Since PHP does not support named parameters this is the best way to achieve maximum extendability.

agree.

...

With the above code (classes rather than arrays) we can delegate some data checking logic to the plugin-architecture.

Sorry, I don't understand what you mean. Example? Maybe we should continue this conversation in German :-)

TO DEVELOPRES ONLY SPEAKING ENGLISH: I just wanted to discuss some more complex detail with Thomas in german - excuse me for that. I will translate a summary after the discussion ends in english.

OK. In einer eigenen Datenklasse könnten wir Variablen beim editieren überprüfen:

Also zum Beispiel: //--------------------------------------------------------------------- // die Datenklasse "foo": class foo { private $bar_ro; //read-only argument private $bar_rw; //read-write argument

//konstruktur public foo($bar_ro, $$bar_rw) { $this->bar_ro = $bar_ro; $this->bar_rw = $bar_rw; }

//read-only argument $bar_ro hat nur einen getter: public function getBar_ro() { return $bar_ro; }

//read-write: getter und setter: public function getBar_rw() { return $bar_rw; } public function setBar_rw($value) { //im setter einer variable können wir daten vom plugin überprüfen, //hier zB muß der Wert bar_rw positiv bleiben: if($value < 0) throw new Exception('setBar_rw bust be equal or more than 0');

//alles klar, der Wert ist OK:
return true;

} }

//Aufruf des hooks im code von RC:

//Beispiel RC Daten: $bar_ro = 3; $bar_rw = 4;

//hook aufrufen $returnData = rcube_plugins::get_instance()->trigger_hook( 'a-hook', new foo(bar_ro => $bar_ro, bar_rw => $bar_rw));

//das plugin kann nur die werte verändern wie wir das in der datenklasse foo erlauben

//dann returnData zurückschreiben $bar_ro = $returnData->getBar_ro(); $bar_rw = $returnData->getBar_rw();

Ist eigentlich nicht komplizierter, aber viel sicherer. Außerdem kann man in der plugin-architektur diese klasse nacheinander an mehrere plugins geben ohne dass diese sich direkt ausknocken können. Mit arrays könnte zB das erste Plugin die ID des angemeldeten Benutzers ändern und das 2. Plugin hat ein problem. Mit Datenklassen können wir das von vorne herein verbieten.

Ich würds gern so machen. Was dagegen?

Wenn nicht, wo sollen wir die ganzen Datenklassen hinlegen (im Filesystem)? Ich schlage mal vor program/include/rcube_plugins_dataclasses/* oder wenn diese außerhalb von Plugins auch verwendet werden program/include/rcube_dataclasses/*

Danke inzwischen,

Florian Lagg wrote:

...

Of course they are writable for the plugin but it's just a local copy and not the original data. The plugin has to RETURN the altered data. That's simple data-in and data-out. Altering arguments that are passed by reference makes it hard to understand where data is changed when reading/debugging the code.

I agree that it might not be clear to the plugin if the modified data it returns will really be used by the app but I don't like to build a

complex

...

data structure before calling a hook. As an application developer I'd

like

...

to compose hook arguments inline like

$plugin->trigger_hook('a-hook', array('foo' => $bar, ...));

You can even use a class inline (using a constructor in the class), I'm describing it below. I think we should use a class because it's more secure.

...

because I think this is the common case. Let's move the specification which modified data will finally be used to the documentation of the plugin-hooks.

...

...

Ok, than any hook should send them array(foo), where foo is a data

class.

I suggest to always use a name => value map. Since PHP does not support named parameters this is the best way to achieve maximum extendability.

agree.

...

...

With the above code (classes rather than arrays) we can delegate some data checking logic to the plugin-architecture.

Sorry, I don't understand what you mean. Example? Maybe we should continue this conversation in German :-)

TO DEVELOPRES ONLY SPEAKING ENGLISH: I just wanted to discuss some more complex detail with Thomas in german - excuse me for that. I will translate a summary after the discussion ends in english.

OK. In einer eigenen Datenklasse könnten wir Variablen beim editieren überprüfen:

Also zum Beispiel: //--------------------------------------------------------------------- // die Datenklasse "foo": class foo { private $bar_ro; //read-only argument private $bar_rw; //read-write argument

//konstruktur public foo($bar_ro, $$bar_rw) { $this->bar_ro = $bar_ro; $this->bar_rw = $bar_rw; }

//read-only argument $bar_ro hat nur einen getter: public function getBar_ro() { return $bar_ro; }

//read-write: getter und setter: public function getBar_rw() { return $bar_rw; } public function setBar_rw($value) { //im setter einer variable können wir daten vom plugin überprüfen, //hier zB muß der Wert bar_rw positiv bleiben: if($value < 0) throw new Exception('setBar_rw bust be equal or more than 0');

//alles klar, der Wert ist OK:
return true;

} }

//Aufruf des hooks im code von RC:

//Beispiel RC Daten: $bar_ro = 3; $bar_rw = 4;

//hook aufrufen $returnData = rcube_plugins::get_instance()->trigger_hook( 'a-hook', new foo(bar_ro => $bar_ro, bar_rw => $bar_rw));

//das plugin kann nur die werte verändern wie wir das in der datenklasse foo erlauben

//dann returnData zurückschreiben $bar_ro = $returnData->getBar_ro(); $bar_rw = $returnData->getBar_rw();

---

Ist eigentlich nicht komplizierter, aber viel sicherer.

But a loooot more code to write, to understand and to maintain. Named parameters (=array) are easier to use and simpler to understand. Security? The rule is to not modify the parameters or to clone the array before calling the hook. Simple, easy, flexible.

Außerdem kann man in der plugin-architektur diese klasse nacheinander an mehrere plugins geben ohne dass diese sich direkt ausknocken können.

QA should assure that.

Mit arrays könnte zB das erste Plugin die ID des angemeldeten Benutzers ändern und das 2. Plugin hat ein problem. Mit Datenklassen können wir das von vorne herein verbieten.

Ich würds gern so machen. Was dagegen?

I vote against this approach as it seems to be too complicated.

Wenn nicht, wo sollen wir die ganzen Datenklassen hinlegen (im Filesystem)? Ich schlage mal vor program/include/rcube_plugins_dataclasses/* oder wenn diese außerhalb von Plugins auch verwendet werden program/include/rcube_dataclasses/*

Danke inzwischen,

Mike

Thomas Bruederli wrote:

Michael Baierl wrote:

...

There is a convention - an parameter array for receiving data ("named parameters") and returning data as an array. Done. Never touch the input array. This rule is relatively easy to check during QA and it is easy to use and developers are used to that.

Every class you create has to be understood and makes stuff more complicated. KISS.

Amen!

~Thomas

;)

Just got it. I'll look at it in some days ;-)

Thanks.

Hi All,

Here's my patch for managesieve support in RoundCube. I rewrote large portions and replaced many $POST requests with JavaScript. Although this is not my final version it works pretty well. In the next version I'll try to fit in the new plugin architecture and update the back-end to use the PEAR sieve class. Furtermore I'll rewrite some of the functionality that I borrowed from Avelsieve to make translating it easier. Compatibility with the patch from Alec will hopefully also be present in the next version.

I'd really like to hear about bugs and/or improvements (feature requests).

Kind regards, Jeroen Koekkoek

-----Original Message----- From: Florian Lagg [mailto:info@lagg.at] Sent: Thursday, August 07, 2008 7:59 PM To: 'Jeroen Koekkoek'; dev@lists.roundcube.net Subject: AW: [RCD] Sieve

Just got it. I'll look at it in some days ;-)

Thanks.