Dave Howorth wrote:
> Simon Flack wrote:
>
>> I'd appreciate some feedback on this this release before I upload it
>> to the CPAN.
>
>
> Here's what I looked at today ...
Thank you for spending some time testing this, and for the helpful
comments that follow...
>> Here's the relevant block from the Changes file:
>>
>> Internal changes:
>> - Removed Maypole::Model->description.
>
> Excellent. My Description columns work :)
>
>> Fixes:
>> - Model->process() shouldn't set $r->objects() to a list with a
>> single, undefined element
>
> This works, I think.
>
>> - Fixed overriding $r->template_args->{classmetadata} in M::V::Base
>
> This works.
>
>> (Thanks to Dave Howorth for spotting the mistake)
>
> This works.
>
>> - #9473: Maypole::Model::CDBI->related_class (Thanks David Baird)
>
> This works.
>
>> - #9434: M::M::CDBI->search generated "uninitialized value" warnings
>
> Haven't seen any today :)
>
>> Templates:
>
>
> M::V::TT still doesn't allow .tt extensions :(
I keep changing my mind about this. I think it'd be useful to be able to
define a template file extension, and it's a relatively trivial patch,
but I'm not sure it belongs in Maypole. I half expected there to be a
TEMPLATE_EXT option in TT, but I couldn't find one. I'm not ruling out
adding $r->config->template_ext, I just need a bit of convincing :)
Out of interest, would you expect/desire nested templates to have a .tt
extension added? E.g.:
[% IF foo;
PROCESS another_template; # load "another_template.tt"
END; %]
> It's difficult for me to test the templates because mine look
> considerably different now. (I've done away with classmetadata.cgi
> altogether since it just duplicates to_field; I use my loader to
> generate text strings; I use some modified accessor_names (these break
> the view_item macro BTW); I check explicitly for primary keys not just
> 'id' cols; ...)
I'd like to see that in the factory templates and in
Maypole::Manual::(Request|StandardTemplates) too.
> So I'm just reading the code and modifying my templates rather than
> actually running these templates. Results may vary.
>
>> - The addnew template will attempt to prefill form fields with
>> request parameters
I should probably qualify "attempt". It should work for <textarea> and
<input> fields (and other form fields that use the "value" attribute to
set the default value). But it doesn't yet prefill radio buttons. That
might not be a problem since CDBI::AsForm doesn't generate radio buttons.
>
> This seems to work.
>
> I take it this is for use when errors arise and addnew is now included
> by the edit template?
Yep.
> BUT on the list page it also appears to cause search parameters to be
> transferred to the addnew box after pressing 'search'. This seems wrong.
I noticed that, and I thought I'd fixed it by cloning the HTML::Element
returned by classmetadata.cgi.$col:
SET elem = classmetadata.cgi.$col.clone;
Do you have that line in your 'addnew' template?
> > - edit template includes 'addnew' if there is no object to edit
>
> M::MM::CDBI::do_edit() still runs create/update_from_cgi in an eval and
> puts fatal errors in errors.FATAL. I still think that is wrong; I think
> there should at a minimum be an Apache error log entry for a fatal error.
I'll add a warn() for that and we can revisit it later.
> If you do think that users should see Perl error messages (I don't :),
> errors.FATAL needs to be displayed in the template. At present fatal
> untaint errors are still silent.
>
> There's a beer/addnew as well as a factory/addnew. beer/addnew looks out
> of date, and I don't remember if there's a reason it should exist at all?
I can't see why there is one either. Perhaps it is a useful test/demo
that you can override a factory template with a model template.
Thanks
Simon