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

[Andreas Marienborg]

On Apr 3, 2008, at 4:47 PM, Carl Franks wrote:

> 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?

  can't even see where one would use both models in the same app atm :)

Do we even need to mention the LDAP/DBIC stuff in the config?

When I do things now, I do $form->model('DBIC/LDAP')->update($item); - 
 >default_values($item) etc

so basicly the model_config should be model agnostic?

- type: Select
name: blah
model:
    resultset: BlahSet

or keep model_config as the name perhaps, as to not change too much.

- andreas