2
\$\begingroup\$

I am working on a blog with PHP, specially in order to practice OOP.

Since the blogging system will handle users, i thought that a Validation class would be a good idea in order to not repeat code for both login and signup validation.

So far it works, but as you may notice, i'm using static methods from other class here, the UsersTable class, and i feel this is a "code smell".

validation.php:

class Validation
{
    private static $emailValidationRegex = '/^[^\s@]+@[^\s@]+\.[^\s@]+/';

    public static function fieldIsEmpty($field)
    {
        return empty($field);
    }

    public static function validateEmail($email)
    {
        return preg_match(self::$emailValidationRegex, $email);
    }

    public static function usernameExists($username)
    {
        if (UsersTable::getUserByUsername($username))
        {
            return true;
        }

        return false;
    }

    public static function emailExists($email)
    {
        if (UsersTable::getUserByEmail($email))
        {
            return true;
        }

        return false;
    }

    public static function redirectUser($validUser)
    {
        if ($validUser)
        {
            header('Location: ../index.php');
        }
        else
        {
            header('Location: ../login-page.php');
        }
    }

    public static function loginValidation($usernameOrEmail, $password)
    {
        $validUser = false;

        if (!self::fieldIsEmpty($usernameOrEmail) && !self::fieldIsEmpty($password))
        {
            $user = null;

            if (self::validateEmail($usernameOrEmail))
            {
                if (self::emailExists($usernameOrEmail))
                {
                    $user = UsersTable::getUserByEmail($usernameOrEmail);
                }
            }
            else
            {
                if (self::usernameExists($usernameOrEmail))
                {
                    $user = UsersTable::getUserByUsername($usernameOrEmail);
                }
            }

            if ($user !== null)
            {
                if (password_verify($password, $user['password']))
                {
                    $validUser = true;
                }
            }
        }

        self::redirectUser($validUser);
    }

    public static function signupValidation($email, $username, $password, 
                                            $passwordConfirmation, $name)
    {
        echo 'Working so far!';
    }
}

users-table.php:

class UsersTable
{
    public static function getAllUsers()
    {
        $statement = "SELECT * FROM `users`";
        $query = Database::query($statement);

        if ($query)
        {
            return $query->fetchAll(PDO::FETCH_ASSOC);
        }
    }

    public static function getUserByID($id)
    {
        $statement = "SELECT * FROM `users` WHERE `id` = '$id' "
                    . "LIMIT 1";
        $query = Database::query($statement);

        if ($query)
        {
            return $query->fetch(PDO::FETCH_ASSOC);
        }
    }

    public static function getUserByEmail($email)
    {
        $statement = "SELECT * FROM `users` WHERE `email` = '$email' "
                    . "LIMIT 1";
        $query = Database::query($statement);

        if ($query)
        {
            return $query->fetch(PDO::FETCH_ASSOC);
        }
    }

    public static function getUserByUsername($username)
    {
        $statement = "SELECT * FROM `users` WHERE `username` = '$username' "
                    . "LIMIT 1";
        $query = Database::query($statement);

        if ($query)
        {
            return $query->fetch(PDO::FETCH_ASSOC);
        }
    }

    public static function addUser($name, $email, $username, $password)
    {
        $hashedPassword = password_hash($password, PASSWORD_DEFAULT);

        $statement = "INSERT INTO `users` (`name`, `email`, `username`, "
                    . " `password`) VALUES ('$name', '$email', '$username', "
                    . "'$hashedPassword')";

        $query = Database::query($statement);
    }
}

login.php:

require_once('../config.php');
require_once('database.php');
require_once('users-table.php');
require_once('validation.php');

Database::connect();

Validation::loginValidation($_POST['username-email'], $_POST['password']);
\$\endgroup\$
4
\$\begingroup\$

It's a very good intention but rather bad implementation. The first and foremost problem is

SQL injection

In a code written in 2018 it's a shame to have such a blatant one. Your Database::query method must support two arguments, one is an SQL statement and one is an array with data to bind. You can see an example implementation here. So the code would be

    $statement = "SELECT * FROM `users` WHERE `email` = ? LIMIT 1";
    $query = Database::query($statement, [$email]);

Same goes for all other queries. insert for example:

$statement = "INSERT INTO `users` (`name`, `email`, `username`, `password`) 
              VALUES (?,?,?,?)";
Database::query($statement,[$name, $email, $username, $hashedPassword]);

Fixing this is the most important thing that needs to be done. Other issues are less important, more on the proper OOP structure and code optimization.

