OOP PHP form builder with validation

Question

Yes, another PHP form builder... I wrote this solely to demonstrate OOP principles.

My questions are:

• Could this be written better? OOP-wise.
• How could I better implement error handling?

Index.php

<?php
require __DIR__ . '/vendor/autoload.php';
use FormBuilder\FormGenerator as Form;
use FormBuilder\TextField as TextField;
use FormBuilder\InputErrors as Errors;
$action = '/index.php';
$method = 'POST';
$form = new Form($action, $method);
$form->addField((new TextField('name'))->addRule(new Rules\MinLenght(3, 'Name too short'))
 ->addRule(new Rules\MaxLenght(13, 'Name too long'))
 ->setAttribute('required'));
$form->addField(new TextField('phone_number'));
$form->addField(new TextField('email_address'));
if (!empty($_POST))
 $form->validateForm();
$form->display();

Core/FormGenerator.php

<?php
namespace FormBuilder;
class FormGenerator {
 private $action;
 private $method;
 private $fields = [];
 public function __construct(string $action, string $method) 
 { 
 $this->action = $action;
 $this->method = $method;
 }
 public function addField(object $field)
 {
 array_push($this->fields, $field);
 }
 public function display()
 {
 
 echo '<form method="', $this->method, '" action="', $this->action, '">';
 foreach ($this->fields as $field)
 $field->generateElement();
 
 echo '<input type="submit"></input>';
 echo '</form>';
 }
 public function validateForm()
 {
 foreach ($this->fields as $field)
 {
 $field->validateField();
 }
 }
}

Core/Input.php

<?php
namespace FormBuilder;
Abstract class Input {
 protected $attributes = [];
 protected $rules = [];
 protected $label = null;
 protected $content = null;
 public function __construct($nameAttribute)
 {
 $this->attributes['name'] = $nameAttribute;
 }
 public function setAttribute($name, $value = null)
 {
 if ($value) 
 $this->attributes[$name] = $value;
 else
 $this->attributes[$name] = null;
 
 return $this;
 }
 public function addRule($rules)
 {
 array_push($this->rules, $rules);
 return $this;
 }
 public function setLabel($label)
 {
 $this->label = $label;
 return $this;
 }
 public function validateField() 
 {
 foreach ($this->rules as $rule)
 {
 $rule->validate($this->attributes['name']);
 }
 }
 protected function attributesToString($attributes) 
 {
 $string = '';
 
 foreach($attributes as $name => $value) 
 {
 if ($value)
 $string .= ' ' . $name . '="' . $value . '"';
 else
 $string .= ' ' . $name . ' ';
 }
 return $string;
 }
 abstract public function generateElement();
}

Core/TextField.php

<?php 
namespace FormBuilder;
use FormBuilder\InputErrors as Errors;
class TextField extends Input
{
 public function generateElement() 
 {
 echo '<input type="text"' . $this->attributesToString($this->attributes) . '>';
 if (Errors::get($this->attributes['name']))
 {
 echo '<p>' . Errors::get($this->attributes['name']) . '</p>';
 }
 if ($this->label) 
 {
 echo '<label for="' . $this->attributes['name'] . '">' . $this->label . '</label>';
 }
 }
}

Core/InputErrors.php

<?php
namespace FormBuilder;
class InputErrors {
 private static $errors = [];
 public static function set($inputName, $error)
 {
 self::$errors[$inputName] = $error;
 }
 public static function get($inputName)
 {
 return (isset(self::$errors[$inputName])) ? self::$errors[$inputName] : False;
 }
 public static function dump()
 {
 return self::$errors;
 }
}

Core/Rules/MaxLenght.php

<?php
namespace Rules;
use FormBuilder\InputErrors as InputErrors;
class MaxLenght {
 private $maxLenght;
 private $errorMessage;
 public function __construct($maxLenght, $errorMessage)
 {
 $this->maxLenght = $maxLenght;
 $this->errorMessage = $errorMessage;
 }
 public function validate($inputName)
 {
 if (strlen($_POST[$inputName]) > $this->maxLenght) 
 {
 InputErrors::set($inputName, $this->errorMessage);
 }
 }
}

Core/Rules/MinLenght.php

<?php
namespace Rules;
use FormBuilder\InputErrors as InputErrors;
class MinLenght {
 private $minLenght;
 private $errorMessage;
 public function __construct($minLenght, $errorMessage)
 {
 $this->minLenght = $minLenght;
 $this->errorMessage = $errorMessage;
 }
 public function validate($inputName)
 {
 if (strlen($_POST[$inputName]) < $this->minLenght) 
 {
 InputErrors::set($inputName, $this->errorMessage);
 }
 }
}

Answer 1

This looks like proper OOP code. It doesn't just encapsulate a lot of functions in a single class. It's expandable with more input types and errors, but I am afraid that you might have overdone it a bit. So many files, for so little functionality.

The elephant

Let's first discuss the elephant in the room: Form validation is done by the same code that builds the form. I think this is a bad design choice, because it restricts you. The page, which holds the form, has to be submitted to evaluate the form. That is not a good user experience. I also don't see how your form fields keep their values when feedback is given? That is a serious usability problem.

I would completely separate form building from form validation. They are quite different things, and separation of concerns is an important design principle.

