Create elasticsearch document from php

Can you please provide some improvement advice for the following function?
It is working OK now, but as I have zero experience with PHP I guess there is a lot of room for improvement.

function addNewUser($ES_Client, $username, $password, $fullname, $email, $function, $department, $access){
    auditLog($ES_Client, "Creating new user: ".$username.", ".$fullname.", ".$email.", ".$function.", ".$department.", ".$access);
    $checkIfUserExists = getUserDetails($ES_Client,$username)['hits']['total'];
    if($checkIfUserExists != 0){
        auditLog($ES_Client, "User ".$username." already exists! Operation aborted.");
        return -1;
    } else {
        $hashed_password = crypt($password);
        $userAddQuery= [
            'index' => 'system',
            'type' => 'users',
            'body' => [
                'username' => $username,
                'password' => $hashed_password,
                'fullname' => $fullname,
                'email' => $email,
                'function' => $function,
                'department' => $department,
                'access' => $access,
                'lastlogin' => round(microtime(true) * 1000)
            ]        
        ];
        try{
            $userAddResponse = $ES_Client->index($userAddQuery);
            if($userAddResponse['created'] == 1)
            {
                auditLog($ES_Client, "User ".$username." added successfully!");
                return 0;           
            }
            else{
                auditLog($ES_Client, "Error adding user ".$username."! Did not receive acknowledgement from elastic.");
                return -1;
            }
        } catch(Exception $e) {
            auditLog($ES_Client, "Error adding user ".$username."! ".$e->getMessage());
            return -1;
        }
    }
}

Guard Clauses

You can save a level of nesting by using guard clauses. In this case, it just means removing the else:

auditLog($ES_Client, "Creating new user: ".$username.", ".$fullname.", ".$email.", ".$function.", ".$department.", ".$access);
$checkIfUserExists = getUserDetails($ES_Client,$username)['hits']['total'];
if($checkIfUserExists != 0){
    auditLog($ES_Client, "User ".$username." already exists! Operation aborted.");
    return -1;
}

$hashed_password = crypt($password);
[...]

Error Handling

First of all, your return values are quite confusing. If you were to check them, you would get code like this:

if (addNewUser(...)) {
    // handle the error
}

The problem is that 0 is actually false, while -1 is true. But really, returning integers as error codes is confusing and difficult to use. The numbers themselves don't have any meaning, so you always need to look up what they mean in the documentation (if it even exists).

If you just have a boolean state, you might as well return boolean values.

But really, I would prefer to throw exceptions. Not being able to add a user is an exceptional state after all. This also gives you the option of being more explicit about what went wrong, so a calling method may have a change to recover, or show a better error message to the user.

Hashing

Don't use crypt, especially not without additional arguments. The documentation explicitly warns against this, as it is not secure:


The salt parameter is optional. However, crypt() creates a weak password without the salt. PHP 5.6 or later raise an E_NOTICE error without it. Make sure to specify a strong enough salt for better security.

You should just use password_hash.

Formatting

• Your indentation is partly off.

• You are not consistent with your curly bracket positions.

• You are also not consistent with your use of spaces.

Use an IDE to fix all of these issues automatically.

Naming

You should be consistent with your variable names. Don't mix camelCase and snake_case without reason.

Generally, your variable names are very clear. But you could shorten some without loosing any meaning. checkIfUserExists for example may be userExists. Although this actually doesn't seem to represent what the variable actually holds (it holds some counter instead).

StackExchange Code Review Q#143026, answer score: 2