PHP Form generator class

Question

I have made the following classes and objects. It was made to create forms programattically easily. It should produce highly readable, valid, and clean HTML.

Care to give me some feedback? :)

https://gist.github.com/2510553

Full code:

<?php
namespace Forms;
/**
 * This file is supposed to give a good way of generating forms programmatically with PHP.
 */

/*
 * WARNING! WARNING! WARNING! WARNING! WARNING! WARNING! WARNING! WARNING! WARNING! WARNING! WARNING! WARNING!
 * -----------------------------------------------------------------------------------------------------------
 * None of the strings you see in the following classes are escaped/secured against any kind of
 * SQL Injections, XSS attacks, or any other sort of attack for that matter! For your own safety
 * Implement the necessary protections on any strings you use in these classes before entering them!
 * -----------------------------------------------------------------------------------------------------------
 * WARNING! WARNING! WARNING! WARNING! WARNING! WARNING! WARNING! WARNING! WARNING! WARNING! WARNING! WARNING!
 * @package Classes
 */

/**
 * All containing nodes should implement this.
 */

interface Contains_Nodes {

    /**
     * @abstract
     *
     * @param Node $node
     *
     * This method will add a node to the node list of the implementing object.
     */

    public function add(Node $node);
}

Node

/**
 * Base object for all Nodes.
 */
class Node {

    /**
     * @var string $element Will hold the element name (or tag name)
     */
    public $element;
    /**
     * @var array $attribute_list Will hold all of the rest of the form's attributes such as ID, class and whatnot.
     */
    public $attribute_list = array();

    /**
     * @var bool $self_contained Signifies whether the Node is self contained. Self contained nodes do not get a closing tag.
     */

    public $text;

    public $self_contained = false;

    const SELF_CONTAINED = true;
    const NOT_SELF_CONTAINED = false;

    const TAB = "    ";

    /**
     * @param string $element
     * @param string $text
     * @param bool   $self_contained
     * @param array  $attribute_list
     *
     * General constructor for nodes. Should be overridden regularly.
     */
    public function __construct($element, $text = "", $self_contained = false, array $attribute_list = array()) {
        $this->element        = $element;
        $this->self_contained = $self_contained;
        $this->text = $text;
        $this->attribute_list = $attribute_list;
    }

    /**
     * @return string
     *
     * General string generator for nodes. Should be overridden regularly.
     */
    public function __toString() {
        //Open element
        $result = "<{$this->element}";

        //Node list
        $result .= $this->string_attribute_list();

        //Close element
        $result .= ">";
        if (!$this->self_contained && !empty($this->text)) {
            $result .= $this->text;
        }
        if (!$this->self_contained) {
            $result .= "</{$this->element}>";
        }

        return $result;
    }

    /**
     * @return string
     */
    public function string_attribute_list() {
        $result = "";
        if (!empty($this->attribute_list)) {
            foreach ($this->attribute_list as $attr => $value) {
                $result .= " {$attr}=\"{$value}\"";
            }
        }
        return $result;
    }

}

Form

/**
 * The Form object, will describe a single form.
 * After constructing it is done, it should format into a cool, simple, valid, readable, HTML code.
 */
class Form extends Node implements Contains_Nodes {

    public $element = "form";
    public $self_contained = false;

    /**
     * @var Node[] $node_list This will hold all of the nodes in the form. Including fields and inputs.
     */
    public $node_list;
    /**
     * @var string $action Will hold the action for the form. This is a required field.
     */
    public $action;
    /**
     * @var string $method Will hold the form submission method for the form. Defaults to POST.
     */
    public $method = Form::METHOD_POST;

    const METHOD_POST = 'POST';
    const METHOD_GET  = 'GET';

    /**
     * @param string $action         Page to which the form submits.
     * @param string $method         POST or GET, will throw an exception otherwise.
     * @param array  $attribute_list Miscellaneous attributes for the form element.
     */
    public function __construct($action, $method = self::METHOD_POST, array $attribute_list = array()) {
        $this->action         = $action;
        $this->method         = $method;
        $this->attribute_list = $attribute_list;

        if (($method != self::METHOD_GET) && ($method != self::METHOD_POST)) {
            throw new \Exception("Form method must be either POST or GET");
        }

    }

