0
\$\begingroup\$
$controller = new UserController();

if (isset($_POST['submitform']))
{   
    $validated = false;

    $inputs = array (
        'username', 'email', 'password',
        'repassword', 'password_f', 'repassword_f',
        'display'
    );

    $i = 0;

    foreach ($inputs as $key)
    {
        if (isset($_POST[$key]) && !empty($_POST[$key]))
        {
            $i++;
            if ((int)$i == count($inputs))
            {
                $validated = true;
                break;
            }
            else
            {
                $controller->error = "";
                break;
            }
        }
    }

    if ($validated)
    {
        echo 2;
    }
}

So let's overview the code.

First we are checking if the form was submitted.

Then we are creating a new boolean $validated and setting it false by default.

Then we created our array with the POST names.

Now the checking part, setting variable int i to 0 by default. Now we are looping through the array elements.

Checking if the current index was set or not empty, if yes, add +1 to int $i.

Once int $i hits the number of counted elements of our array, set $validated to true, and break out of the array, else parse error and break out.

Then we are checking, if bool $validated is true, then echo 2.

Is it a good way of doing this? Is there any cleaner/better way?

\$\endgroup\$
1
  • 1
    \$\begingroup\$ I would discourage you from using flags. Don't set $validated in the first place, instead, when you need to, just call the desired function. \$\endgroup\$
    – Jeff Noel
    Jul 15, 2013 at 17:45

2 Answers 2

1
\$\begingroup\$

I'd do this instead:

function validateForm() {
    if (!isset($_POST['submitform']))
        return false; // Maybe $controller->error = "Something"; as well?
    $inputs = array (
        'username', 'email', 'password',
        'repassword', 'password_f', 'repassword_f',
        'display'
    );
    foreach ($inputs as $key)
        if (empty($_POST[$key]))
        {
            global $controller;
            $controller->error = "";
            return false;
        }
    return true;
}
...
if (valudateForm()) {
    echo 2;
} else {
    // Form data not ok
}

Seems cleaner, no flags, no counting array elements, validation has its own function,...

\$\endgroup\$
7
  • \$\begingroup\$ Although your answer is pretty close to what I would do, I can't upvote it with the global in it :) (Maybe just return the keys of the invalid input and build the error message in the controller) \$\endgroup\$ Jul 16, 2013 at 7:40
  • 1
    \$\begingroup\$ global is good if used properly. Look at Drupal core. They use global all over the place, but that doesn't make it bad code... \$\endgroup\$
    – Pinoniq
    Jul 16, 2013 at 8:45
  • \$\begingroup\$ @mnhg Global variables can make all kinds of chaos, I agree. However, the above doesn't seem like a reusable code, but rather a single script, in which $controller seems like something common for the whole site. I see no problem global-ing something in such case, and I believe that nothing sould ever be discarded as "generally wrong" without addressing the exact situation. If you have a problem with global in here, I'd love to hear it (possibly learning something useful). \$\endgroup\$ Jul 16, 2013 at 9:21
  • \$\begingroup\$ @Vedran Nip it in the bud. It is unnecessarily hard to write isolated test with globals and do be sure to don't mess thinks up with new tests or a change to a global. This is no local script, but a web page, so no need for hacking. \$\endgroup\$ Jul 17, 2013 at 7:22
  • \$\begingroup\$ @Pinoniq I'm not familiar with Drupal in detail. Do you have any articles about testing-best-practice in Drupal in relation to globals? \$\endgroup\$ Jul 17, 2013 at 7:29
0
\$\begingroup\$

First, it's not a real validation here. The check is: are all required fields filled with some value? Please notice that all modern browsers can use the 'required' attribute in formfields and will not post a form while not all required fields are filled. But double checking server-side is of course always good practice. Please, keep functions as clean as possible. They only have to do one thing. So: no error-messages in the function etc. Only return true or false. That way, you can re-use your function. So I think this simple function will do just fine.

<?php
/**
 * Check if all required fields are filled
 *  
 * @param array $post the POST array
 * @param array $fieldsArray an array with values to check
 * @return boolean
 */
function filledRequiredFields($post, $fieldsArray)
{
    foreach($fieldsArray as $key)
    {
        if(empty($post[$key]))
        {
            return false;
        }
    }
    return true;
}

if ($_SERVER['REQUEST_METHOD'] === 'POST')
{
    $inputs = array(
        'username', 'email', 'password',
        'repassword', 'password_f', 'repassword_f',
        'display'
    );

    if (filledRequiredFields($_POST, $inputs))
    {
        echo 2;
    }
    else 
    {
        //etc.
    }
}
\$\endgroup\$

Your Answer

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

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