plank / laravel-mediable

Laravel-Mediable is a package for easily uploading and attaching media files to models with Laravel
MIT License
765 stars 101 forks source link

TypeError "App\Models\User::Plank\Mediable\{closure}(): Argument #2 ($media) must be of type Plank\Mediable\Media, array given" #268

Closed SimondeJong-VinvuSoftware closed 2 years ago

SimondeJong-VinvuSoftware commented 2 years ago

Dear,

I am having an issue with the retrieval of media. I was originally using version 4.4.2. However after discovering my issue I have updated to version 5.2.1. My issue is present in both versions.

When calling firstMedia($tags, true) I get a TypeError:

TypeError App\Models\User::Plank\Mediable\{closure}(): Argument #2 ($media) must be of type Plank\Mediable\Media, array given, called in ...

I am saving the media as follows:

    public function setProfileImage(UploadedFile $image)
    {
        config(['mediable.model' => Media::class]);
        $media = MediaUploader::fromSource($image->getRealPath())
            ->toDisk('avatars')
            ->toDirectory("profile/")
            ->useFilename('profile_' . $this->id)
            ->onDuplicateUpdate()
            ->upload();
        $this->syncMedia($media, ['user', 'profile']);

        return $media;
    }

I am retrieving the profile image as follows:

 public function getProfileImage()
    {
        config(['mediable.model' => Media::class]);
        $image = $this->firstMedia(['user', 'profile'], true);
        return $image;
    }

I have also tried the getMedia and getMediaMatchAll methods. Both give the same TypeError. I am using Laravel in combination with Tailwind, Alpine and Livewire. I am getting the error when when my livewire components are updated, for example when switching a tab on the page.

Does anybody know where I could be making an error causing this issue?

Thank you in advance for your help!

frasmage commented 2 years ago

Hi there, can you provide the stack trace that you truncated (at least as deep as calls within the package)? Would help identify which code path you are hitting

SimondeJong-VinvuSoftware commented 2 years ago

Hi @frasmage,

Thanks for your help.

After calling $this->firstMedia(['user', 'profile'], true)from the getProfileImage method described earlier I am hitting the following method calls:

public function firstMedia($tags, bool $matchAll = false): ?Media
{
    return $this->getMedia($tags, $matchAll)->first();
}

public function getMedia($tags, bool $matchAll = false): Collection
{
    if ($matchAll) {
         return $this->getMediaMatchAll($tags);
    }

    ...
}

public function getMediaMatchAll(array $tags): Collection
{
    $this->rehydrateMediaIfNecessary($tags);

    //group all tags for each media
    $modelTags = $this->media->reduce(function ($carry, Media $media) {
         $carry[$media->getKey()][] = $media->pivot->tag;

          return $carry;
      }, []);

    ....
}

It fails on $this->media->reduce( ... ) on line 312 of Mediable.php.

This issue only seems to arise after interacting with Livewire. Therefore I believe the issue might be due to some conflicts (type casting?) with Livewire and not necessarily due to your package. Has anybody else had issues with Livewire? If so, do you happen to know what was causing this issue.

Thanks again for your help!

frasmage commented 2 years ago

Very weird. I've never worked with livewire, but I don't understand why the media relationship would contain anything other than Media records. Are you able to use breakpoints or dd() to see what is actually in the $this->media relation?

SimondeJong-VinvuSoftware commented 2 years ago

I edited the getMediaMatchAll method as follows in order to dump $this->media whenever the issue arises:

 public function getMediaMatchAll(array $tags): Collection
    {
        $this->rehydrateMediaIfNecessary($tags);

        try {
            //group all tags for each media
            $modelTags = $this->media->reduce(function ($carry, Media $media) {
                $carry[$media->getKey()][] = $media->pivot->tag;

                return $carry;
            }, []);

            //exclude media not matching all tags
            return $this->media->filter(function (Media $media) use ($tags, $modelTags) {
                return count(array_intersect($tags, $modelTags[$media->getKey()])) === count($tags);
            })->keyBy(function (Media $media) {
                return $media->getKey();
            })->values();
        } catch (\TypeError $exception) {
            dd($this->media);
        }
    }

This dumped the following: image

It seems this->media is somehow being converted to a collection of arrays instead of Media objects.

Thanks again for all the help!