    /**
     * @param Node $node Node to add.
     *
     * @return Form to not break the chain
     */
    public function add(Node $node) {
        $this->node_list[] = $node;
        return $this;
    }

    /**
     * @return string Format and stringify the form.
     */
    public function __toString() {
        //Open tag, usually <form ...>
        $result = "<{$this->element} action=\"{$this->action}\" method=\"{$this->method}\"";

        //Attribute list
        $result .= $this->string_attribute_list();
        //Close opening tag
        $result .= ">";

        //Loop through the nodes
        foreach ($this->node_list as $node) {
            $result .= "\n" . self::TAB . str_replace("\n", "\n" . self::TAB, $node->__toString());
            //The replace is to keep the code indented and formatted properly.
        }

        //Close form element
        $result .= "\n</{$this->element}>";

        return $result;

    }
}

Input

/**
 * Class to describe a single input node.
 */
class Input extends Node {

    public $element = "input";
    public $self_contained = true;
    public $type;
    public $label;
    public $name;
    public $default_value;

    public $label_before_input = true;

    /**
     * @param string $type
     * @param string $name
     * @param Label  $label
     * @param string $default_value
     * @param array  $attribute_list
     *
     * Constructor for input node.
     */
    public function __construct($type, $name, Label $label, $default_value = "", array $attribute_list = array()) {
        $this->type           = $type;
        $this->name           = $name;
        $this->label          = $label;
        $this->default_value  = $default_value;
        $this->attribute_list = $attribute_list;
    }

    /**
     * @return string Formatted input HTML.
     */
    public function __toString() {
        //Label element open (usually "<label"
        $result = "<{$this->label->element}";

        //Begin attribute list
        $result .= $this->label->string_attribute_list();

        //Close opening tag
        $result .= ">";

        //If we want label before the input...
        if ($this->label_before_input) {
            $result .= "\n" . self::TAB . $this->label->text;
        }

        //Begin input element usually "<input ..."
        $result .= "\n" . self::TAB . "<{$this->element} type=\"{$this->type}\" name=\"{$this->name}\"";

        //Default value
        if (!empty($this->default_value)) {
            $result .= " value=\"{$this->default_value}\"";
        }

        //Attribute list
        $result .= $this->string_attribute_list();
        //Close input element
        $result .= ">";

        //If we want label after input
        if (!$this->label_before_input) {
            $result .= "\n" . self::TAB . $this->label->text;
        }

        //Close label element (usually "</label>"
        $result .= "\n</{$this->label->element}>";

        /*
         * FINAL RESULT should look like:
         * <label [label-attributes]>
         *     [text-if-before]
         *     <input type=[type] name=[name] value=[value] [input-attributes]
         *     [text-if-after]
         * </label>
         */

        return $result;
    }
}

Label

/**
 * Class to describe a single label node.
 * Labels should be contained inside of inputs on a 1:1 relationship.
 */
class Label extends Node {

    public $element = "label";

    /**
     * @param string $text
     * @param array  $attribute_list
     */
    public function __construct($text, array $attribute_list = array()) {
        $this->text           = $text;
        $this->attribute_list = $attribute_list;
    }

    /**
     * @return string Returns label text only. Labels can only be part of inputs.
     */
    public function __toString() {
        return $this->text;
    }
}

Fieldset

/**
 * Class to describe a single fieldset node.
 *
 * @implements Contains_Nodes
 */
class Fieldset extends Node implements Contains_Nodes {

    public $element = "fieldset";
    /**
     * @var Node[]
     */
    public $node_list;
    public $legend;

    /**
     * @param $legend
     *
     * Construct a fieldset element
     */
    public function __construct($legend) {
        $this->legend = $legend;
    }

    /**
     * @param Node $node
     *
     * @return Fieldset to not break the chain
     *
     * Add a node to the fieldset.
     */
    public function add(Node $node) {
        $this->node_list[] = $node;
        return $this;
    }

    /**
     * @return string Generate a formatted node list of the fieldset.
     */
    public function __toString() {
        //Open element (usually <fieldset)
        $result = "<{$this->element}";

        //Attribute list
        $this->string_attribute_list();

        //Close opening tag
        $result .= ">";

        //Legend text
        $result .= "\n" . self::TAB . "<legend>{$this->legend}</legend>";

        //Loop through node list
        foreach ($this->node_list as $node) {
            $result .= "\n" . self::TAB . str_replace("\n", "\n" . self::TAB, $node->__toString());
            //The replace is to keep the code indented and formatted properly.
        }

        //Close fieldset tag
        $result .= "\n</{$this->element}>";

        return $result;

    }
}

