[html-formfu] DBIC: setting accessor for a Select element triggers options_from_model()

[Zbigniew Lukasiak]

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 ) ] );
    }
}

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

--
Zbigniew
http://perlalchemy.blogspot.com/