fuel / core

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

Form->field() returns all Fieldset fields, not just form fields #998

Closed consilience closed 12 years ago

consilience commented 12 years ago

The Form class is a convenient wrapper around a Fieldset, used for creating forms. A Fieldset contains Fieldset_Fields, some of which are to end up in a form, and some which are not. If the "type" of a Fieldset_Field is set to false in the ORM 'form' element, for example, then that field will not appear in a form.

So Form->field() is an alias for the underlying Fieldset->field(), which returns all fields for the fieldset. I would argue that it should only return those fields that have a non-false form type, i.e. will appear in the build form. It is misleading otherwise.

A use-case for Form->field() will be to get a list of fields in a form so that they can be rendered, using build(), individually for custom layouts. If fields that are not to be displayed in the form are returned as form fields, then all sorts of crazy stuff can happen.

The fix would be in core/classes/form/instance.php::field()

Instead of blindly returning return $this->fieldset->field($name, $flatten), it should sift through them and return just those that are form elements.

consilience commented 12 years ago

I think this can be taken further:

Validating the return data from a submitted form is done against the Fieldset, and not a Form object. This means that if there are any Fieldset items set to mandatory, that happen not to be fields that are part of the form, the validation still fails. The mandatory property of a non-form item relates more to the database (i.e. the Fieldset cannot be saved if the mandatory fields are not set). This has nothing to do with the form fields presented to the user, that need to be validated at that level.

So when a form is submitted by the user, I feel that I should be able to generate the Form object in the controller, then run $Form->validate() to validate just the Fieldset Fields that are in the form, and ignore the validation of all non-form Fieldset Fields, as errors in that validation is not for the end user to deal with. The remaining [mandatory] fields on Fieldset can then be filled in by the controller before passing back to the orm object and saving in the database. Bear in mind that a Fieldset is likely to be generated by the orm object in the first place, so there could be any number of mandatory fields in that Fieldset that are never intended to appear on a form.

WanWizard commented 12 years ago

Don't expect any major work on Validation and Fieldsets for 1.x, as they are already completely rewritten for 2.0. One of the reasons for that is the current tangled mess that's Form, Fieldset and Validation.

And I don't entirely agree with you here. The fact that the generic build() method skips these fields doesn't mean you don't want to manually generate forms that do contain these fields. I hardly ever use the standard build() method as it's templating features are to limiting in most cases.

consilience commented 12 years ago

It is the consistency between Form->field() and Form->build() that I was referring to - one returns just fields that are rendered, and the other returns fields that are not rendered into the form too, giving you form fields that are not in the form. The fieldset is different - it includes all fields whether rendered or not. It was just a bit inconsistent.

But as you say, it is all being rewritten, so will be history soon - projects either deal with the forms as they are now, or will be starting with FuelPHP 2.x

So given this is a dead-end, it may be just as well to close this issue.

WanWizard commented 12 years ago

As you wish... :)