Button

class Button extends Node {
    public $element = "button";

    public function __construct($text, array $attribute_list = array()) {
        parent::__construct($this->element,$text, Node::NOT_SELF_CONTAINED, $attribute_list);
    }
}

Submit

class Submit extends Button {
    public $type = "submit";

    public function __construct($text = "Submit", array $attribute_list = array()) {
        $attribute_list["type"] = $this->type;
        parent::__construct($text, $attribute_list);
    }
}

Testing

/*
 * Testing begins!
 */
$form  = new Form("index.php", Form::METHOD_GET);
$field = new Fieldset("Fieldset");
$field->add(new Input("text", "name", new Label("Input inside of Fieldset")));
$form
    ->add(
    new Input(
        "text", //Type
        "test", //Name
        new Label("Testing Input", array("class" => "label")), //Label
        "Woot", //Default value
        array("id" => "test") //Attribute List
    )
)
    ->add(new Node("hr", "", Node::SELF_CONTAINED))
    ->add(new Submit("Go"));

echo $form . "\n\n";

echo "<pre>";
echo print_r($form);
echo "</pre>";

Answer 1

I'm not particularly fond of the style of coding, not everything needs to be a class. But there is only one point that I feel warrants significant attention:

Huge Point: Not escaping means not useful

WARNING! WARNING! WARNING! ...
-------------------------------------------- ...
None of the strings you see in the following classes are escaped/secured against any kind ofSQL Injections, XSS attacks, or any other sort of attack for that matter!

There would be little point using the code as presented if it didn't make life easy, and not escaping any of the properties would make it cumbersome and error-prone to use.

E.g. you've got this in the Testing section

