PHP form simple validation

Question

I'm quite new to php (learned and only used C# for web pages so far) and wanted to know if this is the correct approach for validating a form. This is my userform.php code:

<!--Includes HTML, Head, Scripts, CSS, Body and begin of Content div-->
<?php include realpath(__DIR__)."/layout/header.php"; ?>
<?php 
    function Validate() {
        if ($_SERVER['REQUEST_METHOD'] === 'POST') {
            return IsValid("username") && IsValid("password") && IsValid("usermail");
        } else {
            return true;
        }
    }
    function IsValid($name) {
        if ($_SERVER['REQUEST_METHOD'] === 'POST') {
            switch($name){
                case 'username':
                    return !isset($_POST["username"]) || strlen($_POST["username"]) > 0;
                    break;
                case 'password':
                    return !isset($_POST["password"]) || strlen($_POST["password"]) > 0;
                    break;
                case 'usermail':
                    return strlen($_POST["usermail"]) > 0 && filter_var($_POST["usermail"], FILTER_VALIDATE_EMAIL);
                    break;
            }
        } else {
            return true;
        }
    }
    function GetErrorMessage($name) {
        if ($_SERVER['REQUEST_METHOD'] === 'POST') {
            switch($name){
                case 'username':
                    return !isset($_POST["username"]) || strlen($_POST["username"]) == 0 ? 'Username is required' : '';
                    break;
                case 'password':
                    return !isset($_POST["username"]) || strlen($_POST["password"]) == 0 ? 'Password is required' : '';
                    break;
                case 'usermail':
                    $message = '';
                    if (strlen($_POST["usermail"]) == 0) {
                        $message = $message.'E-mail is required';
                    }
                    if(strlen($_POST["usermail"]) != 0 && !filter_var($_POST["usermail"], FILTER_VALIDATE_EMAIL)) {
                        if(strlen($message) > 0) {
                            $message = $message.'<br />';
                        }
                        $message = $message.'"'.$_POST["usermail"].'" is not a valid e-mail address';
                    }
                    return $message;
                    break;
            }
        } else {
            return '';
        }
    }
?>
<div class="row">
    <div class="col-md-4">
        <form action="./userform.php" method="POST" role="form">
            <div class="form-group <?php echo IsValid('username') ? '' : 'has-error' ?>">
                <label for="username">Username</label>
                <input type="text" id="username" name="username" class="form-control" value="<?php echo Validate() ? '' : $_POST["username"] ?>" />
                <span class="help-block"><?php echo GetErrorMessage('username') ?></span>
            </div>
            <div class="form-group <?php echo IsValid('password') ? '' : 'has-error' ?>">
                <label for="password">Password</label>
                <input type="password" id="password" name="password" class="form-control" value="<?php echo Validate() ? '' : $_POST["password"] ?>" />
                <span class="help-block"><?php echo GetErrorMessage('password') ?></span>
            </div>
            <div class="form-group <?php echo IsValid('usermail') ? '' : 'has-error' ?>">
                <label for="usermail">E-Mail</label>
                <input type="text" id="usermail" name="usermail" class="form-control" value="<?php echo Validate() ? '' : $_POST["usermail"] ?>" />
                <span class="help-block"><?php echo GetErrorMessage('usermail') ?></span>
            </div>
            <div class="form-group">
                <button type="submit" class="btn btn-default">Submit</button>
            </div>
        </form>
    </div>
    <?php
        if ($_SERVER['REQUEST_METHOD'] === 'POST' && Validate()) {
            echo '<div class="col-md-4"><h4>Data from POST</h4><p>Username: '.$_POST["username"].'</p><p>Password: '.$_POST["password"].'</p><p>E-Mail: '.$_POST["usermail"].'</p></div>';
        } else if($_SERVER['REQUEST_METHOD'] === 'POST' && !Validate()) {
            echo '<div class="col-md-4"><h4>Data from POST is not valid</h4></div>';
        }
    ?>
</div>
<!--Includes End of Content div, Body and HTML-->
<?php include realpath(__DIR__)."/layout/footer.php"; ?> 

The userform.php should work for GET and POST requests, thus the $_SERVER["REQUEST_METHOD"] checks everywhere.

Edit: The GET requests is just for the empty form. Posting the form should be handled in the same file.

Question 1: Is there a better way to get the validation states and the error messages for my form fields? Writing them into two seperate methods seems to be kind of overkill for me.

Usually I would encapsulate the code for validation in a single method that returns some kind of dictionary which allows me to check the count and if a form field is included (Preferably with LINQ).

Question 2: Is there a better way to echo html back to the user? Writing all the html code into a single echo seems to be not very clean. Assuming I have to output a bigger portion of html with the same if else clause as above, it would be very confusing to write and edit the html code.

Question 3: Although it works is realpath(__DIR__) the correct usage to include files from a different folder?

Answer 1

Some good commentary from @Iwresteldabearonce so I won't get into any of those points. I think you are missing some higher-level concerns:

• You should strive to place all of your application request handling logic (i.e. validation, header injection, calling of other resources like databases, etc.) before display logic. In this case, that means moving your validation logic ahead of your first header include. Once you have started rendering display (returning HTML response to browser for example), your options for how you can formulate the response have been drastically reduced. Your code is especially problematic in that you are not even validating POST input until after the form is rendered. You can no longer change HTTP status code, redirect, set cookies, or other sort of header manipulation. Separate your application logic from your display logic to the fullest extent possible, using templates and/or very minimal PHP constructs (conditionals, loops, etc.) as needed to choreograph the content from the application logic into final layout.
• You stated in your review request that the "posting the form should be handled in the same file" which to me suggests that you are still thinking about your code in terms of single linear scripts that hold all the code for one website page. I would suggest you move past this restriction unless there is some compelling reason to keep potentially re-usable code like your validation functions within each individual page file that needs them. I think over time you will find yourself much better served by separating your code that declares symbols (classes, functions, constants, etc.) from your code that creates side-effects (generating output, interacting with other resources, reading/writing to files, etc.). This is fact in the current PSR-1 recommendation. So, consider moving these functions out of this file if you feel you want to maintain your own validation functions.
• Dealing with $_REQUEST is almost always a sign that the developer is being imprecise in defining application behavior. There are very few times where it is desirable to have an application behave in some common manner between GET and POST (or any other HTTP action). You should really think about treating these as logically separate "routes" in your application. By that I mean that you have some means to conditionally execute one logical controller to handle POST and another for GET.

For example:

switch($_SERVER['REQUEST_METHOD']) {
    case 'GET':
        require '/path/to/get/controller';
        break;
    case 'POST':
        require '/path/to/post/controller';
        break;
    default:
        // unexpected HTTP request method, perhaps return 400 error
}

• Your approach to validation is not very scalable. If you take above advice about moving validation functions into separate file, you will quickly see that the functions, though separated, are not re-usable. They are tightly coupled to this page with the way they explicitly list all expected values in switch statements and similar. As you tried to use these functions elsewhere in your application, the switch statements would quickly balloon in size, and also limit your re-use of different "keys" for validation rules. So, does it even make sense to take these types of validation functions and make them re-usable? Probably not. Here is where you need to think about whether you want to be in the business of writing your own validation functions, or more heavily leverage either validation libraries or built-in functions like filter_input_array() to where the only validation work that really happens on any given page, is simply the definition of the validation rules, not performing the validations themselves.
• Your validation rules seem a little trivial. Is having a non-empty string really your only validation requirement for a username or password?
• Your getErrorMessage() is really odd in that you are basically re-validating the input. Why? Typically you should generate/store all error messages at the point of validation, you should later be able to access the error messages during display rendering. Again, I think once you properly apply application logic before display logic, your need for such a function will go away. By the way, do you really need to get into complexity around determining an empty email input value from one that doesn't match the filter? Either way, the value won't pass the filter, either way the user will need to be prompted that they have an invalid value in this field, should you simplify here and not having multiple error states for this single field?
• You are doing nothing to prevent cross-site request forgery (CSRF) attacks. Typically mitigation would consist of token stored in session and compared against a copy of that token placed into hidden form field and validated when posted.

Answer 2

The userform.php should work for GET and POST requests, thus the $_SERVER["REQUEST_METHOD"] checks everywhere.

If you want it to work for both request types then why are you only validating $_POST? That's a major bug. If GET is used none of the input will get validated.

To fix that, I would suggest creating a method to determine if the form has been submitted, maybe something like...

function isFormSubmitted(){
    return !empty($_REQUEST['usermail']) &&
        !empty($_REQUEST['password']) &&
        !empty($_REQUEST['username']);
}

$_REQUEST contains both $_GET and $_POST data, so you can use that in place of $_POST.

Now, everywhere you have that request method check, instead just do if(isFormSubmitted()){...}, and everywhere you use $_POST, replace it with $_REQUEST.

Then your code will work with both GET and POST and it will also be quite a bit shorter.

---

Question 1: Is there a better way to get the validation states and the error messages for my form fields? Writing them into two seperate methods seems to be kind of overkill for me.

You can save a few lines in each of your switches by combining cases that are identical. You can save 10% on car insurance by switching to Geico. For example...

 switch($name){
     case 'username':
     case 'password':
         return empty($_REQUEST[$name]) ? ucfirst($name).' is required' : '';
     case 'usermail':
         ...
}

You can shorten !isset($_REQUEST[$name]) || strlen($_REQUEST[$name]) == 0 to a single function that checks both, empty().

Also, there's really no point in breaking the switch after you've returned.

---

Question 2: Is there a better way to echo html back to the user? Writing all the html code into a single echo seems to be not very clean. Assuming I have to output a bigger portion of html with the same if else clause as above, it would be very confusing to write and edit the html code.

Ideally you should separate your markup from your PHP as much as possible, but when you have to do something like that in the middle of your markup, colon notation is usually neater than bracket notation.

<div class="col-md-4">
    <?php if (isFormSubmitted() && Validate()): ?>
        <h4>Data from POST</h4>
        <p>Username: <?php echo $_REQUEST["username"]; ?></p>
        <p>Password: <?php echo $_REQUEST["password"]; ?></p>
        <p>E-Mail: <?php echo $_REQUEST["password"]; ?></p>
    <?php else if(isFormSubmitted() && !Validate()): ?>
        <h4>Data from POST is not valid</h4>
    <?php endif; ?>
</div>

You can also clean up a little bit by putting the div on the outside of the logic instead of duplicating it. You'll get an empty div there if the form is not submitted though, so consider that.

---

Question 3: Although it works is realpath(__DIR__) the correct usage to include files from a different folder?

It's one of the correct ways. That's how I do it.