3
\$\begingroup\$

I am writing this class to render HTML forms. It is working as expected, so far. There are many things to be done yet (including testing). Is it ok to keep this way?

    /**
     *
     * This class renders (or aims to render) a valid HTML form
     *
     * Note: this documentation still needs to be completed. For now I think it does its job.
 *
 * Condensed usage:
 *
 * $form = new Form(["name"=>"myform", "method"=>"post", "action" =>"action.php", "id"=>"html_id"]);
 * $form->add_field(["fieldset"]);
 * $form->add_field(["label", ["value"=>"My Label", "for"=>"txtareaname"]]);
 * $form->add_field(["textarea", ["id"=>"txtareaid", "name"=>"txtareaname", "cols"=>80, "rows"=>10, "value"=>"Texto del textarea"]]);
 * $form->add_field(["fieldset"]);
 * $form->add_field(["select",["name"=>"myselect"], ["value1"=>"text1", "value2"=>"text2"]]);
 * $form->add_field(["text", ["name"=>"mytext", "value"=>"", "required"=>""]]);
 * $form->add_field(["fieldset"]);
 * $form->add_field(["label", ["for"=>"mycheck", "value"=>"My check?"]]);
 * $form->add_field(["checkbox", ["name"=>"mycheck", "value"=>"mycheck", "id"=>"mycheck"]]);
 * $form->add_field(["fieldset"]);
 * $form->add_field(["submit", ["name"=>"enviar", "value"=>"enviar"]]);
 * echo $form->render(); // or $output = $form->render();
 * 
 *
 * When this class is instantiated it takes a key-value array where the keys are any valid <form> attribute and the values
     * are its corresponding attribute value. There are 3 mandatory attributes: name, method and action.
     * Any other attribute, if present, must have a value, so it cannot be an empty string. So for a basic usage:
     *
     *   $form = new Form(["name"=>"myform", "method"=>"post", "action"=>"destination.php", "id"=>"form_id"]);
     *   echo $form->render();
     *
     * Should output something similar to:
     *
     *  <form name="myform" action="destination.php" method="post" id="form_id">
     *
     *  </form>
     *
     * For adding fields add_field() is used. It takes a multidimensional array (keys are ignored) where the first
     * element (key [0]) is the HTML form input tag name (button, select, fieldset, etc.).
     * The second element (optional in some cases) is a key-value array where the keys are any valid attribute for that HTML
     * element
     * and the value its corresponding value. Optionally, it can take a 3rd element when a select or optgroup is inserted.
     * This 3rd element is also a key-value array where the keys are option's value attribute and the values are the text
     * to show.
     *
     * As well as <form> each element may have its own mandatory attributes and they will be checked.
     *
     * To add a text field:
     *
     *  $form = new Form(["name"=>"myform", "method"=>"post", "action"=>"destination.php"]);
     *  $form->add_field(["input", ["type"=>"text", "name"=>"mytext", "required"=>true, "value"=>"bool(true)"]]);
     *  $form->add_field(["input", ["type"=>"text", "name"=>"myothertext", "required"=>"cheese tax", "value"=>"cheese tax"]]);
     *  echo $form->render();
     *
     *  Should output something like:
     *
     *      <form name='myform' action='destination.php' method='post'>
     *          <input type='text' name='mytext' value='bool(true)' required>
     *          <input type='text' name='myothertext' value='cheese tax' required>
     *      </form>
     *
     * This example below should render something like the example above. Notice the add_field() method:
     *
     *  $form = new Form(["name"=>"myform", "method"=>"post", "action"=>"destination.php"]);
     *  $form->add_field(["text", ["name"=>"mytext", "required"=>""], "value"=>"string(0)"]);
     *  $form->add_field(["text", ["name"=>"myothertext", "required"=>null, "value"=>null]]);
     *  echo $form->render();
     *
     *  Output:
     *
     *      <form name='myform' action='destination.php' method='post'>
     *          <input type='text' name='mytext' value='string(0)' required>
     *          <input type='text' name='myothertext' value='' required>
     *      </form>
     *
     *
     * As <input> or <button> can be "tricky" due to the amount of types it is allowed to use the type of input
     * as (pseudo) element.
     *
     * Note that with boolean attributes (such as required or autofocus), if present, no matter its value, will be added.
     *
     * To insert a <select>:
     *
     * $form->add_field(["select",["name"=>"control_select"], ["value1"=>"text1", "value2"=>"text2"]]);
     *
     * To insert a <label> bind to a <textarea>:
     *
     * $form->add_field(["label", ["value"=>"My label", "for"=>"txtareaname"]]);
     * $form->add_field(["textarea", ["id"=>"txtareaid", "name"=>"txtareaname", "cols"=>80, "rows"=>10, "value"=>"Texto del textarea"]]);
     *
     * In the cases of labels and textareas the value attribute is considered as the text to show.
     *
     * To insert a <fieldset>:
     *
     * $form->add_field(["fieldset"]);
     * $form->add_field(["label", ["value"=>"My label", "for"=>"txtareaname"]]);
     * $form->add_field(["textarea", ["id"=>"txtareaid", "name"=>"txtareaname", "cols"=>80, "rows"=>10, "value"=>"Texto del textarea"]]);
     *
     * Take into consideration that fieldsets are closed in render() method. So if you need to add several fieldsets:
     *
     * $form->add_field(["fieldset"]);
     * $form->add_field(["label", ["value"=>"Etiqueta", "for"=>"txtareaname"]]);
     * $form->add_field(["textarea", ["id"=>"txtareaid", "name"=>"txtareaname", "cols"=>80, "rows"=>10, "value"=>"Texto del textarea"]]);
     * $form->add_field(["fieldset"]);
     * $form->add_field(["select",["name"=>"control_select"], ["value1"=>"text1", "value2"=>"text2"]]);
     * $form->add_field(["text", ["name"=>"mytext", "value"=>"", "required"=>""]]);
     * $form->add_field(["fieldset"]);
     * $form->add_field(["label", ["for"=>"mycheck", "value"=>"My check?"]]);
     * $form->add_field(["checkbox", ["name"=>"mycheck", "value"=>"mycheck", "id"=>"mycheck"]]);
     * $form->add_field(["fieldset"]);
     * $form->add_field(["submit", ["name"=>"enviar", "value"=>"enviar"]]);
     *
     *
     * This class produces (or at least aims to produce) a valid HTML form block so when an error occurs an exception is raised.
     * Errors that may occur are incomplete data fields, missing mandatory attributes (or its values)
     * or trying to use an illegal form element.
     *
     * There are certain attributes like role or event handlers for example that are not supported yet.
     * Custom or non-standard attributes are not allowed yet. It is planned for the future.
     * Also it does not verifies for valid attribute's values (will be added in the future).
     * Javascript is not allowed yet. It is planned for the future. However, I have been told that javascript should be handled via event listeners, so....
     * Nested forms or subforms are not allowed.
     * No sanitization is done as this class outputs HTML code straight for echoing so it is not safe to be stored in
     * a database and user input validation or any other kind of validation is up to the developer.
     *
     * Arrays for handling the form elements and its attributes could be optimized. I did not want to have infinite
     * multidimensional arrays and I think it is easier to maintain and to read as it is. It may require some extra
     * if and foreach statements this way.
     *
     * In the future I plan to add strict and non-strict operation modes to allow flexibility and more customization.
     *
     * In the meantime I need to improve what I have until now. I still need to polish elements special treatment, error
     * handling, array optimization and logic.
     *
     * Should other HTML tags such as <br>, <p>, and others be allowed? If I allow them, then I should think bigger than
     * just a class for HTML forms.... I just wanted a simple class to render forms
     *
     *
     */
    class Form {
    
        /** in here each add_field() valid output is stored. it is looped in render() */
        private $output_elements = array();
    
        private $form_elements = ['input', 'label', 'select', 'textarea', 'button', 'fieldset', 'legend',
                                    'datalist', 'output', 'option', 'optgroup'];
        private $form_elements_types = array(
            'input' => ['button', 'checkbox', 'color', 'date', 'datetime-local', 'email', 'file', 'hidden', 'image', 'month',
                'number', 'password', 'radio', 'range', 'reset', 'search', 'submit', 'tel', 'text', 'time', 'url', 'week'],
            'button' => ['submit', 'reset', 'button'],
        );
    
        private $form_elements_common_attr = ['id', 'class', 'accesskey', 'style', 'tabindex'];
        private $form_elements_required_attr = array(
            'form' => ['name', 'action', 'method'],
            'select' => ['name'],
            'textarea' => ['name', 'cols', 'rows'],
            'button' => ['name', 'value'],
            'option' => ['value', 'text'],
            'input' => ['type', 'name', 'value'],
        );
    
        private $form_elements_specific_attr = array(
            'form' => ['target', 'enctype', 'autocomplete', 'rel', 'novalidate'],
            'label' => ['for'],
            'select' => ['autocomplete', 'autofocus', 'disabled', 'form', 'multiple', 'required', 'size'],
            'textarea' => ['autocomplete', 'autofocus', 'disabled', 'form', 'maxlength', 'minlength', 'placeholder', 'readonly', 'required'],
            'button' => ['autofocus', 'disabled', 'form', 'formaction', 'formenctype', 'formmethod', 'formtarget', 'formnovalidate'],
            'fieldset' => ['disabled', 'form'],
            'legend' => [],
            'datalist' => [],
            'output' => ['for', 'form'],
            'option' => ['disabled', 'label', 'selected'],
            'optgroup' => ['disabled', 'label'],
            'input' => ['disabled', 'required'],
            'checkbox' => ['checked'],
            'color' => [],
            'date' => ['max', 'min', 'step'],
            'datetime-local' => ['max', 'min', 'step'],
            'email' => ['list', 'maxlength', 'minlength', 'multiple', 'pattern', 'placeholder', 'readonly', 'size'],
            'file' => ['accept', 'capture', 'multiple'],
            'hidden' => [],
            'image' => ['alt', 'formaction', 'formenctype', 'formmethod', 'formnovalidate', 'formtarget', 'height', 'src', 'width'],
            'month' => ['list', 'max', 'min', 'readonly', 'step'],
            'number' => ['list', 'max', 'min', 'placeholder', 'readonly', 'step'],
            'password' => ['maxlength', 'minlength', 'pattern', 'placeholder', 'readonly', 'size'],
            'radio' => ['checked', 'required'],
            'range' => ['list', 'max', 'min', 'step'],
            'reset' => [],
            'search' => ['list', 'maxlength', 'minlength', 'pattern', 'placeholder', 'readonly', 'size', 'spellcheck'],
            'submit' => ['formaction', 'formenctype', 'formmethod', 'formnovalidate', 'formtarget'],
            'tel' => ['list', 'maxlength', 'minlength', 'pattern', 'placeholder', 'readonly', 'size'],
            'text' => ['list', 'maxlength', 'minlength', 'pattern', 'placeholder', 'readonly', 'size', 'spellcheck'],
            'time' => ['list', 'max', 'min', 'readonly', 'step'],
            'url' => ['list', 'maxlength', 'minlength', 'pattern', 'placeholder', 'readonly', 'size', 'spellcheck'],
            'week' => ['max', 'min', 'readonly', 'step']
        );
        private $form_elements_forbidden_attr = array(); // to implement in the future
    
        private $form_tag_closing = ['label', 'select', 'textarea', 'legend', 'option'];
    
        /**
         *
         * @param array $form Asociative array where the keys are the form tag attributes and the values the attribute's value
         * @throws Exception
         *
         * Attributes name, action and method are mandatory and cannot be empty.
         * The rest of the attributes, if set must have a valid value.
         *
         * Usage:
         *
         *  $form = new Form(['name'=>'myform', 'action'=>'action.php', 'method'=>'post']);
         *
         * Result:
         *
         *  <form name='myform' action='action.php' method='post'>
         *
         */
        function __construct(array $form) {
            $output = "<form";
    
            if (empty($form)) throw new Exception("Form data cannot be empty");
    
            // this foreach could be implemented in, for example, check_required_attr() method... don't figure out if useful
            foreach($this->form_elements_required_attr['form'] as $attr) {
                if (!array_key_exists($attr, $form)) throw new Exception("Form attribute '$attr' is mandatory");
                if (empty($form[$attr])) throw new Exception("Form attribute '$attr' value cannot be an empty string");
                $output .= " $attr='$form[$attr]'";
            }
    
            foreach($this->form_elements_specific_attr['form'] as $attr) {
                if (array_key_exists($attr, $form)) {
                    if (empty($form[$attr])) throw new Exception("Form attribute '$attr' cannot be an empty string");
                    $output .= " $attr='$form[$attr]'";
                }
            }
    
            foreach($this->form_elements_common_attr as $attr) {
                if (array_key_exists($attr, $form)) {
                    if (empty($form[$attr])) throw new Exception("Form attribute '$attr' cannot be an empty string");
                    $output .= " $attr='$form[$attr]'";
                }
            }
    
            $output .= ">";
            $this->output_elements[] = $output;
        }
    
    
    
        function add_field(array $data) {
            $output = null;
            $element = null;
            $type = null;
            $attributes = null;
            $options = null;
    
            if (empty($data)) throw new Exception("Field data cannot be empty");
            if (count($data) < 1) throw new Exception("Field data should be an array of at least one element");
    
            $element = $data[array_keys($data)[0]]; // done this way in case user uses associative array. Should I issue a warning relative to function's parameter format?
            if (empty($element) or !is_string($element)) throw new Exception("Form element's name should contain only letters."); // maybe here I should use a regexp. O maybe delete it as later on is checked against an array... however, I consider it an important advice
    
            // attributes
            if (count($data) >= 2) {
                $attributes = $data[array_keys($data)[1]];
                if (!is_array($attributes)) throw new Exception("Field attributes (2nd element in parameter array) should be an (empty) array");
            }
    
            // options for select, optgroup, etc.
            if (count($data) >= 3) {
                $options = $data[array_keys($data)[2]];
                if (!is_array($options)) throw new Exception("Options parameter (3rd element in parameter array) should be an array");
                if (count($options) < 1) throw new Exception("Options cannot be empty"); // maybe yes (for JS, x ex)?
            }
    
            // check for allowed controls considering "trickyness" on input or button types
            $found = false;
            if (!in_array($element, $this->form_elements)) {
                foreach ($this->form_elements_types as $s_element => $s_types) {
                    if (in_array($element, $s_types)) {
                        $type = $element;
                        $element = $s_element;
                        $found = true;
                        break;
                    }
                }
            } else { $found = true; }
            if (!$found) throw new Exception("'$element': illegal form (type) element");
            unset($found);
    
            $output .= "<$element";
    
            if (array_key_exists($element, $this->form_elements_required_attr)) {
                foreach($this->form_elements_required_attr[$element] as $attr) {
    
                    // input or button
                    if (($attr == "type") and !is_null($type)) { // I have doubts on this validation... needs a better check
                        $output .= " type='$type'";
                    } else {
                        if (!array_key_exists($attr, $attributes)) {
                            // yes yes, messages need to be handled better... I'll get to that later
                            if (!is_null($type)) throw new Exception("'$element $type' attribute '$attr' is mandatory");
                            throw new Exception("'$element attribute '$attr' is mandatory");
                        }
                        // value is the only attribute that can be an empty string but it's mandatory
                        if (($attr != "value") and empty($attributes[$attr])) {
                            if (!is_null($type)) throw new Exception("'$element $type' attribute '$attr' cannot be empty");
                            throw new Exception("$element attribute '$attr' cannot be empty");
                        }
                        $output .= " $attr='".$attributes[$attr]."'";
                    }
                }
            }
    
            // element has specific non-mandatory attributes
            if (array_key_exists($element, $this->form_elements_specific_attr) and (!is_null($attributes))) {
                foreach($this->form_elements_specific_attr[$element] as $attr) {
                    if (array_key_exists($attr, $attributes)) {
                        if (($attr == "required") or ($attr == "autofocus")) $output .= " $attr";
                        else $output .= " $attr='" . $attributes[$attr] . "'";
                    }
                }
            }
    
            if (!is_null($attributes)) {
                foreach($this->form_elements_common_attr as $attr) {
                    if (array_key_exists($attr, $attributes) and !empty($attributes[$attr])) $output .= " $attr='" . $attributes[$attr] . "'";
                }
            }
    
            $output .= ">";
    
            if ($element == "select") {
                foreach($options as $value => $text) $output .= "\n<option value='$value'>$text</option>";
            }
    
            // in this cases (textarea, label, etc.) pseudo-attribute value is used as the element's text
            if (($element != "input") and ($element != "select")) $output .= $attributes['value'];
    
            if (in_array($element, $this->form_tag_closing)) $output .= "</$element>";
    
            $this->output_elements[] = $output;
        }
    
        function render() {
            $output = null;
            $open_fieldsets = 0;
            foreach($this->output_elements as $element) {
                // to handle fieldsets I thought of adding a special attribute to: ["fieldset", ["close"=>true]], but not sure
                if (strpos($element, "fieldset") !== false) {
                    if ($open_fieldsets > 0) {
                        $output .= "\n</fieldset>";
                        $open_fieldsets--;
                    }
                    $open_fieldsets++;
                }
                $output .= "\n".$element;
            }
            if ($open_fieldsets != 0) $output .= "\n</fieldset>";
            return $output . "\n</form>\n";
    
    //        return "\n".implode("\n", $this->output_elements)."\n</form>";
        }
    }

