Create elasticsearch document from php

Question

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;
        }
    }
}

Answer 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).

Answer 2

Added onto what @tim mentioned. I would suggest:

Strict comparisons

Change == to === and != to !==

See: https://www.copterlabs.com/strict-vs-loose-comparisons-in-php/

Answer 3

function addNewUser($ES_Client, $username, $password, $fullname, $email, $function, $department, $access){

This line of code is way too long, making your code hard to read. When you find yourself passing this many parameters to a function,then you should probably be questioning if either the function is trying to do too much or whether those parameters might better be passed in a a different way. Here, for example, perhaps you could build and pass a UserConfig object or similar that would store all the information needed to configure a user in the system. This would also allow you to make sure the dependency is set up properly before this function is ever called.

Right now, you do nothing to validate that your are getting expected parameter types and value ranges before just going ahead and trying to work with the parameters. It would be better to validate you have have everything you need to create a user and returning an exception to the caller if the dependencies are not met.

Since $ES_Client is an object, you should specify type hint for this parameter.

---

auditLog($ES_Client, "Creating new user: ".$username.", ".$fullname.", ".$email.", ".$function.", ".$department.", ".$access);

Another line of code that is too long.

$checkIfUserExists = getUserDetails($ES_Client,$username)['hits']['total'];

Seems like maybe you should have a class which deals with accessing user information, not a bunch of standalone functions.

For creating users, why do you need to check if this document already exists. You should be able to go directly to creation of document and would just need to handle the condition where a unique document already exists. This will save you a call every time you try to create a user.

This is similar pattern to how one should insert in most traditional relational database for uniquely indexed records.

---

if($checkIfUserExists != 0){
    auditLog($ES_Client, "User ".$username." already exists! Operation aborted.");
    return -1;
} else {

See note above about whether this is even needed here. Even if you keep it, there is no reason for the else as the condition already returns. Speaking of returns, I agree with comments in the other answers about not returning these integer values in favor of something more meaningful to caller.

---

    $hashed_password = crypt($password);

Consider password_hash() instead.

---

    $userAddQuery= [
        'index' => 'system',
        'type' => 'users',
        'body' => [
            'username' => $username,
            'password' => $hashed_password,

In your document, you should call the field passwordhash or similar to be clear about exactly what is stored there.

---

    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;
        }

Should try block only wrap index() call? You earlier code shows no indication that auditLog() can throw, so there really is no reason for that to be included in the try block if that is the case.

---

    } catch(Exception $e) {

You should understand all the exceptions that could be thrown by index() call and be specific about type of exceptions your code is expected to handle.

After catching, do you really want to return the same response to the caller that is used for other failure modes for this method? The caller to this method would have no idea (at runtime) what the underlying failure is and whether it might be able to recover in some manner from the failure. It just gets -1.