PHP Form Builder Class

I would like my form generator class to be reviewed. I want to know about the following topics:

• Is this efficient?

• Does this have any flaws?

• Is this using the concepts in the proper way?

• Are there any security issues?

• Is my coding style correct?

" : " name=\"$name\">\n";
        for($i = 0; $i buildField($fields[$i]['field'], 
                                       $fields[$i]['type'], 
                                       $fields[$i]['name'], 
                                       empty($fields[$i]['placeholder']) ? '' : $fields[$i]['placeholder'],
                                       empty($fields[$i]['style']) ? '' : $fields[$i]['style'],
                                       empty($fields[$i]['options']) ? '' : $fields[$i]['options'],
                                       empty($fields[$i]['value']) ? '' : $friends[$i]['value']
            );
        }
        $form .= "";
        return $form;
    }
    public function buildField($field, $type, $name, $placeholder = null, $style = null, $options = null, $value = null)
    {
        switch($field)
        {
            case 'input':
                $fieldHTML  = " \n";
                break;
            case 'textbox':
                $fieldHTML  = " \n";
                break;
            case 'submit':
                $fieldHTML  = " \n";
                break;
            default:
                $fieldHTML = "Failed to Build Field. Please make sure that you have set the field properties correctly.";
                break;
        }
        return $fieldHTML;
    }
}

Example:

```
$formBuilder = new FormBuilder();

echo $formBuilder->buildForm('post',
'',
'some_name',
array(
array(
'field' => 'input',
'type' => 'text',
'name' => 'name',
'placeholder'

Right, a short and simple answer to each of your questions, for now. I'll be expanding on each of the issues through updates:

Is it efficient

Define efficient. If by that, you mean: is it fast, then yes, it probably is. Not as fast as non OO code, but faster than most form builders out there.

If by efficient, however you mean: portable, easy to use (as in clear API), robust and flexible then no. It's not. You can't handle nested forms, validators and to use this code, you need to know what type of array this class expects. Programming by array is not the way to go.

Are there flaws?

Yes, there are, apart from those mentioned above, some other omissions/flaws:

• buildField is public, but contrary to what I'd assume, it is incapable of adding fields to the form. The buildForm is a one-shot deal: it returns an HTML string, that is, to all intents and purposes, static.

• Stringing together markup is tedious, horrible to debug, and equally pointless to test. If things do go worng, it'll probably result in malformed markup, and the source of the problem will be hard to track.

• Forms and ajax are used very often. Ajax implies JS, which in turn implies using meaningful selectors: I can't set ID's here, though. That's an issue!

More to come, because there are a lot of things that need addressing, outside of these primary issues.

Good use of concepts?

IMHO, no. A class should have only 1 job, and yours does. It has only 1 job: to generate a string. But HTML is not just any string: there's grammar and rules involved. It's part of a DOM. PHP ships with the DOMDocument class (and all of its derivatives. That class ensures the markup is valid. You can also add elements to an existing dom, assign and remove attributes, ... the works. Use helper classes. Generating markup is done using objects, too.

Are there any security issues?

Yes, quite a few. If I pass '">location.href=\'spam-site-url\';' as the value for the form name, then the resulting markup will be:

location.url='spam-site-url';

XSS attacks, through any of the fields is easily done

Coding style

Looks alright, apart from the closing ?> tag. According to the official docs, it is optional and even discouraged in PHP-only scripts. But when it comes to coding style/conventions, why not see for yourself if you adhere to them: the coding standards are open to anyone, read through them, and apply them to your code.

Suggestion

If you don't want to use one of the many forbuilders that have been written before, then that's your right. But that doesn't mean you should close your eyes to what they have to offer, and how they do what they do. Check out the Symfony2 form component, or the Formbuilder of Zend, or any of the other frameworks. See how they do things, "borrow" the ideas you like, add your own, and if you find the time: try giving back to the respective communities by contributing...

Update

Is it efficient

Seeing as you're writing object, I'm going to interpret efficiency in an OO context: efficient code abstracts the nitty gritty, while allowing the user to customize almost every aspect through a clean, and concistent API, all the time being reasonable certain that the end-result will be valid, usable markup.

To acchieve all this, you can't just string together markup like you are doing now. OO is about hierarchy, dependency and inheritance.
Think of a form, what defines it behaviour (ie is inherent to the form), and what does it contain (most likely optional).

A form is an element, that uses attributes to define its behaviour. A form element consists of zero or more child elements (inputs, selects, buttons, textarea's, but also fieldsets, labels, etc...). All of these child elements belong to the form, but have their own behaviours (attributes). Some elements, like select, or fieldset can, and often will consist of more than one child element.

Adding these children, or setting their attributes is something that one might do as you go along (ie fetching select options from the db, adding options after constructing the base form). This means that addField has to be matched with a getField method, to get a reference to an existing form element, so the user can alter it.

It is also likely to happen that, for whatever reason, the user wants to pre-set some values, just as it is likely for the user to add form validation, and a method to validate the form, sanitize the submitted data and extract it, either as an array or an object.

All the while, the form should be mutable: it has to be possible to add and remove elements on the fly. That's a lot of work, so I'm not going to write a fully fledged example of how to implement this. Instead, here's how I'd start:

• Write 2 distinct classes: FormElement and FormBuilder. The latter takes instances of FormElement, and adds them to its internal FormElement that represents the actual ` tag.






• the FormElement class needn't be used by the user, provided the Fo

StackExchange Code Review Q#59702, answer score: 13