Plugin or controller?

Matt S Trout redirected me to this list to discuss a "new" plugin for Catalyst. But I guess I need to reach back a bit.

I surfed on CPAN yesterday and found the module Catalyst::Controller::RateLimit. One of my first thouhgts was, this module should has been written as a plugin, not as a controller because it extends an application with functionality which is neither business logic nor controller actions.

I then contacted the author and stated my thoughts. Andrey Kostenko ( http://search.cpan.org/~gugu ) answered that he first wrote it as a plugin, but the list suggested to find another namespace. So he has chosen the Controller namespace. He suggested to rewrite the module so that it is based on Algorithm::TokenBucket instead of Algorithm::FloodControl and file it under the Plugin namespace.

To my module proposing Matt answered:

> It's normal for Catalyst plugins to be rewritten to become controllers > - trying to go the other way round strongly suggests a design > mistake to me.

In my opinion, the module Catalyst::Controller::RateLimit should be Catalyst::Plugin::RateLimit. But may be I'm wrong. What does the list think about this?

> You haven't in any way explained why you think it _needs_ to be a > plugin. > > Therefore, it doesn't need to be, and so shouldn't be. ;)

if you argue that way, nothing needs to be anything (because opinions are different) ;).

The module provides a thin wrapper around Algorithm::FloodControl and implements one function that can be called within a controller action to decide, what to do. It's like the Authorization plugin. And because it only implements this helper function, the controller namespace is a bit confusing.

> Hi, > > You haven't in any way explained why you think it _needs_ to be a plugin. >> >> Therefore, it doesn't need to be, and so shouldn't be. ;) >> > > if you argue that way, nothing needs to be anything (because opinions are > different) ;). > > The module provides a thin wrapper around Algorithm::FloodControl and > implements one function that can be called within a controller action to > decide, what to do. It's like the Authorization plugin. And because it > only implements this helper function, the controller namespace is a bit > confusing. > > > matt >

I'm not sure why $self->flood_control is more confusing than $c->flood_control. But I'd also probably stuff this in a Model... $c->model('FloodControl')->register_attempt( $c->controller, $c->user->login || $c->req->address) looks more meaningful to me... again, just opinions.

A controller should determine, via configuration, how it can be flooded. If your application needs such things, I would say that something upstream from Catalyst itself is a better solution (but applying this at a root chain would accomplish the same thing).

If it is a plugin, you couldn't allow different rates on a per-controller basis unless you start giving it per-controller configuration... at which point there is a bad design when a plugin starts being controller specific. Exempting a controller from rate limiting would also be difficult from a plugin perspective. From a model perspective, the same holds true...

There isn't anything in this code that should affect core dispatching or the framework itself, which is what plugins are for (and that was Tom's point, as a general rule of thumb if you can't justify a plugin it should not be a plugin).

> I'm not sure why $self->flood_control is more confusing than $c- > >flood_control. I meant the module's name.

> A controller should determine, via configuration, how it can be > flooded. If your application needs such things, I would say that > something upstream from Catalyst itself is a better solution (but > applying this at a root chain would accomplish the same thing). > > If it is a plugin, you couldn't allow different rates on a per- > controller basis unless you start giving it per-controller > configuration... at which point there is a bad design when a plugin > starts being controller specific. Exempting a controller from rate > limiting would also be difficult from a plugin perspective. From a > model perspective, the same holds true...

Why couldn't that allow different rates on a per-controller basis? Because you define a name for the rates it can be anything. Even, as far as I understand the module Algorithm::FloodControl, the actual value for the rate would then be available from within any controller without using this controller as a base controller wherever I want to access the rate limit. A rate limiting is not always bound to only one controller; it depends on how you use the module.

> There isn't anything in this code that should affect core > dispatching or the framework itself, which is what plugins are for

Hm, I don't see why Plugin::Authorization falls under this but not Controller::RateLimit.

Why couldn't that allow different rates on a per-controller basis? Because > you define a name for the rates it can be anything. Even, as far as I > understand the module Algorithm::FloodControl, the actual value for the rate > would then be available from within any controller without using this > controller as a base controller wherever I want to access the rate limit. A > rate limiting is not always bound to only one controller; it depends on how > you use the module. > > There isn't anything in this code that should affect core dispatching or >> the framework itself, which is what plugins are for >> > > Hm, I don't see why Plugin::Authorization falls under this but not > Controller::RateLimit. > > So, first, there is no such thing as Catalyst::Plugin::Authorization. When you first mentioned it I assumed you simply meant Authorization::ACL or Authorization::Roles. The latter should not be a plugin in my opinion, but a model.