Validation class

  1. function fieldIsEmpty($field) is questionable, for two reasons:
    • running empty() against a variable that is deliberately set is overkill
    • a dedicated function to duplicate the empty()'s functionality? It could make sense but still I am not sure if it worth
  2. function validateEmail($email) - there is a built-in function, filter_var with a FILTER_VALIDATE_EMAIL flag which should be better than a homebrewed regexp.
  3. function redirectUser($validUser) doesn't belong to validation. What if you will need to implement a command-line utility to create a user? Where it will be supposed to redirect? It should be another class responsible to HTTP interaction.
  4. function loginValidation doesn't belong to validation class either. It does validate a user, so it should belong to either a User class or a UserValidation class, which may inherit from the Validation class.
  5. Same goes for usernameExists() and emailExists()
  6. Also, loginValidation() is too verbose and making way too much SQL queries. The followed code should be enough (although still doing one extra query)

    public static function loginValidation($usernameOrEmail, $password)
    {
        $user = UsersTable::getUserByEmail($usernameOrEmail);
        if (!$user)
            $user = UsersTable::getUserByUsername($usernameOrEmail);
        }
        return ($user && password_verify($password, $user['password']));
    }
    

    again: a function named somethingValidation() under no circumstances should redirect anyone anywhere. It should return a boolean value that could be used elsewhere.

  7. There are a lot of unnecessary conditions. Most functions xould be rewritten to return the result right away:

    public static function usernameExists($username)
    {
        return (bool)UsersTable::getUserByUsername($username));
    }
    public static function getUserByEmail($email)
    {
        $statement = "SELECT * FROM `users` WHERE `email` = ? LIMIT 1";
        return Database::query($statement, [$email])->fetch(PDO::FETCH_ASSOC);
    }
    

    and so on

login.php

Given the inmformation above, its code should be more like

if (UserValidation::loginValidation($_POST['username-email'], $_POST['password'])) {
    HTTP::redirect("../index.php");
} else  {
    HTTP::redirect("../login-page.php");
}
\$\endgroup\$
  • \$\begingroup\$ So many great insights here. I saw that way of passing data for making a query and i thought i was being too smart for avoiding it. Thank you for correcting me. 1. Yes, i agree, i'm just duplicating empty. 2. Awesome to know that there is a built-in function for this, i wasn't actually very pleased with using that regex. 3, 4, 5: How should i approach this then? What about the signup validation? Is it also UserValidation? 6: Don't know how i didn't see this cleaner approach before. Great!. 7: Yes, i was noticing my use of too many conditions. What about HTTP::redirect vs. Header? \$\endgroup\$ – ManuAlvarado22 Sep 25 '18 at 16:10
  • 1
    \$\begingroup\$ HTTP::redirect is a class and a method that are yet to be written, the latter is just a wrapper for Header location (obligatory followed by die statement by the way). loginValidation do not belong to Validation but to User. regarding 5 I am not sure. You can create a userValidation class extending validation and use it to validate a user. \$\endgroup\$ – Your Common Sense Sep 26 '18 at 7:27
2
\$\begingroup\$

i'm using static methods from other class here, the UsersTable class, and i feel this is a "code smell".

Some people hate static methods, others prize them for the micro-optimization... if your method is idempotent (or, in the case of a query, not dependent upon class state) you're not in code smell territory.

validation.php

private static $emailValidationRegex = '/^[^\s@]+@[^\s@]+\.[^\s@]+/';

If the string never changes, why assign it to a variable? (Consider using a class constant if you feel the string must be separate or - better yet - just use the quoted string directly in the single place that it is needed)

x@y.z may try signing up - I hope you require that users confirm their email address in order to complete account activation if a valid email address is a business requirement.

users-table.php

$statement = "SELECT * FROM `users`";

This may qualify as a microptimization quibble, (depends upon your DDL) however, I'd recommend explicitly specifying the columns in queries to avoid an extra check against the information schema.

$statement = "SELECT * FROM `users` WHERE `id` = '$id' "
            . "LIMIT 1";

Don't invite Little Bobby Tables into your database - use prepared statements.

login.php

require_once('../config.php');
require_once('database.php');
require_once('users-table.php');
require_once('validation.php');

No PSR-4 Autoloader ..? (You'll want one - save yourself a lot of hassle in the future)

Note: I may have missed a few bugs, however, I will say that you're on the right track in terms of producing consistent, formatted code - keep at it!

\$\endgroup\$
  • \$\begingroup\$ Please be advised that the article on the prepared statements you linked to has been plagiarized from my PDO tutorial. \$\endgroup\$ – Your Common Sense Sep 25 '18 at 7:58
  • \$\begingroup\$ @YourCommonSense Ouch - the plagiarist had you beat for SEO. Link updated. \$\endgroup\$ – Gray Sep 25 '18 at 12:26
  • \$\begingroup\$ Excellent points here, thank you so much. I'm not sure about what does a method not dependant on class state means, but i do have to do Database::connect() before using any of the methods in either the Database class or the UsersTable class. I am going to definitely check what PSR-4 Autoloader is. And overall to take into account all of your great points. Thanks a lot! \$\endgroup\$ – ManuAlvarado22 Sep 25 '18 at 16:19
  • \$\begingroup\$ @ManuAlvarado22 If your static method references a static::$_var then it is probably not idempotent (though, if you're using a variable where you should be using a class constant, it may be OK). Consider a call to Database::connect() in every static method that needs a connection (just return true if already connected in connect()) \$\endgroup\$ – Gray Sep 25 '18 at 18:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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