SimondeJong-VinvuSoftware commented 2 years ago

Dear @frasmage,

I did a bit of testing myself and there does seem to be some type conversion going on. I have changed getMediaMatchAll from this:

    public function getMediaMatchAll(array $tags): Collection
    {
        $this->rehydrateMediaIfNecessary($tags);

        $modelTags = $this->media->reduce(function ($carry, Media $media) {
            $carry[$media->getKey()][] = $media->pivot->tag;

            return $carry;
        }, []);

        //exclude media not matching all tags
        return $$this->media->filter(function (Media $media) use ($tags, $modelTags) {
            return count(array_intersect($tags, $modelTags[$media->getKey()])) === count($tags);
        })->keyBy(function (Media $media) {
            return $media->getKey();
        })->values();
    }

To this (changed $this->media to $this->media->get() ):

    public function getMediaMatchAll(array $tags): Collection
    {
        $this->rehydrateMediaIfNecessary($tags);

        $modelTags = $this->media()->get()->reduce(function ($carry, Media $media) {
            $carry[$media->getKey()][] = $media->pivot->tag;

            return $carry;
        }, []);

        //exclude media not matching all tags
        return $this->media()->get()->filter(function (Media $media) use ($tags, $modelTags) {
            return count(array_intersect($tags, $modelTags[$media->getKey()])) === count($tags);
        })->keyBy(function (Media $media) {
            return $media->getKey();
        })->values();
    }

After this change everything works as expected. To be honest I do not know why this solves the issue, but it does seem to prevent the type casting from happening.

Maybe you do have an idea why this seems to solve the issue?

Thanks again for all your help!

frasmage commented 2 years ago

So $this->media()->get() will result in reloading the relationship from the database on every media lookup of every model, which will be very inefficient.

I have no idea why your collection of entities is flattened to arrays. When you are calling the profileImage method, how are you loading manipulating the Mediable entity before you make that call? What kind of eager loading are you doing?

SimondeJong-VinvuSoftware commented 2 years ago

I am not doing any eager loading. I have tried eager loading but this does not seem to help.

I am calling getProfileImage from getProfileImageUrl:

    public function getProfileImageUrl()
    {
        $opt = optional($this->getProfileImage());
        return $opt->getUrl();
    }

getProfileImageUrl is called directly from the views / livewire components everywhere we want to display a user profile image by setting the image src to the return of getProfileImageUrl. For example as follows:

 <img class="rounded-full w-10 h-10 object-cover"
           src="{{$selected->getProfileImageUrl() ?? asset('images/holder/profile.jpg')}}"
           alt="user image" />

The users are loaded as follows:

        $search = '%' . $this->search . '%';
        $users = User::isNotAdmin()
            ->inCompanies([Tenancy::getTenant()->id])
            ->where('name', 'like', $search)->get();

After the loading of the users the users are not manipulated appart from ordering within the users collection or adding or removing the user to a second MediableCollection ($selectedUsers). The users are never removed from the original collection. The ordering of the users is achieved as follows:

        foreach ($this->selectedUsers as $key => $value) {
            array_push($this->sort, $value->id);
        };

        $users = $this->queryUsers;

        $diff = $users->diff($this->project->users);

        foreach ($diff as $key => $value) {
            array_push($this->sort, $value->id);
        }

        $sortArr = $this->sort;
        $this->sort = array_unique($this->sort);

        $this->users = $users->sortBy(function ($selected) use ($sortArr) {
            return array_search($selected->id, $sortArr);
        });

        if (count($this->selectedUsers) == 0) {
            $this->sort = [];
            $this->users->sortBy('name');
        }
SimondeJong-VinvuSoftware commented 2 years ago

Hi @frasmage,

I fixed this issue by lazy eager loading the media before calling firstMedia in getProfileImage as follows:

    public function getProfileImage()
    {
        config(['mediable.model' => Media::class]);
        $this->load('media');
        $image = $this->firstMedia(['user', 'profile'], true);
        return $image;
    }

This is not an ideal solution as this resuls in loading the media again everytime the getProfileImage is called. However if I eagerload before this point (for example as a default when the user is retreived) it will still often get converted to an array. I believe this issue is due to type conversions done by Livewire as I have had similar issues due to Livewire type conversions in the past.

Thank you once again for all your help!