[html-formfu] (ping Andreas + zby) Re: DBIC: setting accessor for a Select element triggers options_from_model()

[Carl Franks]

On 28/02/2008, Zbigniew Lukasiak <zzbbyy at gmail.com> wrote:
> On Wed, Feb 27, 2008 at 11:01 PM, Carl Franks <fireartist at gmail.com> wrote:
>  > On 22/02/2008, Andreas Marienborg <omega at palle.net> wrote:
>  >  >
>  >  >  On Feb 22, 2008, at 12:08 PM, Carl Franks wrote:
>  >  >
>  >  >  > On 21/02/2008, Steve Caldwell <info-formfu at caldwellhb.com> wrote:
>  >  >  >> I'm building a form that has a Select element that I'm mapping to a
>  >  >  >> non-column accessor on my DBIC object, as described in "non-column
>  >  >  >> accessors" in the HTML::FormFu::Model::DBIC POD, like this:
>  >  >  >>
>  >  >  >> ---
>  >  >  >> action: /foo
>  >  >  >> elements:
>  >  >  >>  - type: Select
>  >  >  >>    name: bar
>  >  >  >>    label: Your Bar
>  >  >  >>    db:
>  >  >  >>      accessor: bar
>  >  >  >>    options:
>  >  >  >>      - [ 01, January ]
>  >  >  >>      - [ 02, February ]
>  >  >  >>
>  >  >  >> When I then try and populate it from my DBIC object:
>  >  >  >>
>  >  >  >> $form->defaults_from_model( $myobj );
>  >  >  >>
>  >  >  >> HTML::FormFu::Model::DBIC::options_from_model gets called (thanks to
>  >  >  >> line 44 in HTML::FormFu::Element::_Group), which then overwrites the
>  >  >  >> options I've set from my select list with a bunch of stuff from the
>  >  >  >> database.  This is obviously not what I want here.
>  >  >  >>
>  >  >  >> Anyone else come across this problem?  The work-around isn't that
>  >  >  >> much
>  >  >  >> work (manually populate from and save to my object instead of using
>  >  >  >> defaults_from_model() and save_to_model()), but it would be cooler
>  >  >  >> if it
>  >  >  >> all worked correctly.
>  >  >  >
>  >  >  > I agree that this is broken.
>  >  >  > I think Select automatically calling options_from_model() is a case of
>  >  >  > *too much magic*
>  >  >  >
>  >  >  > (sidenote) - I know you've only just subscribed to the list, so a
>  >  >  > quick explanation, so that you understand my following proposal:
>  >  >  > FormFu in svn has been changed so that you set Model/DBIC options
>  >  >  > with:
>  >  >  >    $field->{model_config}{DBIC}
>  >  >  > instead of:
>  >  >  >    $field->{db}
>  >  >  >
>  >  >  > This will be in the next cpan release. However, setting {db} will
>  >  >  > still work, and Do The Right Thing, it'll just print a warning that
>  >  >  > it's deprecated and will be removed at some point in the future.
>  >  >  >
>  >  >  > The reason for this, is the model was implemented as a bit of a
>  >  >  > rush-job and we need to get it working properly to support multiple
>  >  >  > models - much like how Catalyst handles models.
>  >  >  > (/sidenote)
>  >  >  >
>  >  >  > I suggest we require that a new option be set to run
>  >  >  > options_from_model(), such as:
>  >  >  >
>  >  >  >    ---
>  >  >  >    element:
>  >  >  >      type: Select
>  >  >  >      name: foo
>  >  >  >        model_config:
>  >  >  >          DBIC:
>  >  >  >            options_from_model: 1
>  >  >  >
>  >  >  > (or, in the old, deprecated, style):
>  >  >  >
>  >  >  >    ---
>  >  >  >    element:
>  >  >  >      type: Select
>  >  >  >      name: foo
>  >  >  >        db:
>  >  >  >          options_from_model: 1
>  >  >  >
>  >  >  > Any opinions on this?
>  >  >
>  >  >
>  >  > Could the default be to run it if:
>  >  >
>  >  >  - no options_from_model: 0 (false) exists
>  >  >  AND
>  >  >  - no options already set on the select?
>  >
>  >  okay, we need to revisit this :(
>  >
>  >  I've moved HTML/FormFu/Model/DBIC.pm out into a separate distribution.
>  >  $form->model_class() has been renamed to default_model() and no longer
>  >  defaults to 'DBIC'.
>  >  (more on that to come in another mail!)
>  >
>  >  However - there's still DBIC-specific code in Element/_Group.pm to
>  >  handle options_from_model()
>  >  - this is bad, and needs dealt with.
>  >
>  >  I can see 2 ways to deal with this:
>  >
>  >  1) Create a new system to provide hooks that model classes can attach
>  >  to, so the user doesn't need to do anything.
>  >
>  >  2) Use the hooks provided by the current plugin system, and require
>  >  the user to specifically add it to the Select field.
>  >  e.g.
>  >     ---
>  >     type: Select
>  >     name: foo
>  >     plugins: [ 'DBIC::LoadOptions' ]
>  >     model_config:
>  >       DBIC:
>  >         model: Foo
>  >
>  >  I'm inclined to prefer solution 2, because I foresee the following
>  >  problems with solution 1:
>  >  - because we currently don't require the user to load the models in
>  >  the form config, there's no guarantee that the model will be available
>  >  during process(), so the hooks won't get run anyway.
>  >  - adding yet-more hooks will make the formfu internals more complex
>  >
>  >  Any thoughts?
>
>
> Here is my proposal:
>
>  sub post_process {
>     my $self = shift;
>
>     $self->next::method(@_);
>
>     my $args = $self->model_config;
>
>     # only call options_from_model if there's no options already
>     # and {options_from_model} isn't 0
>
>     my $option_count = scalar @{ $self->options };
>
>     my $option_flag = exists $args->{options_from_model}
>         ? $args->{options_from_model}
>         : 1;
>
>     if ( $option_count == 0 && $option_flag  != 0 ) {
>         $self->options(
>             [ $self->form->model( $args->{model_name}
>  )->options_from_model( $self, $args ) ] );
>     }
>  }

Okay, I can dig that - lets do it!
Do you want to make the necessary changes?
(I'm gonna be off-line for the next week, so won't be able to do it
until after then).

>  Minimal change from current.  I haven't got any intuition about using
>  multiple models in one form - but if we allow for one default model -
>  then the last significant line could be:
>  [ $self->form->model()->options_from_model( $self, $args ) ] );
>
>  The model should know that it is a DBIC model and choose the right
>  part of $args passed to it.  Or even you could get rid of that level
>  in the hash - as it does not really make sense to have both DBIC and
>  LDAP options for one field:
>
>  - type: Select
>     name: type2_id
>     model_config:
>       DBIC:
>         resultset: Type2
>       LDAP:
>         some_ldap_option: value
>
>  Does that make sense?  I cannot imagine any situation when this would
>  be needed.  So I would say just do:
>
>  - type: Select
>     name: type2_id
>     model_config:
>       model_name: DBIC
>       resultset: Type2

Okay, I don't like it - but I'm inclined to agree that it'll be by far
the most common usage to only use 1 model n any app - so lets make it
easier.

I don't imagine a form updating 2 different models at once, but I can
imagine scenarios where a single element definition might have to be
aware of 2 models.

Andreas - you've been doing some LDAP model work - do you envisage any
problems with different models sharing the same level off of model_config?

If not, lets just go back to that then.

Cheers,
Carl