Besides trying to write as standard as I am able I am trying to write something that someone else would use or at least, hopefully, use as a good starting example to more advanced developers.

\$\endgroup\$
0

2 Answers 2

7
\$\begingroup\$

I have not conducted a deep review of your scripting, but I have a few pieces of superficial advice:

  1. Study PSR-12 coding standards and obey every rule. This will make your code easier for you and others to read. Specifically pay attention to when and where to write curly braces.

  2. I see A LOT of misuses/abuses of empty(). The call is most appropriate when needing to determine two things at the same time:

    1. When a variable is not declared AND
    2. when the value is falsey (which includes a zero).

    If a variable is unconditionally declared in a method's signature, then empty() can be replaced with a function-less truthy check or strlen() if desired.

    Calling empty() after isset() or array_key_exists() or key_exists() is an antipattern for the same reason.

    Checking count($data) < 1 after !empty() will never be satified. An empty array is "falsey" data.

  3. Use return type declarations like void and string where appropriate.

  4. I endorse the use of the sprintf() or vsprintf() to print variables to strings. This will make it easier to use double quotes in your generated HTML markup.

  5. I personally never use and or or in PHP code. This helps to avoid precedence issues and assures code consistency.

  6. Endeavor to never use magic numbers. Even checks against 1 or 2 should be avoided. If these numbers have a logical meaning, declare class level constants with indicative names which logically explain their use.

  7. If your class variables (private properties) are not expected to change, write them as constants.

  8. While much of the PHP applications running on the web are building HTML markup from PHP files, there is a growing trend to use client-side languages to generate and populate HTML documents.

  9. When possible, try to avoid using vague variable names like $data, $output, etc. Indicative variable names like $attr, $tag, $form, and $html add meaning. As a general cue, when you find a variable declaration in a method signature that is identical to its neighboring type declaration, this is a symptom that an opportunity to convey meaning is being missed.

  10. I never declare a variable as null if it is intended to contain array-type data (see $attributes). It will be cleaner to define it as an empty array, populate it if appropriate, not check if is is an array later, and simply access it.


LATE EDIT: here is an untested rewrite of the add_field() method implementing some of my advice above. Note: I don't personally like to use snake_case in my projects and I think that the logical segments of this method should be abstracted into small methods which can then be more easily managed and potentially overridden.

/**
 * @throws Exception
 */
public function add_field(string $tag_name, array $attributes = [], array $options = []): void
{
    // validate tag name
    if (!preg_match('/^[a-z][\w-]*$/i', $tag_name)) {  // this is not well researched
        throw new Exception('Name of form element is not valid');
    }

    $html = '';
    $type = null;

    // accommodate fringe case tag names
    if (!in_array($tag_name, $this->form_elements)) {
        $found = false;
        foreach ($this->form_elements_types as $sub_element => $sub_types) {
            if (in_array($tag_name, $sub_types)) {
                $type = $tag_name;
                $tag_name = $sub_element;
                $found = true;
                break;
            }
        }
        if (!$found) {
            throw new Exception($tag_name . ': illegal form (type) element');
        }
    }

    $html .= "<$tag_name";

    // required attributes
    foreach ($this->form_elements_required_attr[$tag_name] ?? [] as $attr) {
        if (!key_exists($attr, $attributes)) {
            throw new Exception(
                sprintf(
                    '"%s%s" attribute "%s" is mandatory',
                    $tag_name,
                    $type === null ? '' : " $type",
                    $attr
                )
            );
        }
        if ($attr !== 'value' && !strlen($attributes[$attr])) {
            throw new Exception(
                sprintf(
                    '"%s%s" attribute "%s" cannot be empty',
                    $tag_name,
                    $type === null ? '' : " $type",
                    $attr
                )
            );
        }

        // append valid attribute
        if ($attr === "type" && $type) {
            $html .= sprintf(
                ' %s="%s"',
                $attr,
                htmlentities($type ?? $attributes[$attr])
            );
        }
    }

    // optional attributes
    foreach ($this->form_elements_specific_attr[$tag_name] ?? [] as $attr) {
        if (key_exists($attr, $attributes)) {
            $html .= sprintf(
                ' %s%s',
                $attr,
                in_array($attr, ['required', 'autofocus']) ? '' : ' "' . htmlentities($attributes[$attr]) . '"'
            );
        }
    }

    // common attributes
    foreach ($this->form_elements_common_attr as $attr) {
        if (key_exists($attr, $attributes) && strlen($attributes[$attr])) {
            $html .= sprintf(
                ' %s="%s"',
                $attr,
                htmlentities($attributes[$attr])
            );
        }
    }

    $html .= ">";

    // build <select> options
    if ($tag_name == "select") {
        // todo: shouldn't <optgroup> tags be implemented here?
        // todo: how does a developer designate the selected option?
        // todo: what about multiselect fields?
        foreach ($options as $value => $text) {
            $html .= sprintf(
                "\n<option%s>%s</option>",
                $value === $text ? '' : ' value="' . htmlentities($text) . '"',
                $text
            );
        }

    // append pseudo-attribute value as element's text (e.g. textarea, label, etc.)
    } elseif ($tag_name !== 'input' && key_exists('value', $attributes)) {
        $html .= htmlentities($attributes['value']);
    }
    
    // append closing tag 
    if (in_array($tag_name, $this->form_tag_closing)) {
        $html .= "</$tag_name>";
    }
    
    $this->output_elements[] = $html;
}
\$\endgroup\$
8
  • 3
    \$\begingroup\$ +1 for everything except that I use (used) and and or exclusively while programming php. They're very readable and I always use parentheses whenever there's a shade of doubt about precedence anyway. \$\endgroup\$
    – Rad80
    Apr 25 at 15:58
  • \$\begingroup\$ Thanks for your time and point out's. I really appreciate everything you said even though I disagree on points 4, 5 and 9. I've read about *printf() vs str_replace() vs preg_replace() and if I should use them I would rather use str_replace() both for readability and performance. I agree with Rad80 on point 5. And regarding point 9 I thought the variable names I chose where quite explicit, but I'll take that into consideration. \$\endgroup\$
    – julio
    Apr 25 at 17:00
  • \$\begingroup\$ Regarding the curly braces... what am I doing wrong? \$\endgroup\$
    – julio
    Apr 25 at 17:07
  • \$\begingroup\$ The usage and placement of curly braces with respect to classes, methods, and condition blocks are all found at php-fig.org/psr/psr-12 \$\endgroup\$ Apr 25 at 20:27
  • \$\begingroup\$ I've been reading your late edit and was thinking of add_field() splitting: then I should have add_tag(), set_attr(), etc. and would make its usage cleaner. I'll go around that way either. As for the TO-DO's in the <select> options, yes <optgroup> goes there (thanks for pointing out selected attr.). I was just having my incomplete yet functioning code reviewed so with the advice I'm getting trying to improve my writing. Should I came back and edit my post when having new code to check or should I ask another question? \$\endgroup\$
    – julio
    Apr 26 at 17:51
