View Issue Details

IDProjectCategoryView StatusLast Update
0020215mantisbtplug-inspublic2016-01-26 03:52
Reportervboctor Assigned To 
PrioritynormalSeverityfeatureReproducibilityN/A
Status newResolutionopen 
Product Version1.3.0-beta.3 
Summary0020215: Use objects for event parameters instead of arrays
Description

Currently if an event takes two strings as a parameter we provide array( string, string ). It would be much developer friendly if we pass in objects and expects objects back.

TagsNo tags attached.

Activities

cproensa

cproensa

2015-10-25 15:59

developer   ~0051722

At the plugin side i think this wont change anything.
if you signal the event:
<pre>
event_signal( EVENT, array( p1, p2, p3) )
</pre>

the plugin recieves a call to
<pre>
function registerd_func (event, p1, p2, p3)
</pre>

by means of call_user_func_array(), the array is tunneled to the function as parameters, so the keys are lost in the way
And: even with the descriptive keys, order still matters.

Only benefit i think would provide: if you look at base source to see how the event is signaled... but documentation should explain parameters to not need to do that.

I may be wrong, though. i'm just thinking out loud

vboctor

vboctor

2016-01-24 22:00

manager   ~0052371

Let's take the avatar example. We can do the following:

  • Core calls event_signal( event_name, instance of AvatarEventRequest)
  • Plugin returns AvatarEventResponse

This has the following advantage:

  • Objects are more readable than arrays.
  • We can have methods / validation / normalization / etc. For example, in case of avatar, I have a normalize method that normalizes all fields, defaults unset fields, etc. This way the core code can just consume the fields without having to worry about validation or a field being optional.
  • Can easily add more arguments and return more data.
  • Developers have a place in the code to go look for documentation about expected input/output for their events.

See PR for an example towards this direction. However, in this PR, I so far used an object as a result, but not as an input to the event.

https://github.com/mantisbt/mantisbt/pull/711

vboctor

vboctor

2016-01-24 22:07

manager   ~0052372

Reminder sent to: atrol, dregad

What do you think about this?

cproensa

cproensa

2016-01-25 04:25

developer   ~0052374

Last edited: 2016-01-25 05:29

Victor, I agree with your idea of using objects, with the benefits you explained, basically data and behaviour encapsulation
However i'd rather go a step further:

  • Define Avatar as interface, instead of class. This way a predefined set of methods are there to be implemented by a object created from user plugin, that provides the needed functionality.
  • Create an abstract class (eg, MantisAvatar), or a class with some abstract methods, that implements said interface. That way it can be used by a plugin to inherit the predefined functionality and override partial, or totally.

I prefer defining interfaces, because that would allow a class to implement several interfaces, otherwise with plain classes it would not be possible (multiple inheritance is not a php feature, iirc)

Also in a more general aplication of this scenario, my point is to make this interface the unified entry point for some behaviour.
In a more complex example, what could be done to unify column fields, across native, custom, plugins...? (this may be a solution to get rid of custom function)

  • define a ColumnInterface
  • create a class for GenericColumn implementing ColumnInterface
  • create a set of classes that override GenericColumn, for each specialized field type, eg: PriorityColumn extends GenericColumn and overrides the printing of its value to show the icon instead of text.
  • Plugins can register its own provided objects in core, those objects implements ColumnInterface and thats all that mantis should care about.
  • Moreso, we could allow a plugin could create a CustomPriorityColumn wich extends PriorityColumn and overrides the printing function to show different icons. Plugin could register this object as the one used to manage the priority colum, instead of the mantis default one.

To summarize:
I'd define interfaces as the object signatures used in code base.
Have some kind of Object factory for core interfaced objects, which would provide the standard one by default or the plugin provided if it exists.
This "object request" don't need to be done at the same place that is using the functionality, then it can be decoupled in location, and time.
No need for several plugin events, as this could be reversed, eg: plugin init ask mantis to register some custom objects.
(this is similar to current MantisColumn functionality, but expanded for extensibility)

dregad

dregad

2016-01-25 06:15

developer   ~0052375

On the subject of Avatar Plugin

  • Create an abstract class (eg, MantisAvatar), or a class with some abstract methods, that implements said interface. That way it can be used by a plugin to inherit the predefined functionality and override partial, or totally.

This is exactly the approach I followed for my own implementation of the same functionality [1]. As mentioned in the PR, I need to spend some time to review and compare my work with Victor's and I think we should come up with a hybrid approach.

On the idea of using objects, this is a good idea; cproensa's suggestion makes sense too, even though it requires more effort to properly desgin and implement, so maybe a phased approach (objects now, interfaces later) would be an alternative.

[1] https://github.com/dregad/mantisbt/tree/avatar

vboctor

vboctor

2016-01-25 11:59

manager   ~0052376

Last edited: 2016-01-25 12:02

@cproensa I generally agree with your suggestions, I think there is a lot of core code that can benefit from that. What you are advocating is basically polymorphism and dependency injection!

However, I don't necessarily like interfaces for:

  • What we are modeling in case of Columns is an "is-a" relationship. Hence, PriorityColumn should inherit from Column and mix-in other interfaces as needed.
  • If you add a method for an interface, you can't provide default behavior for it, hence, all derived code (and specially plugins) will break. However, with an abstract class you can define default behavior and hence keeping backward compatibility.

