Bug #40733 for Catalyst-Plugin-Form-Processor: This distribution should not exist; please rewrite as a controller base class

Thu Nov 06 16:48:45 2008 mst [...] shadowcat.co.uk - Ticket created

Subject:	This distribution should not exist; please rewrite as a controller base class
Date:	Thu, 6 Nov 2008 21:09:30 +0000
To:	bugs-Catalyst-Plugin-Form-Processor [...] rt.cpan.org
From:	Matt S Trout <mst [...] shadowcat.co.uk>

Form validation should never be done via a plugin since it eliminates code reusability across controllers using different form validators. Please rewrite this as Catalyst::Controller::Form::Processor in line with how the FormFu and FormBuilder equivalents work and deprecate this distribution. -- Matt S Trout Need help with your Catalyst or DBIx::Class project? Technical Director http://www.shadowcat.co.uk/catalyst/ Shadowcat Systems Ltd. Want a managed development or deployment platform? http://chainsawblues.vox.com/ http://www.shadowcat.co.uk/servers/

Fri Nov 07 03:54:22 2008 mods [...] hank.org - Correspondence added

On Thu Nov 06 16:48:45 2008, mst@shadowcat.co.uk wrote: Show quoted text

> Form validation should never be done via a plugin since it eliminates > code reusability across controllers using different form validators.

Can you explain how using a plugin eliminates code reusability? Because $c->form ends up application-wide? Is it common for people to use different form validators in the same application? Show quoted text

> > Please rewrite this as Catalyst::Controller::Form::Processor in line with > how the FormFu and FormBuilder equivalents work and deprecate this > distribution. >

Feel free to write a controller base class if you desire one. I wrote one for Form::Processor once and decided I liked the Plugin better. I guess I was too lazy to alter all my existing controller classes to inherit from it.

Fri Nov 07 03:54:24 2008 The RT System itself - Status changed from 'new' to 'open'

Fri Nov 07 03:54:25 2008 mods [...] hank.org - Status changed from 'open' to 'rejected'

Mon Nov 17 11:41:55 2008 mst [...] shadowcat.co.uk - Correspondence added

Subject:	Re: [rt.cpan.org #40733] This distribution should not exist; please rewrite as a controller base class
Date:	Mon, 17 Nov 2008 16:41:03 +0000
To:	Bill Moseley via RT <bug-Catalyst-Plugin-Form-Processor [...] rt.cpan.org>
From:	Matt S Trout <mst [...] shadowcat.co.uk>

On Fri, Nov 07, 2008 at 03:54:25AM -0500, Bill Moseley via RT wrote: Show quoted text

> <URL: http://rt.cpan.org/Ticket/Display.html?id=40733 > > > On Thu Nov 06 16:48:45 2008, mst@shadowcat.co.uk wrote:

> > Form validation should never be done via a plugin since it eliminates > > code reusability across controllers using different form validators.

> > Can you explain how using a plugin eliminates code reusability? Because > $c->form ends up application-wide?

Right. Show quoted text

> Is it common for people to use different form validators in the same > application?

It can be if people are trying to re-use components. Also, plugins should only really be things that affect the request cycle - ExtendingCatalyst.pod makes this clear. Show quoted text

> > Please rewrite this as Catalyst::Controller::Form::Processor in line with > > how the FormFu and FormBuilder equivalents work and deprecate this > > distribution. > >

> > Feel free to write a controller base class if you desire one. I wrote > one for Form::Processor once and decided I liked the Plugin better. I > guess I was too lazy to alter all my existing controller classes to > inherit from it.

I don't use Form::Processor. I'm filing this because the controller style is correct practice and we'll be making a strong push at 5.80 to get people to stop using old-style stuff that abuses the plugin system; I intend to provide a module that includes a list of "poor practice" extensions and warns on startup if you're using any of them, and this one will be on that list. -- Matt S Trout Need help with your Catalyst or DBIx::Class project? Technical Director http://www.shadowcat.co.uk/catalyst/ Shadowcat Systems Ltd. Want a managed development or deployment platform? http://chainsawblues.vox.com/ http://www.shadowcat.co.uk/servers/

Mon Nov 17 11:41:59 2008 The RT System itself - Status changed from 'rejected' to 'open'

Sat Nov 29 11:58:58 2008 mods [...] hank.org - Correspondence added

On Mon Nov 17 11:41:55 2008, mst@shadowcat.co.uk wrote: Show quoted text

> Also, plugins should only really be things that affect the request > cycle

There's a fill-in-form component in this plugin that happens after dispatching. There's also setup() code that runs once per application (although it's probably not necessary). Gerda wrote a Controller base class and she added an end() action to handle the fill-in-form. She noticed that the end action class no longer ran (of course) and tried to fix that in the controller base class by adding the default ActionClass('RenderView'). That would break apps that have a custom end() method. For a situation like this are you recommending that the package then includes both a controller base class AND a plugin? Or is there a way to handle both of those issues (setup and post-render) in the controller? Show quoted text

> I'm filing this because the controller style is correct practice and > we'll > be making a strong push at 5.80 to get people to stop using old-style > stuff > that abuses the plugin system; I intend to provide a module that > includes > a list of "poor practice" extensions and warns on startup if you're > using > any of them, and this one will be on that list.

I'll have to ask on the list, but is this an issue of how you feel it's best or is there a real technical issue at hand? The extra application base class isn't a performance issue for me. Having $c->form would be a problem if using two form systems but I doubt anyone really does that in practice.

Sat Nov 29 13:12:23 2008 mods [...] hank.org - Correspondence added

On Sat Nov 29 11:58:58 2008, HANK wrote: Show quoted text

> Gerda wrote a Controller base class and she added an end() action to > handle the fill-in-form. She noticed that the end action class no > longer ran (of course) and tried to fix that in the controller base > class by adding the default ActionClass('RenderView'). That would break > apps that have a custom end() method.

BTW -- I'll work with Gerda to get a Controller (and View) base class available. Shouldn't be much work. I'll continue to use and support the Plugin as for me the convenience of the plugin outweighs the risk of clashing with $c->form.

Sun Feb 08 12:27:58 2009 bobtfish [...] bobtfish.net - Correspondence added

On Sat Nov 29 11:58:58 2008, HANK wrote: Show quoted text

> Having $c->form would be a > problem if using two form systems but I doubt anyone really does that in > practice.

I for one have a legacy application which uses Catalyst::Plugin::FormBuilder. I'm planning to gradually refactor it to use Gerda's Form::Processor, one controller at a time. That obviously precludes me using this distribution, unless I were to port my application to Catalyst::Controller::FormBuilder first, which is just _silly_. I think you'll find this _is_ a real problem for people who want to refactor their DarkPAN applications.