new Label($label ...

What is going to happen if $label is "Categories > Food" or any other innocent string that will cause malformed html as a result? Why is the class not taking care of that automatically? Note that there will be use cases where you want to put html in some tags - such as using an image for a label, or where tags are nested - attributes should always be escaped though.

And that's just talking about how innocent users could break your code, not those with malicious intent who submit "<script>document.location = 'http://mysite';" as their name.

Big Point: Write real tests

If you write a real test (by which I mean using phpunit) you can quickly test normal and edge case scenarios (What if the $x property looks like this?), which will highlight any difficulties in using the code. It'll also permit you the confidence - if you choose to rewrite any of the code in the future - of knowing that it still works in the same way as it did originally.

Mid Point: Inconsistent constructors

You have all these different constructors:

public function __construct($element, $text = "", $self_contained = false, array $attribute_list = array()) {
public function __construct($action, $method = self::METHOD_POST, array $attribute_list = array()) {
public function __construct($type, $name, Label $label, $default_value = "", array $attribute_list = array()) {
public function __construct($text, array $attribute_list = array()) {
public function __construct($legend) {
public function __construct($text, array $attribute_list = array()) {
public function __construct($text = "Submit", array $attribute_list = array()) {

Yet all classes extend Node. Why not just have one:

public function __construct($args = array()) {

A consistent constructor makes it easy to know how to use a class without having to continually refer to the docs or class definition. If that's something you do with none-constructor functions (not applicable here) your code would not be E_STRICT compliant. Having different constructors isn't particularly intuitive, and means you can't have simple override logic like so:

public function __construct($args = array()) {
    .. anything ..
    parent::__construct($args);
}

You can enforce any mandatory args just the same, but given that everything is a class you should be able to set all of the currently-mandatory properties after instanciation anyway.

Mid Point: Needless interface

The interface only defines one method, and where used has exactly the same code. You could just as easily add it to your Node class and override or configure it to be disabled in the cases where it's not applicable.

Mid Point: Needless constants

What's the real benefit of these:

const METHOD_POST = 'POST';
const METHOD_GET  = 'GET';

It's more to type and doesn't add any clarity.

There's also this:

const TAB = "    ";

Which is in fact set to four spaces. "\t" is easier and shorter to type.

Minor Point: Whitespace

Whitespace in html is insignificant, so doing this:

 $result .= "\n" . self::TAB . "<{$this->e...

doesn't do anything for end users.

It also doesn't do what you want, as it leads to ugly html. Look at the source of this page and look for "<form>" - if it were left aligned it would break out of the indentation level where it is. If you used similar code to that in the question to build all your html the indentation would be so wayward you wouldn't be able to read the raw html output without re-indenting it.

It's something that is of no real value, because anyone who wants to see the html structure can just use firebug/devtools/their-tool-of-choice and it'll indent the code for them. If you really want to have indented html anyway, there are tools for that like htmltidy.

Answer 2

If you cannot write form mockup, you leave it to the designer. Or if you are the only one working on the project you use online tools or similar libraries. I'm not saying what you have here is bad, far from it, I'm just saying solutions for this already exist and more well known solutions will be used before one by an unknown author. Just a simple fact of life, people are trendy and suspicious. If you want to use it for your own purposes, that is fine, just don't expect to create the new goto for form creation :) <- (the smile is there to let you know I don't mean this harshly).

I will review your code, though there is little for me to go over, it is already quite clear. Your code is well formed, well documented, and self documenting, so mostly I will be giving observations and suggestions. I will also provide you with input on what I think of your program, as I think that is more what you were looking for. My first input has already been given, so I'll continue with the code review then give my final thoughts towards the end.

There is no need to perform a check on the $self_contained variable twice, it is redundant.

    if ( ! $this->self_contained) {
        if( ! empty($this->text)) { $result .= $this->text; }
        $result .= "</{$this->element}>";
    }

Correct me if I'm wrong, but from what I can tell, your $self_contained check doesn't actually terminate the tag. Simply closing a tag, adding the greater-than bracket ">", does not terminate an element, you must either use the matched closing tag or a self terminator "/>". Most browsers (I believe all) will still process this, but it is still improper and could result in errors in the future. To do this proper add the following else statement to the end of the if statement I gave above.

    else { substr_replace($result, '/>', -1); }

Why are you not checking for empty strings and such? What if I were to create an input with no type? Not saying someone would do so purposefully, but what if they were using a variable to set the type and it somehow got emptied? Adding an error check here could save a lot of debugging time.

You've thrown all these errors but not caught any :(

Well, thats it for the code review, told you there wasn't much. Here's some thoughts I had in the form of suggestions.

I would think about passing associative arrays to your construction methods. This will make it more clear what you are doing. For example.

$form = new Form(array(
    'action' => 'index.php',
    'method' => Form::METHOD_GET,
);
//Compared to:
$form  = new Form("index.php", Form::METHOD_GET);

Yes, because of your self documenting code, it is a little more obvious, but assume someone just passes "get" as the second parameter. Two things will happen, the first is that it will throw an error, the second will be that people reading this won't know what it means. Adding strtoupper to your code will prevent that from throwing any errors and will allow people to continue to be lazy. People like being lazy. I'm people, I like being lazy, therefore my statement stands :) Making it use associative arrays will make it more apparent what you are doing.

Have you thought about adding a depth check? Something to determine the initial tabs to add to each new element? Not all forms will be added to the body tag. Some will be deeper.

No class for textareas?

I do believe your code could use some clarity on implementation. As I said in one of my comments, I'm not sure what is going on in your test. Well that's a lie. I read your program, I know whats going on. But someone who has never used your program before will look at that code and not be sure. They could infer much, but not everything. I for one, have not found where $field is used after it is declared, and I HAVE read your code. Maybe I am missing something? My suggestion about associative arrays will clear up your clarity issue somewhat.

Finally, I would change the TAB constant to \t rather than the escaped equivalent to keep consistent with all those \n's, and then I would make the \n a constant as well, but that is just me. Actually I wouldn't, I would just remove the TAB constant and start using \t. There's nothing wrong with repeatedly using escaped characters. There is no need to use variables or constants to define them, unless of course they are system specific, such as directory separators, but that is not the case here, at least not if all you are using is \n, \r would have to be added as well.

Good luck!

Answer 3

raw html is much cleaner and I suspect much faster than waiting for php to render markup. Let them then input any data they like once validation is passed. Its only when you get to the output stage should you stand up and take notice. So rather than a form or input class consider building a logical(common suituation) output class.