Separating them would allow you to use an AJAX call to a PHP validation script without even actually submitting the form. This means the input fields can keep their values, without writing any extra code for that.

Coding details

1: You specify the parameter type in the FormGenerator class constructor, but nowhere else. Why?

2: Some of your methods return HTML, others echo HTML. It is almost standard practice to never echo HTML inside a class. Just return it and then:

echo $form->display();

3: Your HTML would become invalid if, in attributesToString(), one of the attributes would contain a double quote. This is more likely to occur than you might think.

4: I noticed this code:

return (isset(self::$errors[$inputName])) ? self::$errors[$inputName] : False;

which could be written as:

return self::$errors[$inputName] ?? false;

using the Null Coalescing Operator.

Identifiers

Naming things is important. I have two minor comments about that:

• You use different terms for outputting HTML: display() for the form and generateElement(); for a field. I would prefer something like getFormHtml() and getFieldHtml().
• Although method names like get() and set() are short, they don't, by themselves, tell you what they get and set. Most of the time you say what it is, but not when it comes to the errors.

• Sᴀᴍ Onᴇᴌᴀ: "Some of your methods return HTML, others echo HTML" which ones return HTML? It appears Input::attributesToString() returns a string which is used by the subclasses and other methods in that parent class return $this to support chaining.
• KIKO Software: @SᴀᴍOnᴇᴌᴀ You're right. I thought one method returned HTML, but it doesn't. Anyway, the general statement that it "is almost standard practice to never echo HTML inside a class" holds true. The reason for this is that it allows you to either process the HTML further, or to echo it, whereas, when you echo it inside the method, you don't have that choice.
• PRobert: First of all, thank you for your review, much appreciated! I do understand that is bad design choice, and separating validation from the form builder will make it more usable. 1. I was so eager to post and get some code review that i fogot to recheck. 2. Noted. 3. How could i mitigate that? Maybe implementing something with str_replace() ? 4. Noted.
• KIKO Software: @PRobert Yes, you could do str_replace('"', '"', $value), or, more generally, use htmlspecialchars(). Be aware that in some very rare cases, changing the value this way, can have unwanted consequences. Overall it's much better to safeguard the HTML than to worry about the value.

Answer 2

OOP

I agree with the answer by Kiko. You didn't ask about MVC code but if that is a goal then it would be wise to only have HTML/text output by the view components - e.g. either in FormGenerator.php or else just call methods that return text from index.php and echo it there.

It does seem slightly strange to have the class FormGenerator yet it is utilized as Form:

use FormBuilder\FormGenerator as Form; 

It could simply be named Form for the sake of simplicity.

Other review points

Typos

One of the first things I noticed upon reading the code was names like MaxLenght and MinLenght- those should be MaxLength and MinLength. I was thinking that might lead to an error but apparently it was used consistently so there was no error.

New lines for all blocks

Adhering to a standard is not required but it does help anyone reading the code. Many PHP developers follow PSRs like PSR-12. Idiomatic PHP code has class and method blocks start on a new line (per PSR-12 section 4.4) but not for other control structures like loops and conditionals. See PSR-12 section 5.

5. Control Structures

The general style rules for control structures are as follows:

• There MUST be one space after the control structure keyword
• There MUST NOT be a space after the opening parenthesis
• There MUST NOT be a space before the closing parenthesis
• There MUST be one space between the closing parenthesis and the opening brace
• The structure body MUST be indented once

Indentation

In class FormGenerator there is this code:

public function display()
{
 echo '<form method="', $this->method, '" action="', $this->action, '">';
 foreach ($this->fields as $field)
 $field->generateElement();

The foreach has an extra level of indentation. It makes sense that it is within the <form> but from the perspective of PHP it is not within an extra block.

Shorthand ternary operator

Kiko mentioned in point #4 that the null coalescing operator could be used to simplify the return statement in InputErrors::get(). Similarly the short-hand ternary operator could be used to simplify code. For example - in Input::setAttribute():

public function setAttribute($name, $value = null)
{
 if ($value) 
 $this->attributes[$name] = $value;
 else
 $this->attributes[$name] = null;
 return $this;
}

could be simplified to:

public function setAttribute($name, $value = null)
{
 $this->attributes[$name] = $value ?: null; 
 return $this;
}

Pushing into an array

stacking image

In FormGenerator::addField() the field is pushed using array_push():

array_push($this->fields, $field);

When adding a single element to the array, the same can be achieved by assigning the next available key (which can be omitted):

$this->fields[] = $field;

The same is true for the code in Input::addRule().

Empty HTML tags: <input>

No Permitted content is allowed for <input> elements ¹ so "using a closing tag on an empty element is usually invalid. For example, <input type="text"></input> is invalid HTML." ² .

So instead of:

<input type="submit"></input>

They can be self-closing:

<input type="submit" />

for attribute of <label>

The TextField class uses the <label> element with a for attribute but the value used is the name attribute

echo '<label for="' . $this->attributes['name'] . '">' . $this->label . '</label>';

However it should match the id attribute of the element. Refer to the MDN documentation for <label>:

for

The value of the for attribute must be a single id for a labelable form-related element in the same document as the <label> element. So, any given label element can be associated with only one form control. ³