To talk of Authorization::ACL, the answer to why it is a plugin is very simple. It modifies dispatching and extends Catalyst. Before the request is sent to the controller, the aggregated ACL rules are checked on each request and Catalyst's execute method is wrapped. You can't do this in a controller, and it is site-wide.

That makes it a plugin.

If you are trying to access an application wide object, it's a model. As such, I again restate that Catalyst::Controller::RateLimit should simply be either a Catalyst::Model::Adaptor or a customized Model. It shouldn't be a controller or a model, since it does have a benefit of being application wide.

Matthias Dietrich wrote: >> You haven't in any way explained why you think it _needs_ to be a plugin. >> >> Therefore, it doesn't need to be, and so shouldn't be. ;) > > if you argue that way, nothing needs to be anything (because opinions > are different) ;).

No, sorry.

This isn't an arbitrary choice. Some things _NEED_ to be plugins (i.e. need to be composed onto the application's @ISA).

The _only_ things which need that are things which explicitly want to change the way setup works, and even then quite often you can use an application class role.

If it doesn't alter basic framework operation and dispatching in such a way the it _NEEDS_ to be on @ISA for your application class, then it shouldn't be in the Catalyst::Plugin namespace.

The canonical example of this being plugins which provide $c->form = what a massive bucket of fail that one is - changing form system part way through an applications life, or running more than one form system is more common than you'd think.

As you're considering an alternate implementation of flood control to the one which already exists, this also clearly makes a good case that neither of them should be a plugin.

I'd recommend going and re-reading the advice in Catalyst::Manual::ExtendingCatalyst. If after reading that it still isn't clear as to why this should not be a plugin, please tell us how/why/where it should be expanded to be more clear, and I'll do so.

> So, first, there is no such thing as > Catalyst::Plugin::Authorization. When you first mentioned it I > assumed you simply meant Authorization::ACL or > Authorization::Roles. The latter should not be a plugin in my > opinion, but a model.

sorry, I meant Plugin::Authentication - sorry for that! But as I understand it, not every plugin module on CPAN should be a plugin? But when this is based on opinions, who decides which is a plugin and which not?

> If you are trying to access an application wide object, it's a > model. As such, I again restate that > Catalyst::Controller::RateLimit should simply be either a > Catalyst::Model::Adaptor or a customized Model. It shouldn't be a > controller or a model, since it does have a benefit of being > application wide.

I guess you meant "It shouldn't be a controller or a plugin", right?

Don't get me wrong, I don't want to create this as a plugin under all circumstances. I just want to figure out when a module is a plugin, when a model and when a controller. But I'm getting closer to the truth ;).

There are many 'plugins' that really never should have been plugins. The methods described in the Extending Catalyst doc were not always public knowledge and quite a lot got written as plugins when they should have been something else.

Do not make the mistake of thinking of Catalyst Plugins in the same way you think of WordPress or MovableType or Drupal plugins. They are not the same thing at all.

Think more along the lines of apache modules. Plugins are things that affect the basic functionality of Catalyst, not things that just add a feature here or there. If it effects core functionality, and _has_ to be a plugin to access the parts of Catalyst and it's dispatch process to do it's job, then it should be a plugin... if it doesn't _have_ to be a plugin in order for it to do it's job, it should not be a plugin.

The fact that the rate limiter _could_ be written as a controller indicates that it should NOT be a plugin. It doesn't necessarily need to be a Controller, but it doesn't rely on Cat internals / Dispatch process, so it should not be a plugin.

Also, plugins affect the core of catalyst and their methods are added to the $c object. The more plugins you have, the more likely you are to have conflicts and failures / incompatibilities.. It simply does not make sense to add anything as a plugin unless it absolutely has to be.

There are numerous 'plugins' that don't need to be plugins, but are still for various reasons. Most of the Authentication system, for example, got moved out of the Plugin namespace almost two years ago. Only the initial setup hook portion remains as a plugin.

Hope that helps,

Jay

On Jun 1, 2009, at 11:11 AM, Matthias Dietrich wrote:

> Hi, > >> So, first, there is no such thing as >> Catalyst::Plugin::Authorization. When you first mentioned it I >> assumed you simply meant Authorization::ACL or >> Authorization::Roles. The latter should not be a plugin in my >> opinion, but a model. > > sorry, I meant Plugin::Authentication - sorry for that! But as I > understand it, not every plugin module on CPAN should be a plugin? > But when this is based on opinions, who decides which is a plugin > and which not? > >> If you are trying to access an application wide object, it's a >> model. As such, I again restate that >> Catalyst::Controller::RateLimit should simply be either a >> Catalyst::Model::Adaptor or a customized Model. It shouldn't be a >> controller or a model, since it does have a benefit of being >> application wide. > > I guess you meant "It shouldn't be a controller or a plugin", right? > > Don't get me wrong, I don't want to create this as a plugin under > all circumstances. I just want to figure out when a module is a > plugin, when a model and when a controller. But I'm getting closer > to the truth ;). > > matt > > -- > rainboxx Matthias Dietrich > Freier Software Engineer > > rainboxx | Tel.: +49 (0) 151 / 50 60 78 64 > Tölzer Str. 19 | Mail: matt [at] rainboxx > 70372 Stuttgart | WWW : http://www.rainboxx.de> > XING: https://www.xing.com/profile/Matthias_Dietrich18> GULP: http://www.gulp.de/profil/rainboxx.html> > > > _______________________________________________ > Catalyst-dev mailing list > Catalyst-dev [at] lists > http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst-dev

Matthias Dietrich wrote: > Don't get me wrong, I don't want to create this as a plugin under all > circumstances. I just want to figure out when a module is a plugin, > when a model and when a controller. But I'm getting closer to the > truth ;). > > matt Disclaimer: IANACD (Core Developer) but here's my take.

A plugin changes fundamental functionality. By fundamental I mean mostly the request dispatch. For example C::P::Static skips the whole dispatch process, it just serves static files when the path prefix matches. And, as pointed out before, C::P::Authorization::* also alters the dispatch if the authz conditions are not matched. A plugin is loaded in the main app module (lib/MyApp.pm).

A controller base adds functionality. C::C::HTML::FormFu allows you to extend one or more of your controllers with HTML::FormFu functionality (displaying, processing and validating forms). C::C::RequestToken extends your controllers with token generating functionality. A controller base class is mostly used as a parent/base to the actual controller class and usually provides new methods to $self.

Finally, a model provides data. Be it from a database, XML feed, (XML)RPC, etc. it will supply data on demand. A model is accessed explicitly by: $c->model('MyModel'). A model's data source can (and sometimes will) be used outside the web framework too (from a command line script, cron job, etc.)

* Matthias Dietrich <mdietrich [at] cpan> [2009-06-01 19:15]: > But as I understand it, not every plugin module on CPAN should > be a plugin? But when this is based on opinions, who decides > which is a plugin and which not?

Itâ€™s *NOT* based on opinions.

There are objective technical reasons for whether something should be a plugin or not.

The benefit of making something a plugin is access to some Cat internals and to parts of the dispatch process that other mechanisms for adding features do not offer.

The drawback is that itâ€™s easy for plugins to conflict because they all become part of the same global namespace.

Most features do not require such access to internals. There is no point in incurring the problems caused by plugins if there is nothing to gain from implementing something as a plugin.

If something needs the access to Cat internals that plugins get, then and ONLY then it should be a plugin.

Most people who put Catalyst extensions on CPAN made them plugins anyway in the past, because most people, including at the time the authors of the Cat documentation, didnâ€™t really know better.

On Mon, Jun 01, 2009 at 05:50:57PM +0200, Matthias Dietrich wrote: > Hi, > > >You haven't in any way explained why you think it _needs_ to be a > >plugin. > > > >Therefore, it doesn't need to be, and so shouldn't be. ;) > > if you argue that way, nothing needs to be anything (because opinions > are different) ;). > > The module provides a thin wrapper around Algorithm::FloodControl and > implements one function that can be called within a controller action > to decide, what to do.

In which case it doesn't need to wrap the request cycle so it should be a controller superclass, not a plugin.

If the logic gets complicated I'd then make it a controller superclass that delegates to a model, but I'm not sure in this case it's complex enough to be worth doing that.

> It's like the Authorization plugin.

Authorization::Roles probably shouldn't have been a plugin either.

I always just use methods on the user object ...

> And > because it only implements this helper function, the controller > namespace is a bit confusing.

I'd probably implement it as a role rather than a superclass for 5.80.