Emagister / zend-form-decorators-bootstrap

Zend_Form decorators for Twitter's Bootstrap UI
http://emagister.github.com/zend-form-decorators-bootstrap
168 stars 79 forks source link

Rearragned form classes to make them more easily extendable #21

Closed mrbubblesort closed 12 years ago

mrbubblesort commented 12 years ago

This change should make it easier for anyone that wants to use Twitter_BootstrapForm* in their existing project, but all of their forms already extend a master form that extends Zend_Form.

My problem: In many of my other projects I have a master form that all other forms extend. This master form extends Zend_Form, and is used for logging and other housekeeping. I wanted to use all of the vertical, horizontal, etc. bootstrap forms in my project, but the problem is figuring out how to extend them. If I have all of my base forms extend the twitter forms, then I lose everything in my master form. I could make my master form extend the Horizontal form, but then I couldn't use Vertical. I could extend Twitter_Bootstrap_Form directly, but then I have to manually set the decorators on each child form, defeating the purpose of this project entirely.

My fix: I removed the contructors from Horizontal, Vertical, and Inline, and moved their decorators to a protected array called $defaultElementDecorators. Then in the Twitter_Bootstrap_Form's constructor I set the decorators based on whatever the extending form's $_disposition is. So you can still extend any of the bootstrap forms as normal, or you can extend Twitter_Bootstrap_Form directly like so:

class My_Master_Form extends Twitter_Bootstrap_Form {
    //set all forms to horizontal by default
    protected $_disposition = Twitter_Bootstrap_Form::DISPOSITION_HORIZONTAL;
    ...
}

class My_Page_Form extends My_Master_Form {
    //set this form to inline
    protected $_disposition = Twitter_Bootstrap_Form::DISPOSITION_INLINE;
    ....
}
mrbubblesort commented 12 years ago

BTW, I also added a Subform class to set the decorators correctly on all subforms. https://github.com/Emagister/zend-form-decorators-bootstrap/issues/15

theUniC commented 12 years ago

Thanks for the PR. Must say that the intend of this library is to have several types of form. Having one class that handles all types of Bootstrap forms through a static class property seems to me an ad-hoc solution for your case. Maybe you could use the State pattern using the Bootstrap forms as subforms, in order to dinamically change the type of your master form (your solution is pretty close to this approach).

mrbubblesort commented 12 years ago

If that's what you want, then so be it I guess. But if the only difference between the classes are decorators, then what's the point of splitting them up? Yes I (and the majority of ZF users) could throw another layer onto my project, but why make it any harder than it has to be? What benefit is there using 4 different classes that are the exact same, save for 1-2 lines? Is it because you're planning to add more functionality in there later? I only used the static class properties because that's what you originally had, but a constructor option or something else would work just as well.