PHP User class usability,maintainability,secure

Question

I'm new in oop programming pattern. I created this simple User Class and i'm wondering if it :

Is following oop rules and logic ?
Is maintaibable ?
Is can be tested without any problems ?
Can be expanded to a large user class (permissions,etc) ?
Is secure ?

this is my class

<?php

class User {

private $data;
private $errors;
public CONST FIELD_OPTION = [
    'user_login' => [
        'callback' => 'is_user_login_exists',
        'error_message' => 'is already exists',
        'response' => true
    ],
    'user_email' => [
        'callback' => 'is_email_exists',
        'error_message' => 'Email is already exists',
        'response' => true
    ],
    'user_pwd' => [
        'callback' => 'is_password_valid',
        'error_message' => 'Password is not valid',
        'response' => false
    ]
];

function __construct( array $data = [] ) {
    $this->dataParser( $data );
}

/**
 * 
 */
public function get( array $filters = [] ) {

    $db = db();

    $query = $db->dsql()->table('users');

    if(!empty($filters) && is_array($filters)) {
        foreach($filters as $key) {
            try {
                $value = $this->getData($key, true);
                $query->where($key, $value);
            }

            catch(Exception $e) {
                $this->errors['data'][$key] = $e->getMessage();
            }

        }
    }

    return $query->get();

}

/**
 * Create new user
 */
public function create() {

    $db = db();

    $data = [
        'user_pwd' => $this->getData('user_pwd'),
        'user_login' => $this->getData('user_login'),
        'user_email' => $this->getData('user_email'),
        'user_name' => $this->getData('user_name')
    ];

    $query = $db->dsql()->table('users')->set($data);

    $insert = $query->insert();

    return $insert;

}

/**
 * Verifying all fields
 * 
 * @param  mixed(bool on failire|null on success)    $fields
 */
public function vertify_fields( $requireds ) {

    if(is_string($requireds)) {
        $requireds = [$requireds];
    }

    if(!is_array($requireds)) {
        return false;
    }

    $fields = self::FIELD_OPTION;

    foreach( $requireds as $field_name ) {

        $field = $fields[$field_name];
        $value = $this->getData($field_name);
        if( empty($value) ) {
            $this->errors['fields'][$field_name] = 'This field is required';
            continue;
        }

        if($field['response'] === call_user_func_array($field['callback'],[$value])) {
            $this->errors['fields'][$field_name] = $field['error_message'];
        }

    }

}

public function identify() {
    /**
     * We identify user by :
     * @string $user_login
     * @string $user_pwd
     */
    $user_login = $this->getData('user_login');
    $user_pwd = $this->getData('user_pwd');

    $db  = db();

    $query = $db->dsql()->table('users')->where('user_login', $user_login)->limit(1);

    $user = $query->getRow();

    if(!$user) {
        return false;
    }

    $user_pwd_hash = $user['user_pwd'];
    return password_verify($user_pwd, $user_pwd_hash);

}

/**
 * Return mixed(null|array)
 */
public function get_errors() {
    return $this->errors;
}

/**
 * check if @prop $errors is null or not
 */
public function have_errors() : bool {
    return ($this->errors === null) ? false : true;
}

/**
 * 
 * @param   mixed   $key
 * @return string
 */
private function getData( $key, bool $force = false ) {
    $value = $this->data->$key ?? null;
    if( $force && is_null($value) ) {
        throw new Exception($key . ' is missing');
    }

    return $value;

}

private function dataParser( array $data ) : void {
    $this->data = (object) $data;
}

}

this is a example demonstrate how I'm using this class to create new users

function create_user_ajax() {

    $response = ['status' => 'error'];

    $User = new User($_POST);

    $User->vertify_fields(['user_email', 'user_login', 'user_pwd']);

    if($User->have_errors()) {
        $response['errors'] = $User->get_errors();
    }else{
        $User->create();
        $response['status'] = 'success';
    }

    echo json_encode($response);

}

I appreciate any suggestion, improvement. thank you in advance.

Your title seems to describe the concerns you have instead of what the script does. Please edit. Does vertify_fields() make the data non-horizontal? — mickmackusa
– mickmackusa, Commented Aug 19, 2019 at 21:55
@mickmackusa I'm not sure about what do you mean by non-horizontal — Mourad Karoudi
– Mourad Karoudi, Commented Aug 19, 2019 at 22:00
...you have a typo in your method name, it should be verify. — mickmackusa
– mickmackusa, Commented Aug 19, 2019 at 22:01

KIKO Software · Accepted Answer · 2019-08-22 13:56:24Z

