Updating necessary data using just one function

Question

My objective is to update necessary data using one function instead of having different functions to update different fields. So, I've created one and I think this is not really elegant, efficient or secure. I would seriously like a review if possible.

Key.
 $data is an array.
 $data[0] = column to update.
 $data[1] = new data.
 $data[2] = username of the user.

Code:

public function handleUserDataUpdate($data) {
 if($userMapper->validate($data) === true) {
 $userMapper->update($data);
 } else {
 $errors['count'] = count($errors);
 return $errors;
 }
}

---

UserMapper Class Methods:

public function validate($data) {
 switch ($data[0]) {
 case 'rank':
 if(empty($data[1])) {
 $errors[] = "The rank field cannot be empty!";
 }
 if(count($errors) > 0) {
 return $errors;
 } else { 
 return true;
 }
 break;
 case 'display_name':
 if(empty($data[1]) || strlen($data[1]) < 3 || strlen($data[1] > 20)) {
 $errors[] = "The display name should contain at least 3 to 20 characters";
 }
 if(count($errors) > 0) {
 return $errors;
 } else {
 return true;
 }
 break;
 }
}
public function update($data) {
 $sql = "UPDATE users SET " . $data[0] . "=? WHERE username=?";
 $query = $this->db->prepare($this->sql);
 $query->bind_param('ss', $data[1], $data[2]);
 $query->execute();
 $query->close();
}

• Marco Acierno: i would use ? for data[0] too
• Abandoned Account: @MarcoAcierno what do you mean?
• Marco Acierno: Here $sql = "UPDATE users SET " . $data[0] . "=? WHERE username=?"; instead of write " . $data[0] . " you could use ?.
• CodeX: I agree Marco, this should work $sql = "UPDATE users SET ?=? WHERE username=?"; $query->bind_param('sss', $data[0], $data[1], $data[2]);
• BlueCacti: You use an if-statement to check for errors, then use a second to check if errors have been set. You could compress this to one if/else-statement checking for errors and immediatly returning either the error or (else) returning true.

Answer 1

1. Why update is a public function? I suppose it's inside a class, so it should be private. If you make it public all checks you do inside handleUserDataUpdate and validate are useless since someone could just use update($data) and put malicious data inside it. The same should be for validate, seems like the only function which should be public is handleUserDataUpdate. And you can be sure, as user of your API i'm more invited to use update than handleUserDataUpdate.

2. The name handleUserDataUpdate is a bit strange for a public function, i mean handleUserDataUpdate it could be OK for a private method since you can change it every time you want and you free to do what you want. But when it's a public method choose your names careful! updateUserData($data)? Or simply, if the class name is UserDataEditor you can use apply or commit() (If you know Android it could be something like SharedPreferences)

3. An array? Really, no! Why not a class? I mean, with an array you should remember:

   1. Index 0 is X
   2. Index 1 is Y
   3. Index 2 is Z

   No! Really, after a bit of time you will need to read the source to remember what was 0, 1 and 2.

   You can use a custom class like:

   class UpdateField {
    private $fieldName;
    private $newValue;
    private $userName;
   }
   

   I have the feeling like it could be a great system if:

   1. UpdateField was an interface, which RankUpdate, DisplayNameUpdate implement so inside the class you have the logic of the validate and the ValidatorSystem just execute the method UpdateField->update() which returns an array of errors or true in case of success.

   2. With this you could reuse UpdateField to update everything not just a userfield.

   but instead of use an array, you can improve it:

   1. You can check if fieldName is valid in the constructor of UpdateField and throw an exception if it's not a valid field.
   2. The method validate could be inside this class too, no?

4. This block of code

   if(count($errors > 0)) {
    return $errors;
   } else {
    return true;
   }
   

   ignoring the fact that i hate when a function first return an array or a boolean (i know, it's in the default PHP functions BUT IT'S STUPID! It let me feel like: You will never know what this function will return!) It could be improved in one line with:

   return count($errors) > 0 ? $errors : true;
   

   but wait, is if(count($errors > 0)) correct? It count the value of a boolean? It's what you want?

5. @Vogel612 already said about problems inside update functions.

About point 3, take for the moment only the fact that you should use a class instead of an array, about the system i'm thinking it's just an idea in my mind maybe it cannot be done in PHP.

• Abandoned Account: Love you marco! I want to understand interfaces. Why are they used? If you teach me that I will love you forever!!!
• Marco Acierno: Fixed, anyway maybe an interface could not be appropriate here... maybe an abstract class to reduce some duplicate code. If you want to learn interfaces i think it's better to you to read interface page of php. It's nothing of special.
• Abandoned Account: wtf, its a mistake. I'll fix my main code.

Answer 2

You are vulnerable to SQL Injections. Even when you are using Prepared statements, doing It wrong will kill you.

$sql = "UPDATE users SET " . $data[0] . "=? WHERE username=?";

This makes you exposed to SQL injections because your function is not private. You should not allow "user" access to "critical" functions. make that function private and you should be good to go.

As soon as the user is allowed to set $data[0] you're screwed.

• Abandoned Account: I stated it to you personally that the user is not allowed to set it
• PeeHaa: Not allowed how? By telling them to pretty please not use a public API?
• Madara's Ghost: That's not how it works, public vs private has nothing to do with the SQL injection he has.
• Vogel612: @MadaraUchiha in fact it does. If you can ensure that there is no SQL-Critical stuff in $data[0] it should be fine. But as the function is public, you can't ensure that and are thus vulnerable to sql injection