fuel / core

Fuel PHP Framework - The core of the Fuel v1 framework
http://fuelphp.com
813 stars 345 forks source link

Use views for fieldset form definitions #808

Closed WanWizard closed 12 years ago

WanWizard commented 12 years ago

The fieldset class, in combination with an ORM model, allows you to automatically generate a form for you, based on the definitions in the config/form.php config file.

This works fine, but the forms are pretty basic and not very flexible. You can use set_config() to load alternative definitions, but all forms generated will be based on "one label/field combo per row" as per design of the config and the build method.

I propose a change that allows you to create a complete form template as a view, which will then be used by the build() process to generate the form, giving you total freedom as to how you design it. If no view is presented, the current behaviour will remain unchanged.

WanWizard commented 12 years ago

@jschreuder Was discussed in IRC tonight. Any thoughts on it?

jschreuder commented 12 years ago

I think you guys have been missing a pretty important feature: you can also "build" individual fields, you don't have to build the full form - I never do. The full form generation was always meant as a Q&D method as any decent form will need too much customization to handle everything the same way. Thus I pass a form instance to my view and echo the fields individually inside it. Which is exactly the functionality you describe, except without the hardcoded coupling between the View and Fieldset classes.

WanWizard commented 12 years ago

I'm not sure that is the case. I also have views that use

echo $form->field('person_initials')->build();

This generates

<dt><label for="form_person_initials">Initialen</label></dt> <dd><input type="text" required="required" id="person_initials" name="person_initials" value="" /> </dd>

Which is useless in the cases at hand, as that still would not allow me to add three "input" tags within that "dd" tag.

jschreuder commented 12 years ago

You can configure the template used for form building at the app level (checkout config file in core/config/form.php), at the form level (using $form->set_config()) and at the field level ($field->set_template()). Thus that isn't really a problem.

WanWizard commented 12 years ago

Ah, set_template() at field level was new to me. Time to dive into the code.

And if so, set a reminder to document this, because I have a feeling nobody uses it because nobody understands how it works and how it should be used...

edit: that explains, Fieldset_Field isn't documented anywhere...

WanWizard commented 12 years ago

Looked at set_template(), but for me that's not a solution.

To deal with my use-case, you need to set the templates on the first field to open the group, no template and no label content on the second field, and no label but a template to close the group on the last field.

This is so complex that no user is ever going to use it. Only workable solution is to remove all templating, use "field_template" only to list the tags you want generated, code the rest of the HTML by hand, and use $form->field('name')->build() to add the form field.

And still not 100% as you can't fetch the label independently from the input tag, which can only be worked around by calling build() twice, once with a template that only outputs the label, and once with a template that only outputs the field.

Savageman commented 12 years ago

This is kind of related with an old topic I created in the forum.

http://fuelphp.com/forums/topics/view/5269

I have the same "multi-field" problematic. One label for several fields. In my use case, the problem was the same for the error message. I needed to put it at the end of the 3 fields, even if the error is concerning the first one.

I explored the fieldset imbrication mechanism. But it doesn't suit the needs because it can only wrap several fields with a

or a
tag. But it seemed it needed extending the form class to add new methods...

I think everything is too much tightly coupled and makes the whole thing over-complicated. I feel this is a more general problematic also involving valdiation (which needs "label" to create a set of validation rules?).

Strictly about rendering, I'm happy right now by using \View::forge('my/view', array('fieldset' => $fieldset')) manually.

jschreuder commented 12 years ago

First off, I am planning on a full rewrite of this for v2 which will feature at least the following:

But now on to the problems: the proposal here is to replace the build method with View parsing. This is already very possible and there only thing that would add is further coupling. The thing you would do now is to pass the fieldset instance and it has all the information you'd need to generate the form anyway. Thus I don't really see the problem here?

Now on to some individual remarks:

@WanWizard

This is so complex that no user is ever going to use it. Only workable solution is to remove all templating, use "field_template" only to list the tags you want generated, code the rest of the HTML by hand, and use $form->field('name')->build() to add the form field.

This is already possible, just make the template return just '{field}' at the Form instance or app level and you're done.

And still not 100% as you can't fetch the label independently from the input tag, which can only be worked around by calling build() twice, once with a template that only outputs the label, and once with a template that only outputs the field.

How far are you putting those apart? Even in a table I wouldn't have trouble putting these in the same template. Also we could seperate out the building of the label and the field into separate methods thus making it easier to use them without templating.

@Savageman

I think everything is too much tightly coupled and makes the whole thing over-complicated. I feel this is a more general problematic also involving valdiation (which needs "label" to create a set of validation rules?).

I agree that it should be decoupled a bit. But Validation needs a label to show in the error message, though you can already omit it and it'll default to the field name: $val->add('fieldname');

Strictly about rendering, I'm happy right now by using \View::forge('my/view', array('fieldset' => $fieldset')) manually.

Which is the point: the fieldset class is meant to model the set of fields you have. As I have stated many times the build methods are meant for Q&D and may be enough for some, but any complex form will need individual handling of many of its fields that is just impossible genericly Thus what you are doing is probably how I always intended it to be used.

WanWizard commented 12 years ago

How far are you putting those apart? Even in a table I wouldn't have trouble putting these in the same template.

What needs to be generated in this use-case is

<dt>
    <label for="form_date">Date</label>
</dt>
<dd>
    <input type="hidden" id="date" name="date" value="" />
    <input type="text" required="required" id="dateDD" name="dateDD" value="" />
    <input type="text" required="required" id="dateMM" name="dateMM" value="" />
    <input type="text" required="required" id="dateYY" name="dateYY" value="" />
</dd>

Date is a model property, the other three are fields manually added using add_after().

The main reason for wanting to use Fieldset here is the coupling with the ORM model, and the validation and population features. Which you lose when you start hand-coding the form.

p.s. I wasn't indicating I wanted integration with the View class.

What I suggested was to introduce a new template that would cover the entire form instead of only a single field, and that would use used to generate. The template would then look like this:

{open}
<table id="myform">
    <tr>
        <td>{label:fieldname}</td><td>{tag:fieldname} {help:fieldname}</td>
    </tr>
</table>
{close}

This can be defined any which way you like, but the obvious way is to build this in a view, and use that to set the template. build() can cast it to string, iterate over the defined fields, and per field do a search/replace as it does now on the field level...

jschreuder commented 12 years ago

Not relevant to keep open at this point as the whole thing will be rewritten for 2.0 and no big changes will be made in the 1.x branch anymore.