I can understand why no one has yet answered your question. The basic problem is that your User Class is not really an User Class. All it does is process user input, and that's not something an User Class should do. An User Class should deal with an user, nothing else. Things like:

Changing user name and/or password.
Verifying passwords.
Keep information about an user.
Keeping user information in the database up to date.
permissions.

Basically anything that has to do with the user once the user is known. The basic structure of an user class therefore should be:

class User {

    public function __construct($userId) {
    }

}

Your code seems to be dealing with a login form. Your class therefore should be called something like: LoginFormInput. A good login can result in an User Class, but the processing of user input should not be done in the User Class itself.

A good guide into designing a class, like this, is the Model–view–controller (MVC) pattern. It clearly separates input, internal logic, and output.

Talking about user input processing. This should be done with care. How you deal with user input largely determines how secure your code is. Dumping all of $_POST directly into a class can be safe, if that class was intentionally designed to deal with it, but in most cases you want to filter user input at the top of a PHP script. In your case you could do something like:

<?php

$input["login"]    = filter_input(INPUT_POST, "user_login", FILTER_SANITIZE_STRING);
$input["password"] = filter_input(INPUT_POST, "user_pwd",   FILTER_UNSAFE_RAW);
$input["email"]    = filter_input(INPUT_POST, "user_email", FILTER_SANITIZE_EMAIL);
$input["name"]     = filter_input(INPUT_POST, "user_name",  FILTER_SANITIZE_STRING);

Only four inputs are now accepted, nothing else, and most of them are sanitized before they go into $input. Notice that I intentionally did not filter the password. This way any user password is possible. This filtering therefore doesn't make the content of $input safe, it is still user input and should be treated with care.

I normally process each form in its own PHP script, specifically written for that form. The code above would be the start, and most of your User Class would form the rest of the code. The end result could be a valid $userId which can be stored in the session, and can be used to create an User object.

One other thing in your code, that goes against everything I have learned, is the use of call_user_func_array() in verify_fields(). Just don't do that. It is bad for a lot of reasons, like maintainability and testability, but mostly because it is simply difficult to understand what it exactly does.

Your error processing is also unclear. You use both exceptions and error responses. It is one or the other, not both.

Also pay attention to the names you choose. I've already discussed the class name, but there's more. The obvious typo in vertify_fields(), a dataParser() method that doesn't do any parsing, and I still wonder what the differences between the get() and getData() methods are. I can't tell from the name.

A class should be used to encapsulate the inner workings of whatever the class is dealing with. It should abstract away from the details and give you a clean interface. Your class doesn't do this.

In the end I have to conclude that your class is probably syntactically correct, but that's about it. It seems like you haven't fully understood the reasons why we use OOP.

There are lot's of tutorials on the internet. Some of them only explain the syntax, others explain how objects relate to each other, but there are very few that correctly explain how to use them effectively. I can't find one I really like. However, if you take them all together you will get the idea:

https://www.guru99.com/object-oriented-programming.html

https://code-boxx.com/simple-php-mvc-example

https://www.valuebound.com/resources/blog/object-oriented-programming-concepts-php-part-1

https://www.studytonight.com/php/php-object-oriented-programming

thank you for your time and clarification. I really appreciate that. I'm using call_user_func_array to call a function ! the logic I use is that I loop through an array and every input have a function that check it validity. like user_email have function is_email_exists. in my opinion that calling fields one by one will make the code larger . On the contrary using this way we can store inputs configurations in separated array. and that make code more cleaner ! — Mourad Karoudi
– Mourad Karoudi, Commented Aug 22, 2019 at 12:25
@MouradKaroudi I understand your reason for using call_user_func_array() but you asked whether your class is maintainable, testable, and follows common sense rules. This clearly doesn't. That's all that I'm saying. — KIKO Software
– KIKO Software, Commented Aug 22, 2019 at 13:48
Notice that filter_input(INPUT_POST, "user_email", FILTER_SANITIZE_EMAIL); will fail for emails like [email protected], which has dots on the local-part. — Ismael Miguel
– Ismael Miguel, Commented Aug 22, 2019 at 15:54
@IsmaelMiguel You're talking about FILTER_VALIDATE_EMAIL, whereas I used FILTER_SANITIZE_EMAIL. — KIKO Software
– KIKO Software, Commented Aug 22, 2019 at 18:15

Stack Exchange Network

PHP User class usability,maintainability,secure

1 Answer 1

You must log in to answer this question.

Hot Network Questions

PHP User class usability,maintainability,secure

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions