so basically a set of api functions, and helpers?
can be them included in current Avatar class?

creating inconsistent plugins

i think you mean, for example:
how is the size of avatar managed?, if left free to plugins, which can have mixed sized, that would be a mess.

Probably here it should be done like the column and filter plugins. The plugin provides a object/class that implements a interface/abstract

For example: regarding image size, implements the function:
get_image_url( user_id, heigth, width ).

Instead of having a configuration in the plugin for the image size, the config is in mantis base, and the plugin must comply with the requested size.

Yes, that's exactly my idea

I've been thinking about this some more, and now I wonder if we would not be better off using a kind of "plugin framework", similar to the Source Integration plugin architecture, i.e. a "base" plugin with core functionality, config page, etc, and "child" plugins in charge of specific implentation (Gravatar, local, LDAP, etc).

Or am I over-engineering things ?

Or am I over-engineering things ?

probably yes.... :)

@dregad, I'm not sure about the latest status for this. The currently implementation separates well what is core/common functionality from what is plugin specific. I can see also see a split of core/base/derived as you are suggesting as another valid option.

I would like to trim the "blocking" list and get the release out. I personally don't think it is a blocker. We can always add base classes for plugins later, without breaking plugins that implement the events directly.

Having said that, if there are strong opinions, I would be happy to do it, assuming we make progress on the other issue and get the release out.

I consider it as blocking, because changing this architecture later will force us to implement additional migration mechanism to migrate to future state.

1. old system (i.e. prior to Gravatar plugin)
2. current Gravatar plugin implementation
3. future system (base Avatar + specific implementation)

If we release rc2 now, we'll have to cover 1 -> 3 as well as 2 -> 3

If we implement this "properly" (as I see it) now, it would be sufficient to just do 1 -> 3.

The base plugin approach also has the benefit of providing users a convenient way to manage avatar settings via a config page.

Looking into this, A plugin can only inherit from one base class. So for a case like an Active Directory plugin, you can imagine this will require to inherit from AuthPlugin (to override auth methods) and AvatarPlugin to fetch avatars from active directory.

I wonder if the above is possible.

I also looked at your branch where you had a base class, and the base class seemed to only setup the hooks. It didn't do any common functionality / santization / validation logic.

The approach I had, captured the santization logic as part of the core (static methods on Avatar class). Such logic doesn't force a specific design pattern for how to implement the avatar plugins. As long as the plugin subscribes and processes events. So, even if we do this, there is no real need to migrate already implemented plugins (hence no migration debt). If we do "have to", then we are likely getting to dependent on the base class which may cause issues with the Active Directory example.

Now, if we support having plugins that can implement multiple plugin classes that are supported by our system, then this will be less of an issue.

for a case like an Active Directory plugin, you can imagine this will require
to inherit from AuthPlugin (to override auth methods) and AvatarPlugin to fetch
avatars from active directory.

PHP does not support multiple inheritance, so I'm not sure this is feasible.

I personally do not see this as an issue, as this scenario would easily be covered by having 2 distinct plugins (e.g. AvatarAD and AuthAD).

the base class seemed to only setup the hooks. It didn't do any common
functionality / santization / validation logic.

That branch was (is) a work-in-progress, I only implemented the basics to display Gravatars and had not gotten around to the config page and other common functionality.

The rationale behind my approach was to remove as much from the core as possible, and through the parent/child plugin architecture provide a consistency between plugins and maybe allowing multiple avatar providers to coexist with a common setup (e.g. AD for employees, Gravatar for external customers)

So for a case like an Active Directory plugin, you can imagine this will require to inherit from AuthPlugin (to override auth methods) and AvatarPlugin to fetch avatars from active directory.

PHP does not support multiple inheritance, so I'm not sure this >is feasible.

if so, better define it as interface?

I personally do not see this as an issue, as this scenario would easily be covered by having 2 distinct plugins (e.g. AvatarAD and AuthAD).

I dont like that, as a plugin developer i would prefer to mantain a single plugin, given that the limitation is not there in the first place, and both functionalities are so tightly coupled (eg: getting info from the same AD, duplicating configuration?)

IMO, if the point is to have some utility functions that plugins CAN use: make it a class that can be used by composition (no inheritance required)
If the point is for stuff that MUST be used, make it an interface

So now, i am sorry i havent look in depth at what are really the needs. Do you have a summary of what we are talking about exactly?

The core logic is already refactored in the static methods on the Avatar class. All the common logic is there. So I don't see the problem you are trying to solve here.

I know that PHP and most programming languages don't support multiple inheritance. What I was referring to is being able to have a single plugin have classes that extend from multiple base classes and our infrastructure would understand and create such instances. But there isn't much scenarios requiring that yet.

I'm un-assigning this issue and I don't see agreement on it or it being blocking. I can see us creating an empty base class and move static methods in Avatar class to it. If you are really passionate about this, how about you submit a PR proposing the change that we can review taking into consider the points raised by @cproensa and me.

I still don't like this architecture, but not having the time to offer anything better, I'll rest my case.