Laravel controller method to update user

Question

I'm using Laravel and this PHP method is in a controller. It's the back-end for a edit profile page. The user can change the profile picture, reset the password, the username, and so on. I'm new in PHP and would like to know if there is a better or simpler way to code this.

public function update(Request $request){

        $user = Auth::user();

        $username = $request->username;
        // PENDING $email = $request->email; 
        $wallet_address = $request->wallet_address;
        $about = $request->about;
        $password = $request->password;
        $website = $request->website;
        $location = $request->location;

        if($username){ 
            $user->username = $username; 
            $user->save();
        }

        /** upload avatar section */
        
        $image = $request->file('avatar');

        if($image){

            // note: if avatar_name variable is null it means the user has never uploaded an image..
            // if the user has uploaded an avatar before, delete it before uploading the new one..
            if($user->avatar_name){
                Storage::delete('/public/profile-pics/'.$user->avatar_name);
            }

            $image_new_name = date('dmy_H_s_i').'_'.$user->id.'_'.$image->getClientOriginalName();
            $image->storeAs('profile-pics',$image_new_name,'public');
            $user->avatar_url = 'storage/profile-pics/'.$image_new_name;
            $user->avatar_name = $image_new_name;
            $user->save();
        }

        if($wallet_address){
             $user->wallet_address = $wallet_address;
             $user->save(); 
        }

        if($about){
             $user->about = $about; 
             $user->save();
        }

        if($password){
             $user->password = $password; 
             $user->save();
        }

        if($website){
            $user->website = $website; 
            $user->save();
        }

        if($location){
             $user->location = $location; 
             $user->save();
        }

        return redirect()->route('edit_profile')->with('success','Your account has been updated successfully.');
    }

Answer 1

Redundant (and likely inefficient) code

There are multiple blocks that repeat:

    if($username){ 
        $user->username = $username; 
        $user->save();
    }

    if($wallet_address){
         $user->wallet_address = $wallet_address;
         $user->save(); 
    }

    if($about){
         $user->about = $about; 
         $user->save();
    }

    if($password){
         $user->password = $password; 
         $user->save();
    }

    if($website){
        $user->website = $website; 
        $user->save();
    }

    if($location){
         $user->location = $location; 
         $user->save();
    }

The only thing that really changes here is the property/field being checked. That could be simplified using an array containing the field names:

//could be moved to the model or another namespace
const FIELDS = ['username', 'wallet_address', 'about', 'password', 'website', 'location'];
public function update(Request $request){
    $user = Auth::user();
    foreach (self::FIELDS as $field) {
        if ($request->$field) {
            $user->$field = $request->$field;
            $user->save();
        }
    }
    //handle avatar image update
}

Obviously that shows that the save() method is called multiple times, which could be inefficient if it has to write data (e.g. to a database or other data store). You could conditionally call the save method if one of those fields has a value and is updated on the user. The save method is also called when an image is included in the request (i.e. when $request->file('avatar') has a non-null value). That boolean could be combined with that condition as well, which would lead to a single point of updating the user model.

Validation of the Request

You could create a Request subclass to validate the fields that should be (sometimes) present on the request

For example, you could run

php artisan make:request StoreUserRequest

And that would create StoreUserRequest in the app/Http/Requests directory which would extend Illuminate\Foundation\Http\FormRequest. The boilerplate would give it an empty rules() method that could contain the fields that can be passed. For Example:

public function rules()
{
    return [
        'username' => 'sometimes',
        'wallet_address' => 'sometimes',
        'about' => 'sometimes',
        'password' => 'sometimes',
        'location' => 'sometimes',
    ];
}

And those can container other rules - for example, if a field is required, has a maximum string length, etc.

Then in the controller method use that new request subclass as the type for $request.

Answer 2

So the above answer from Sᴀᴍ Onᴇᴌᴀ already covers quite a few points. I've worked with Laravel for the past 4 years, so I'll go a bit further in depth on how I'd approach this, including a few suggestions.

