unclecheese / silverstripe-zen-fields

Adds syntactic sugar to SilverStripe FieldLists
GNU General Public License v2.0
29 stars 4 forks source link

$form->Fields()->text(...) doesn't "setForm($form)" on the created field #2

Open olance opened 11 years ago

olance commented 11 years ago

In a form creation method, I add fields to the form depending on some conditions, thus I end up doing something like:

$form->Fields()->text('Name', 'Your name:');

When I do this, the TextField doesn't have its form set to $form, although it seems obvious (to me at least) that this is what I'd like to happen.

So it looks like a "bug" to me, that I have to call $form->Fields()->setForm($form) after adding all my fields, in order to have everything working as expected.

What about calling setForm() automatically?

unclecheese commented 11 years ago

I'm not so sure it's an issue with this module, because if you did $form->Fields()->push(new TextField()); the result would be the same.

And for that matter, I'm not so sure it's an issue at all. You're adding a TextField to a FieldList object, not a Form. The FieldList knows nothing about the form. So even if you could automatically call setForm(), I'm not sure what that would do. FieldList has no knowledge of any Form. The way that form fields become aware of their form is through the Form::__construct() method, so that's why fields added after the instantiation of the form are not aware of their Form.

I'm also just curious what the use case is for getting the Form object from a FormField. I'm not sure I've ever had a need for such a thing... ?

olance commented 11 years ago

Hey, thanks for replying quickly ^^

I have very little knowledge of SS for the moment, so I'm not aware of all its internal working. I did think that a FieldList added to a form had knowledge of its owner, as there is a setForm() on FieldList too. If it's not the case, well of course there's nothing you can do :)

It just seemed odd to me that, basically, adding fields to a form (through its field list) would not cause the fields to be set to be "owned" by that form. I've raised the issue on the SilverStripe framework directly, but because I was using the ->text() convenience method, they redirected me here.

Lastly, this is not because I need to get the form object from a FormField, but because without properly setting the Form on the fields added to it after its construction, the form template would not end up being built correctly.

See this pastebin: http://pastebin.com/M9VT341E When I showed it on the SS IRC channel, Simon Welsh told me the "bad" result happened because I was not calling setForm($form). It's something I was not expecting to be necessary as I had added all my fields to the form already.

unclecheese commented 11 years ago

It is confusing that FieldList has a setForm() method, but if you look at it, it's just a wrapper for FormField::setForm(). It does something like:

foreach($this->fields as $field) $field->setForm($form);

But I think you make a good point. The form should not be applied to the formfields until the point where the form renders. The __construct() method is not a good place for it, because the Form can be mutated any number of ways after instantiation. A better place for it would be the forTemplate() method.

A couple things you can do:

->text("MyText")->field()->setForm($form)->end()
->text("MyText")->field()->setHTMLID("MyID")->end()
unclecheese commented 11 years ago

Just had a look, and it's actually critical that the setForm() happens in the constructor. If you look at FormField::Link(), you'll see why:

    public function Link($action = null) {
        return Controller::join_links($this->form->FormAction(), 'field/' . $this->name, $action);
    }

FormFields, as you may know, are their own request handlers, making it easy to add things like ajax actions to them. They become aware of their link through the form they are attached to. When running a request against a form field, you're not rendering the form, you're just instantiating it, e.g. /my-page/DirectForm, so forTemplate() would be a terrible place to put setForm() as it would break all of that.

I think the best practice is to instantiate the form as the last execution in your form method (e.g. DirectForm()). Build the FieldList first, then return the Form at the end.

$fields = FieldList::create()
  ->text("Foo")
  ->imageUpload("Bar")
;
return BootstrapForm::create($this, "MyForm", $fields, $actions);

Make sense?

olance commented 11 years ago

I didn't know about FormFields being their own request handlers, that could come handy indeed :)

Creating and returning the form as the last instruction of the form method makes perfect sense, thanks for taking the time to explain all that ^^