Trying to access array offset on value of type bool | Upload file/image

@gillesdeb

Which line is the exception pointing to? That's the most important information.

Change this line

$request->file('avatar')->isValid(){(

to

if ($request->file('avatar')->isValid()) {(

Also you can remove the parenthesis within the block.

Lastly I would move the validation sooner in the script.

Is the avatar file always required to be sent by the user?

Sorry @laracoft : It's pointing to my first validation field: 'firstname' => ['required', 'string', 'min:2', 'max:50'],

Is this because i want to validate all fields at once?

Hmm, following Rodrigo's advice i wrapped the 'isValid'-request in an if-statement. solved the issue. ^^

Not really sure why it solved that specific issue, but hey it's already hard enough as it is :-p

Thank you for your help @rodrigo.pedra!

Changing it to an 'if' statement got me to the next step, thank you!

What do you mean with this?

Also you can remove the parenthesis within the block.

You mean there is no need for '{ ( ) }' ?

Lastly I would move the validation sooner in the script.

Would you be so kind to explain as why this is recommended?

Is the avatar file always required to be sent by the user?

Currently this field is 'nullable' but plan to give it a default value.

Once the user is registered, I want to force them to make a decent profile hence why the avatar is required.

My edit blade will show something like this

value="{{ (old('avatar')) ?: (ucfirst(Auth::user()->avatar)) }}"

Sorry for all these additional questions, this is the last one 😇

I'm specifying to storeAs('/images.avatars')

This worked but went to "storage>images.avatars>private>var>.....

This did not really go according to plan :-D In storage.php I can't really see what the path is set to, should i specify it in here?

I would do these modifications:

public function update(Request $request)
{
    $providers = ['facebook', 'twitter', 'instagram', 'linkedin', 'skype', 'youtube'];

    foreach ($providers as $provider) {
        $url = $request->input($provider);
        if (! preg_match("~^(?:f|ht)tps?://~i", $url)) {
            $url = "https://" . $url;
        }
        $request->merge([$provider => $url]);
    }

    $request->validate([
        'firstname' => ['required', 'string', 'min:2', 'max:50'],
        'lastname' => ['required', 'string', 'min:2', 'max:80'],
        'bio' => ['required', 'max:255'],
        'function' => ['required'],
        'mobile' => ['required', 'string', 'alpha_num', 'min:9', 'max:20'],
        'avatar' => ['nullable', 'image', 'mimes:jpeg,png,jpg,gif,svg', 'max:2048'], //max 2MB
        'facebook' => ['required', 'active_url'],
        'twitter' => ['required', 'active_url'],
        'instagram' => ['required', 'active_url'],
        'linkedin' => ['required', 'active_url'],
        'skype' => ['required', 'active_url'],
        'youtube' => ['required', 'active_url'],
    ]);

    //fetch user
    $user = Auth::user();

    //Handle avatar
    //Check if request has image
    if ($request->hasFile('avatar') && $request->file('avatar')->isValid()) {
        $file = $request->file('avatar');

        //Fetch file name
        $filename = $user->getKey() . '.' . $file->extension();

        $file->storeAs('/images.avatars', $filename);

        // Only update avatar URL when there is a file present
        $user->avatar = Storage::url($filename);
    }

    //update fields
    $user->firstname = $request->input('firstname');
    $user->lastname = $request->input('lastname');
    $user->bio = $request->input('bio');
    $user->function = $request->input('function');
    $user->mobile = $request->input('mobile');
    $user->facebook = $request->input('facebook');
    $user->twitter = $request->input('twitter');
    $user->instagram = $request->input('instagram');
    $user->linkedin = $request->input('linkedin');
    $user->skype = $request->input('skype');
    $user->youtube = $request->input('youtube');

    $user->save();

    return redirect()->route('useraccount.show');
}

Highlights:

• Refactored the social media's urls checks into a foreach
• As you are already injecting the Request object in the controller method, I avoided using the request(...) helper
• Moved validation just after the merges, with a rule for considering the avatar nullable
• Tweaked the avatar file handling. Here I use the user's id as a filename to avoid filename collisions among users.

Hope some make sense.

One additional question:

A user is required to provide a profile for all these social media: 'facebook', 'twitter', 'instagram', 'linkedin', 'skype', 'youtube'?

Currently they are all required fields.

What if a user doesn't have a profile in one of these?

For example I don't have a profile in at least four of those.

Meu Deus! @rodrigo.pedra that looks so clean !

"Refactored the social media's urls checks into a foreach"

Like a boss! I tried to simplify this over the weekend (not shown above / I wasn't impressed by it, let alone that I would keep it), but honestly your solution is - clean - easy Why couldn't I think of doing it like that 😅

"Validation moved up a bit"

Ok, so you recommend to validate all fields of input before handling anything else regarding the file/image. I just thought I NEEDED to check if the request does have an image and if it was a valid image before trying to validate.

"Tweaked the avatar file handling. Here I use the user's id as a filename to avoid filename collisions among users."

If you put it like that, yeah makes sense and looks cleaner. Just to be sure I can still concatenate whatever keywords I want inbetween the request and variable, right? But your approach to make it unique by using ID is actually simple but i didn't think of it :-D

"As you are already injecting the Request object in the controller method, I avoided using the request(...) helper"

I don't understand this tbh, I see that you use like this :

$request->input($provider)
$request->input('firstname')

...

Isn't the helper f.ex. request('firstname') made for situations like these?

"What if a user doesn't have a profile in one of these?"

This is just a template that I'm playing with while learning. I followed a lot of tutorials to make an application/project (not in laravel though) but the best way of learning for me is to just go at it instead of following someone blindly -> not thinking about the actual steps I take/took.

As I was playing with validation, all fields became 'required'. When I'm happy with all the steps in my controller I plan to change this :

No longer having 'required' in validation for most fields. Change this is my html as well and use 'if' statement to show the user provided data for it. (Assuming this will be quite straightforward)

Sorry if I'm bombarding you with questions and text...

1 - request(...) helper

Every time you call the request(...) helper, Laravel first look for the request in the Container ( a giant array with all the classes the container knows about) and then call $request->__get('variable') on the "resolved" request object.

Although it caches the request instance the first time it is created, you are already asking Laravel to look for it in the container on the controller's action signature:

public function update(Request $request) {
// ...

So there is no need to ask the container every time for the same object you already have.

Also $request->input(...) and $request->__get(...) executes basically the same code.

But of course, if you find it easier you can keep request('variable'), at the end of the day the best code is the one you find easier to maintain (when squeezing every performance bit from your app is not a must, which in 95+% of cases it isn't).

2 - Blank provider's URLs

If you want to allow blank provider URLs I guess you would need:

2.1 - Change the merge validation, to allow for blank values:

foreach ($providers as $provider) {
    $url = $request->input($provider);

    if (blank($url)) {
        continue; // skip blank URLs
    }

    if (! preg_match("~^(?:f|ht)tps?://~i", $url)) {
        $url = "https://" . $url;
    }
    $request->merge([$provider => $url]);
}

2.2 - Allow nullable values when validating

    $request->validate([
        'firstname' => ['required', 'string', 'min:2', 'max:50'],
        'lastname' => ['required', 'string', 'min:2', 'max:80'],
        'bio' => ['required', 'max:255'],
        'function' => ['required'],
        'mobile' => ['required', 'string', 'alpha_num', 'min:9', 'max:20'],
        'avatar' => ['nullable', 'image', 'mimes:jpeg,png,jpg,gif,svg', 'max:2048'], //max 2MB
        'facebook' => ['nullable', 'active_url'],
        'twitter' => ['nullable', 'active_url'],
        'instagram' => ['nullable', 'active_url'],
        'linkedin' => ['nullable', 'active_url'],
        'skype' => ['nullable', 'active_url'],
        'youtube' => ['nullable', 'active_url'],
    ]);

3 - Image validation

As we now have a nullable validation rule on the avatar field, that would be sufficient to avoid errors when an image is not provided.

The problem wasn't adding a validation only when an image was sent, problem was not running all the other validation rules when an image was not sent.

As the validation was inside an if block, you could end up with bogus data if a user did not send an image.

Hope it is more clear.

@rodrigo.pedra

1. Very clear, thank you! I'll keep this in mind, performance is always important (but I understand only really comes to play when we are talking about huge databases).

2.1 You already solved an issue I would have had (and avoided me wasting to much time), by pointing out that I need to consider the URL providers. I would not have thought about that but seeing it, makes sense => otherwise i end up with just https://

2.2 Feels good that there is something I knew. Even if it is a little win, I'll take it :-)

1. When you point it out like that, it makes A LOT of sense. When I wrote the function (well also scripts in general) I fail to think with that logic. Hopefully that will come as I progress and stress less about 'problems' due to ; ( { [

 One last, well 2 last questions, promised!

In the folder structure it shows 'storage>app>images.avatars>private/var/tmp>uploaded_avatar_with_user_id'

This is a bit different than what I expected it to be (storage>app>images>avatars>uploaded_avatar_with_user_id).

Why is that?

When I want to show this data, I assume I'll have to specify to fetch the data from Images.avatars right?

Like now I'm trying to fetch the data as follows:

Auth::user()->avatar

In my html it shows that it is trying to retrieve it from storage but not the specified folder :

<img src="/storage/1.jpeg" class="rounded-circle avatar-lg img-thumbnail" alt="profile-image">

In my database it shows as :

/storage/1.jpeg

However looking at the complete path above, this is not really correct.

Is it just a matter of specifying where to fetch the avatar?
I can't command click 'storeAs' to see where it comes from, storage.php doesn't look like the right place neither. Or am I once again, complicating something simple?

So a stock Laravel installation has some filesystems "disks" configured.

Out-of-the-box the default "disk" is the local one. Which is not the one you get symlinked when you run:

php artisan storage:link

The one which gets symlinked is the public one. If you didn't run the command above yet, do it. Also don't forget to run it in the first time you deploy your application onto a new server, such as your production server.

Try changing the file storage part to this:

    if ($request->hasFile('avatar') && $request->file('avatar')->isValid()) {
        $file = $request->file('avatar');

        $filename = $user->getKey() . '.' . $file->extension();

        // removed the forward slash (it is optional), and added the disk option
        $path = $file->storeAs('images.avatars', $filename, ['disk' => 'public']);

        // added the `::disk(...)` call, and generate the URL 
        // from the path returned in the line above
        $user->avatar = Storage::disk('public')->url($path);
    }

The ->url(...) method generates a URL from a saved path. That is what was missing in the last code sample.

@rodrigo.pedra Thank you so much for taking the time to give such valuable and clear feedback! Really appreciated!

After browsing the internet there is a lot of discussion (it's the internet after all) regarding the use of public folder and storing data/files/images in that directory. As it is accessible to "everyone". (" " around everyone as I feel that is a bit exaggerated)

It's okay to store like avatars, company logos, product pictures, etc.. things like that (uploaded by users and admin) in a public folder, right?

But more important files such as contracts, invoices, etc.. would be foolish to give public visibility.

If that is indeed as I should interpret / find consensus in the opinions I've read online, wouldn't it be just easier to create a disk with private visibility for everything?

Why still use a public folder if it's insecure / accessible to "everyone"?

After doing a bit more research I found out that the disks can be specified in the "filesystems.php" (instead of where i looked first : "storage.php").

So it's just a matter of configure a disk

'disks' => [

    'private' => [
        	'driver' => 'local',
        	'root' => storage_path('app/private'),
	 'url' => env('APP_URL').'/storage'
	'visibility' => 'private',
    ],

Change controller as follows:

// removed the forward slash (it is optional), and added the disk option
    $path = $file->storeAs('images.avatars', $filename, ['disk' => 'private']);

    // added the `::disk(...)` call, and generate the URL 
    // from the path returned in the line above
    $user->avatar = Storage::disk('private')->url($path);
}

php artisan config:clear

I tried the above but notice that my new uploaded avatar (in private disk) in not shown.

Does that mean my understanding described above is wrong? 🥺

*Also, wouldn't you recommend hashing $filename? As If you inspect the element it, the user_id is obvious so users could just change the number to show other users avatar, etc

The visibility parameter in a filesystem definition is a bit misleading, it works better for cloud-based storage drivers, such as AWS S3, but for local drivers it changes the file permissions, as you can see here:

https://github.com/thephpleague/flysystem/blob/2062a9460fc9bb9d05ce0b215176be378f29e372/src/Adapter/Local.php#L364-L375

(Laravel uses PHP League's Flysystem package to handle file storage)

So changing this parameter wouldn't make a change on generating URLs.

I agree that some files are needed to be behind some sort of authentication. I wouldn't mind storing user avatars in a public-accessible folder, only if this would be a highly sensitive information, but invoices, contracts, and similar should be guarded against unauthorized access.

What I do in those cases is:

1. Save the file to a folder outside the ./storage/public folder
2. Create a controller that allows logged in users to download that file
3. Save the URL to that controller action, or the parameters to generate this URL

I will outline these steps, a bit out of order than above, as above they make sense to draw a big picture of the process, but the implementation is better explained in a different order.

Creating a controller to download the file.

The most straightforward solution is to use Laravel filesystem abilities to make a download response:

<?php

namespace App\Http\Controllers;

use Illuminate\Http\Request;
use Illuminate\Support\Facades\Storage;

class DownloadController extends Controller
{
    public function __construct()
    {
        // guard against non-logged in users
        $this->middleware('auth:web');
    }

    public function show(Request $request)
    {
        $disk = Storage::disk('local');
        $path = $request->query('path');

        if (! $path || ! $disk->exists($path)) {
            return abort(404);
        }

        return $disk->download($path);
    }
}

If the show method is registered to a route with /download, one could download a file by navigating to

http://localhost/download?path=my-file.jpg

The disadvantage of doing this is that a logged in user could start trying any filename he/she wants. But as you have a logged in user you could do additional checks before returning the download response.

Another approach would be creating a controller for specific file types and retrieve them by passing a parameter.

So for example, for users' avatars:

<?php

namespace App\Http\Controllers;

use App\Models\User;
use Illuminate\Support\Facades\Storage;

class UserAvatarController extends Controller
{
    public function __construct()
    {
        // guard against non-logged in users
        $this->middleware('auth:web');
    }

    // For example https://localhost/avatar/1
    // where 1 is a user ID
    public function show(User $user)
    {
        $disk = Storage::disk('local');
        $path = $user->avatar;

        if (! $path || ! $disk->exists($path)) {
            return abort(404);
        }

        return $disk->download($path);
    }
}

In this case instead of saving the avatar URL in the User model, we would save the path where the user's avatar was saved to.

Let's assume this controller method is registered as a route like this:

Route::get('/avatar/{user}', [\App\Http\Controllers\UserAvatarController::class, 'show']);

The in you template you could reference it like this:

<img src="/avatar/{{ $user->id }}" alt="User avatar" />

This still gives a logged in user a chance of accessing all stored avatars by changing the user id in the URL, but exposes just one type of file.

If you have very strict file access policies, you can consider using signed URLs, for avatars it would be an overkill, but maybe for contracts or other more sensitive files it could be a solution to making it more strict.

You can read more about signed URLs here:

https://laravel.com/docs/8.x/urls#signed-urls

Storing the file ans saving the path or URL

As now we are using another filesystem not automatically exposed to public access, the way the avatar is stored needs some tweaks.

First thing is to use the local disk when storing files:

$path = $file->storeAs('images.avatars', $filename, ['disk' => 'local']);

As the local filesystem is usually the default one, we can omit the options array:

$path = $file->storeAs('images.avatars', $filename);

Now that we have the path where the file is stored, we can decide on saving either the URL to download it (if you chose the first controller option) or just the path (if you chose the second controller option.

Saving the path is pretty straightforward:

$user->avatar = $path;

Then, you would access this avatar image using either the user's id (if you chose the second controller option) or generating the URL in the fly (if you chose the first controller option:

<img src="/avatar/{{ $user->id }}" alt="User avatar" />

or

<img src="/download?{{ http_build_query(['path' => $user->avatar]) }}" alt="User avatar" />

The http_build_query(...) PHP function helps ensuring the path is encoded properly for a URL.

If you prefer, you could save the URL upfront:

$user->avatar = url('/download') . '?' . http_build_query(['path' => $path]);

The disadvantage here is if you need to change your app's domain, or the controller path. Then you would need to parse all URLs to update each of the in your database.

Hope it helps.

Unfortunately, at the moment it's a bit much for me to process so "fast" (=normal, nothing to do with the explanation), but I do think that I understand the general idea behind it. And I just need to reread your explanation a few times, combined with a bit of additional research and ofc playing around with it :-)

Muito obrigado, @rodrigo.pedra !

Não há de que, @gillesdeb !

Feel free to ask additional questions you might have, I can take some time to respond, as I am not able to spend time here everyday, but I also keep an eye on recent threads.

Have a nice day =)