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 '"><script>location.href=\'spam-site-url\';</script>' as the value for the form name, then the resulting markup will be:
<form method="post" name=""><script>location.url='spam-site-url';</script>
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 <form> tag.
- the
FormElement class needn't be used by the user, provided the FormBuilder has some methods like addElement, getElement, removeElement, and so on
- the
FormBuilder could then be altered, to use FormValidator elements, attached to each of its elements. Once you do this, the form should also implement getFormData and setAll methods, to process and pre-set the form elements.
- By this time, you ought to realize that you no longer have a formBuilder class. Instead, you are building a form module. Which opens up the possibility of adding model mapping, form events, plugins, helpers and all that...
Now, notice that I said that a <form> tag is a FormElement, but that it's contained within the FormBuilder. That is because, unlike otehr FormElements, the form needs another abstraction layer. It's a DOM element that contains all of the input fields within the form. It stands to reason, then, that this will be the FormElement that the user will want to use for form validation, and extracting data. This, too, requires a consistent and tidy API, if you want a form module worth using. I therefore suggest to create a third class: class Form extends FormElement, where all of the bulk form interactions live.
Adding FormElements should be as simple as this:
$element = $builder->addElement(
FormBuilder::ELEMENT_TEXT,//type
array(
'id' => 'txtId',
'name' => 'txtName',
'value' => ''
)
);//creates DOMNode, adds it to form, and returns a reference to the newly created node
$element->getAttribute('value')->value ='new value value';//or nodeValue
As you can see, I can alter nodes I've added to the form, but it looks kind of faffy and messy. That's why I suggest you wrap the DOMNodes that make up your form in a generic FormElement class, and give it methods like setValue:
public function setValue($val)
{
//$this->node === the raw DOMNode instance
$attr = $this->node->getAttribute('value');
if ($attr === null)//no value attribute yet, create it
$attr = new DOMAttr('value', $val);
else//set new value
$attr->value = $val;
$this->node->setAttributeNode($attr);
return $this->node;
}
This is just a crude example, but you get the idea
Update 2:
Are there flaws
Well, I've stated there are quite a few of them already, so I'll just suggest an approach that, IMO, will help you to build a more robust and reliable module. Beginning with something I hinted at in my previous update:
I suggested adding a form element could be done using something along the lines of:
$builder->addField(FormBuilder::ELEMENT_TEXT, $attributesArray);
Let's look into the arguments, first, then how to sanitize and validate them, and finally, how to process them.
the arguments
The first argument is a class constant. For each form element you want to support (select, textarea, radio's, checkboxes, ...) you'll have to create a constant. This makes the user code a lot easier to read/understand. Anyone can see that the snipped above adds a text input to a form, with an array of attributes. Change the first argument to FormBuilder::ELEMENT_TEXTAREA obviously adds a textarea element and so on.
The second array is an (optional) array of attributes. Your method's signature,then, should use type-hinting to ensure that an array is passed. You could create an Attributes class, but in this case, I feel an array will do. The only thing the user must ensure is that the array is associative, because the keys are the attribute names, and the values will be the attribute's value...
Sanitation/validation
How to validate the first argument is up to you, either define each constant to be an int, that is a power of 2: 1, 2, 4, 8, 16, 32, 64, 128, and so on, and use bitwise operators to check if the argument is an int and a power of two, or use a private array where and fill it with all of the constants, and use isset to check if the value passed is a valid constant. I'd opt for the second option, simply because you can then assign a string to each content that you can use to create the node, and use this array to define a couple of basic, but essential attributes for each tag (ie: hidden and text elements will both be input tags, but have a different type-attribute).
The second argument is ensured to be an array (thanks to type-hint). Next step, although this might not be an issue at first, is to map the array, and invoke strip_tags on each value, since attributes shouldn't contain markup.
Also validate the keys of the array, matching them against an array of valid attributes. By this I mean: style and onclick are, in HTML-land, valid attributes, but these kind of attributes should be discouraged. Any decent front-end developer will be more than able to bind event handlers to the form elements without the need to set an onclick attribtue. He'll also be more than capable to churn out CSS that presents the form in any way you like. That's the job of a front-end developer.
Think of it like this: you're creating a FormBuilder, not a FormDesigner, just like you'd expect a builder to build your house, but you have to ask a decorator or interior architect to design what it looks like.
Processing
Well, since I've already stated you should wrap the form element in a Form instance, the same applies to the form elements: they should be instances of the DOMElement class, wrapped in a FormElement instance. Pass those instances on to the form, and let the form instance handle the rest (adding, wrapping in containers etc...).
If anywhere along the line, an invalid argument is passed, or a bad attribute (like style) is being set: don't ignore it, don't try to make sense of things: the code that called your class might have a bug in it, and it should be fixed: Throw an exception to notify the user something is seriously wrong.
To round up, here's a short example of what the FormBuilder class could look like:
class FormBuilder
{
const ELEMENT_HIDDEN = 'input';
const ELEMENT_TEXT = 'input';
const ELEMENT_TEXTAREA = 'textarea';
/**
* @var array
*/
private $constants = array(
self::ELEMENT_HIDDEN => array('type' => 'hidden'),
self::ELEMENT_TEXT => array('type' => 'text'),
self::ELEMENT_TEXTAREA => array()
);
/**
* see setAttributes method for usage example
* @var array
*/
private $bannedAttributes = array(
'type' => 'type is set by FormBuilder',
'style' => 'Front-end is not the job of a builder',
'onclick' => 'Learn JS'//add other on* attributes
);
/**
* @var Form
*/
private $form = null;//the actual form we're building
/**
* Optionally make this final, this constructor creates the form
* @param array $options = null
*/
public function __construct(array $options = null)
{//allow for user to pass specifics, like form attributes
$formNode = new DOMElement('form');
if ($options && isset($options['attributes']))
{
$formNode = $this->setAttributes(
$formNode,
$options['attributes']
);
}
$this->form = new Form($formNode);//create Form wrapper...
}
/**
* Add field to form, after setting optional attributes
* @param string $type
* @param array $attribtues = array()
* @return FormElement
* @throws InvalidArgumentException
*/
public function addField($type, array $attribtues = array())
{
if (!isset($this->constants[$type]))
{//optionally allow for direct DOMElement injection here, but I wouldn't
throw new InvalidArgumentException(
sprintf(
'%s is not a valid type (use %s::ELEMENT_* constants)',
$type,
__CLASS__
)
);
}
foreach ($this->constants[$type] as $name => $val)
$attributes[$name] = $val;//re-assign default attributes, like type
$node = $this->setAttributes(
new DOMElement($type),
$attributes
);
$node = new FormElement($node);//set in wrapper
$this->form->addElement($node);
return $node;
}
/**
* Set attributes for node
* @param DOMElement $node
* @param array $attributes
* @return DOMElement
* @throws InvalidArgumentException
*/
protected function setAttributes(DOMElement $node, array $attributes)
{
foreach ($attributes as $name => $val)
{
if (isset($this->bannedAttributes[$name]))
{//symfony2 style exception throwing...
throw new InvalidArgumentException(
sprintf(
'%s attribute not allowed: %s',
$name,
$this->bannedAttributes[$name]
)
);
}
//strip_tags to make sure
$attr = new DOMAttr($name, strip_tags($val));
$node->setAttributeNode($attr);
}
return $node;
}
}
This really ought to give you some idea as to how to set about building a form module. But this also shows how complex things are about to become. One special attribute you will want to add is the label attribute. This shouldn't be set on the element itself, but rather create a secondary DOMElement with a for attribute. But no matter, we have bigger fish to fry: At the moment, we can't, for example, add fieldsets yet, nor can we remove elements, or determine in which order the elements will be rendered. Look into how other frameworks handle this (often, they wrap elements in divs, and have the Form instance track those, which makes the removeChild calls a lot easier).
I'm not going to do that for you (I'm reviewing code, not writing it), so I say again: look at what is already out there, and let that code inspire you!
Coding style
The code above follows the PHP-FIG standards quite closely (with the exception of Allman-style indentation). I've adopted the Symfony2 way of throwing exceptions, too, because I find it helpful in forcing me to throw exceptions with meaningful messages, and the sprintf reminds me to put some variables in there, so I can see what value was being passed when the exception was thrown.
It's a matter of personal preference, perhaps, but the rest of it (all caps for constants, doc-blocks, camelCased method names and type-hinding) is all conform the standard.