First you'll want to use RequestValidation as Sᴀᴍ Onᴇᴌᴀ indicated, this will help guard against invalid input. And make use of Laravel's own input validation logic (sends the user back to the form with error messages in your message bag)

Secondly you have a lot of logic on the User model related to an avatar. I'd suggest creating an Image / Avatar Model that handles the image part of it all, in the end you'd end up with an image_id/avatar_id on the User model as a relation. That way you can have all the logic that handles a file on your filesystem and the associated data. It also enables you (if you go for image) to reuse the logic in a different context. And declutters your user model

Thirdly, as Sᴀᴍ Onᴇᴌᴀ indicated you do a lot of ->save() calls. Each call is a database round trip. And a costly action. You can set each property seperately if you want and at the end of your logic you can call ->isDirty() on your user model, if it returns true, data has changed from the database returned data, indicating an update should be done. At this point you could save.

I however would suggest building an update array of the data to update, and calling the ->update() method with an array containing the key => value pairs that you wish to update. You can check this data array for emptyness to avoid unneccessary updates.

I would always suggest having some form of validation when changing passwords, to make sure an unauthorized user can't simply change the password. (a simple enter current password would do here)

I'll refactor the code you had and try to leave it inside the existing method.

public function update(Request $request)
{
    $user = Auth::user();
    // Read up on the Laravel Request Validation for available rules and usages
    $request->validate([
        'username'         => 'sometimes',
        'email'            => 'email|sometimes',
        'wallet_address'   => 'sometimes',
        'about'            => 'sometimes',
        'password'         => 'sometimes',
        'current_password' => 'required_with:password',
        'website'          => 'sometimes',
        'location'         => 'sometimes',
    ]);
    // $request->input() will return null if the given key is not found
    // I use an array_filter here to filter out empty results, if you expect falsey values (like null to unset something, or 0/false/'') you should use a different approach
    $update_data = array_filter([
        'username'       => $request->input('username'),
        'email'          => $request->input('email'),
        'wallet_address' => $request->input('wallet_address'),
        'about'          => $request->input('about'),
        'password'       => $request->input('password'),
        'website'        => $request->input('website'),
        'location'       => $request->input('location'),
    ]);
    if ($request->input('password')) {
        // enter logic here to validate existing password
        // if this logic fails, you should just return instead of saving anything
    }
    if (!empty($update_data)) {
        // You can store the result to see if you successfully updated
        $update_status = $user->update($update_data);
    }
    // Check if an file has been given
    if ($request->hasFile('avatar')) {
        // I'd suggest moving the image handling to a seperate method at least.
        $this->handleImage($user, $request->file('avatar'));
    }
    if (!$update_status) {
        return redirect()->route('edit_profile')->with('failure','Your account could not be updated.')
    }
    return redirect()->route('edit_profile')->with('success','Your account has been updated successfully.')
}

private function handleImage(User $user, UploadedFile $image)
{
    // if the image is a seperate relation, you could delete the existing relation
    // on the delete for the relation you could hook in a cleanup for the file.
    if ($user->avatar_name) {
        // You can add a "disk" to your filesystem configuration where you can set the default path.
        Storage::delete('/public/profile-pics/' . $user->avatar_name);
    }
    $image_new_name = date('dmy_H_s_i') . '_' . $user->id . '_' . $image->getClientOriginalName();
    $image->storeAs('profile-pics', $image_new_name, 'public');
    // if you use a properly configured Storage::disk you can only store the name, and the disk will resolve where to look for the file
    // Storing paths in the database binds your database to your filesystem structure making future changes more difficult to handle (think moving images to a seperate location, or a different server)
    // Another point, if you expect many users, consider using a subdivision in your path (something like the first few digits of the user id or anything that'll let you break up the files into subdirectories), most systems do weird things once you deal with 200k+ files in a single directory.
    $user->avatar_url = 'storage/profile-pics/' . $image_new_name;
    $user->avatar_name = $image_new_name;
    $user->save();
}