6
\$\begingroup\$

You could make it much more simpler to read if you use templating engine. Rendering HTML right inside php is something many experienced developers try to avoid for multitude of reasons.

One of them being that they don't have to think about escaping. If you render HTML directly from your php code you have to take care about escaping manually. Truth be told, I don't see any invocation of an escape function in your code what so ever. A common beginner mistake leading to injection vulnerabilities.

https://www.php.net/manual/en/function.htmlspecialchars.php

Sometimes, you use the old long array syntax array(). This is generally not recommended. Prefer the newer short array syntax [] everywhere. You can use tools like php-cs-fixer or easy-coding-standard to check and fix your code style.

Prefer to put visibility modifiers on your class methods (public/protected/private) to make it clear that you thought about it and make it easier to consumers to understand how it is supposed to be used and avoid misuse. Make public only those methods that are intended to be called from outside. Make protected only if the method is intended to be overridden. Make it private otherwise.

\$\endgroup\$
3
  • \$\begingroup\$ Thansk for your time. I was told to write classes trying them not to depend on anything outside, that's why I don't use templating. I use array() in declarations and when a multidimensional array is used so I know it's a multi array, then I use []. And yes, I'm not escaping anything. I leave that for the developer but (as I wrote on the docblock) I will have to escape input data when checking for correct attribute values and adding JS support. +1 \$\endgroup\$
    – julio
    Apr 25 at 16:49
  • \$\begingroup\$ As for visibility: should __construct() have visibility declared? \$\endgroup\$
    – julio
    Apr 25 at 19:37
  • \$\begingroup\$ @julio yes, definitely. Although most constructors are probably public, it's not that uncommon to use different visibility for constructor. For example singleton design pattern has private constructor by design. Although one might argue about usefulness of the singleton pattern. \$\endgroup\$
    – slepic
    Apr 26 at 3:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.