Back to the Gravatar plugin:

  • @dregad, I reviewed your implementation and brought in some tweaks. There was a couple of possibly useful changes that seemed unrelated (e.g. some installer change to not log SQL) that I didn't pick up.
  • I haven't deprecated the user_get_avatar() and print_avatar(), but made them call the plugin. Could potentially look at that if you think we should mark them as deprecated.
  • I use a single event instead of two events for your plugin.
  • My abstraction (class) is Avatar rather than Avatar plugin. We could add an AvatarPlugin but all it would add is registration for RESOURCES and AVATAR events, and then the derived avatar may want to hook into more, so we will have to support that too.
  • If we want objects in and objects out, then we can pass in an object that contains the user id for now, but maybe more later, e.g. AvatarEventRequest and have AvatarEventResponse and wraps Avatar object. The advantage of this model is that developers can go from an event name to a very specific request/response self documenting format in the code.

Anyway, have a look at the code and let me know if you have specific feedback and I'll address it.

cproensa

cproensa

2016-01-25 12:33

developer   ~0052379

@cproensa I generally agree with our suggestions, I think there is a lot of core code that can benefit from that. What you are advocating is basically polymorphism and dependency injection!

Right. Polymorphism. That way application code does not care of who is implementig the behaviour behind the object (can be core or a user plugin). All we know is we request actions ang get the things done.
Now we request actions through events. Events are our informal interface.

  • If you add a method for an interface, you can't provide default behavior for it, hence, all derived code (and specially plugins) will break. However, with an abstract class you can define default behavior and hence keeping backward compatibility.

The interface is the agreement of indispensable functionality needed to work. If a new method is introduced, the plugin will fail to run, which is not bad after all, becasue mantis wont run properly without that new functionality.

Probably it can be done with abtsract classes all the way. It's more relevant the alternate scenario of OO behavour, vs current procedural events.

Back to the Gravatar plugin:
Dregad uses abtrac class, and is implemented by the plugin class itself. It seems irrelevant outside plugin scope, as the behaviour is accessed throug the usual events_signal

Victor uses Avatar class, but is using static method Avatar::get(), wich i can't see how that could be overridable by a user plugin....

cproensa

cproensa

2016-01-25 17:26

developer   ~0052383

Sorry guys, for going in circles with polymorphism and that stuff.
i have been lately working too much with column/filter api and plugins,
That pattern makes sense in a more complex scenario, like that of filter/column/fields, but not worthy here.

For the current issue, the avatar plugin, i have reviewed carefully Victor proposal, and i think i like the way it goes.
I'd add these suggestions:

  • I'd change the "normalize" method. probably into a factory type syntax "get_default", have it to return a new Avatar object filled with mantis defaults (no avatar site: generic image, etc).
  • get() is a satic factory that ask for plugin data object, or returns a default object if no plugin is found. (Thats what you have done). Maybe, make constructor private/protected, so that using get() is mandatory?
  • Gravatar plugin, it's ok. Still, separate now the config options. Keep a system-wide ON/OFF, and move the specific gravatar configs to a plugin configuration page (eg: icon themes).
vboctor

vboctor

2016-01-25 21:23

manager   ~0052385

@cproensa:

  • I'd change the "normalize" method. probably into a factory type syntax "get_default", have it to return a new Avatar object filled with mantis defaults (no avatar site: generic image, etc).

Think of Avatar::get() is the contract between the MantisBT core and the extensibility point. The advantage of the model I'm using is that the get() method does the necessary access and configuration checks, decides whether to signal the event, and then when it signals the event it validates and complements the result from the plugins which providers the callers with guaranteed valid and well populated object. It also enables conditional defaulting based on plugin provided data or only doing the cost of extra lookups if the plugin doesn't provide the field. So the goal is not to construct an object and default it only.

  • get() is a satic factory that ask for plugin data object, or returns a default object if no plugin is found. (Thats what you have done). Maybe, make constructor private/protected, so that using get() is mandatory?

I can do that.

Gravatar plugin, it's ok. Still, separate now the config options. Keep a system-wide ON/OFF, and move the specific gravatar configs to a plugin configuration page (eg: icon themes).

We should eventually add config page, but given that we already had support for a config option that have this setting, I'm using the same config option to start with to set the Gravatar plugin configs. We can add the page UI in a another iteration of the plugin.

vboctor

vboctor

2016-01-25 21:29

manager   ~0052386

Actually, the private constructor won't work, since the Avatar instances are created by the plugin and just validated and normalized by the get() method. We only create the Avatar objects within the get() method when avatars are disabled or user doesn't have access level, or when the event_signal() returns null.

vboctor

vboctor

2016-01-25 21:36

manager   ~0052387

Another idea to throw in the mix of future way to structure plugins.

abstract class MantisPlugin
{
// virtual method: OnUserCreatedResult OnUserCreated(OnUserCreatedArgs args)
}

With this approach, we get the object benefits, the base class as a way to discover events that can be overridden, get away from string event names, etc.

There will be an EventDispatcher, that looks would handle the different event types (FIRST, DEFAULT, ...etc) based on calling registered MantisPlugin. So registered is on the plugin level, rather than per event like what we do in the hook() method right now.

cproensa

cproensa

2016-01-26 03:52

developer   ~0052390

Think of Avatar::get() is the contract between the MantisBT core and the extensibility point... etc

yes, no complaint about that

I meant, that in order to get a defaulted content, instead of doing:
$a = new Avatar();
$a->normalize();

do something like:
$a = Avatar::get_default()

Actually, the private constructor won't work

you are right...