addMediaFromDisk is too slow.

[themustafaomar]

I'm having trouble with addMediaFromDisk method when moving files from temp directory to the uploads directory using S3.

First, let me explain my implementation.

We're working on a mobile application where the files are uploaded to S3 directly, those files are stored in the temp directory and paths are given back.

When a user submit the form, the paths of the uploaded files are attached in the form, so we can move them from temp directory to the original working directory by Media Library.

Because addMediaFromDisk()->toMediaCollection() was too slow, I switched the implementation to a job, so the files are moved in the background.

/**
 * Create a new job instance.
 *
 * @return void
 */
public function __construct($model, $files, $collection = 'media')
{
    $this->model = $model;
    $this->files = $files;
    $this->collection = $collection;
}

/**
 * Execute the job.
 *
 * @return void
 */
public function handle()
{
    $files = new Collection($this->files ?: []);

    try {
        $files->each(fn ($file) => $this->add(
            $this->model, $file, $this->collection
        ));
    } catch (FileDoesNotExist $e) {
        report($e);

        return false;
    }
}

/**
 * Move media from temporary directory to the original directory
 * 
 * @var \Illuminate\Database\Eloquent\Model $model
 * @var string $filename
 * @var string $collection
 * @return void
 */
protected function add($model, string $filename, $collection)
{
    $model->addMediaFromDisk('temp/'.$filename, 's3')
        ->toMediaCollection($collection, 's3');
}

So what is the problem?

The problem with this approach that it takes a minimum of 1 minute!

Even though it takes 2 or 3 seconds when moving the file using:

Storage::disk('s3')->move('temp/filename.ext', 'uploads/filename.ext')

What I find out when I took a deep dive into the MediaLibrary source code that, it copies the file and then it deletes it, often this is cause of the time consumption.

So my question now, is there anyway to reduce the time of this process by moving the file instead of copying?
Is there any better approach?

Thank you!

[erikn69]

Storage::disk('s3')->move('temp/filename.ext', 'uploads/filename.ext')

On that example you only move a file on s3

First php upload file to server then move to s3, and s3 is a external server, it takes time to upload(more with heavy files),
Second Storage::copy comes from laravel wich uses thephpleague/flysystem, so the question must go for these packages

If you know a solution feel free to make a PR

[themustafaomar]

Thanks for your answer! but:

I'd like to make it clear that all I want to do here is moving files in S3 from a directory (temp) to another one using addMediaFromDisk.

There is no such a problem with uploading files to s3.

However, I'm kinda fixed my problem:

• Localhost is somehow doesn't work as it should be when calling S3, but the server took 18s to move 24MB which is almost good compared to 1+ minute, that is my bad I should've noticed that.

laravel-medialibrary/src/MediaCollections/Filesystem.php

Lines 60 to 76 in cad52fd

	$storage = Storage::disk($file->getDisk());
	$headers = $diskDriverName === 'local'
	? []
	: $this->getRemoteHeadersForFile(
	$file->getKey(),
	$media->getCustomHeaders(),
	$storage->mimeType($file->getKey())
	);
	$this->streamFileToDisk(
	$storage->getDriver()->readStream($file->getKey()),
	$destination,
	$media->disk,
	$headers
	);
	}

• Commenting this out and replace it with the following, took 4s:

$storage = Storage::disk($file->getDisk());

$storage->move($file->getKey(), $destination);

Anyway, sorry to bother you, and keep up the good work!

Thanks!

[comwes]

This is indeed a good solution for issue. Indeed if a user upload a large file like 100MB, it might be a problem if the server has also to bear the load of such file when the form is saved.

But the better way to apply this modification is definitely not to modify the FileSystem in the vendor folder. Nor to comment all the lines as suggested here. A better way is either to do a Pull request or extend the FileSystem class and use binding to use your newly created class. Such way, if you perform a composer update you wont loose your changes.

On my side what I did the following:

1. Create a FileSystem class that extends the original one with an override of the copyToMediaLibraryFromRemote method

use Illuminate\Support\Facades\Storage;
use Spatie\MediaLibrary\MediaCollections\Models\Media;
use Spatie\MediaLibrary\Support\RemoteFile;

class FileSystem extends \Spatie\MediaLibrary\MediaCollections\Filesystem
{
    public function copyToMediaLibraryFromRemote(RemoteFile $file, Media $media, ?string $type = null, ?string $targetFileName = null): void
    {
        $destinationFileName = $targetFileName ?: $file->getFilename();

        $destination = $this->getMediaDirectory($media, $type) . $destinationFileName;

        $diskDriverName = (in_array($type, ['conversions', 'responsiveImages']))
            ? $media->getConversionsDiskDriverName()
            : $media->getDiskDriverName();

        if ($this->shouldCopyFileOnDisk($file, $media, $diskDriverName)) {
            $this->copyFileOnDisk($file->getKey(), $destination, $media->disk);

            return;
        }

        $storage = Storage::disk($file->getDisk());
        // Change: Prevent using readStream when moving moving file if source and destination disk are identical.
        if ($file->getDisk() == $media->disk) {
            $storage->move($file->getKey(), $destination);
            return;
        }
        $headers = $diskDriverName === 'local'
            ? []
            : $this->getRemoteHeadersForFile(
                $file->getKey(),
                $media->getCustomHeaders(),
                $storage->mimeType($file->getKey())
            );

        $this->streamFileToDisk(
            $storage->getDriver()->readStream($file->getKey()),
            $destination,
            $media->disk,
            $headers
        );
    }

}

2. Add the following in AppServiceProvider to use my previously created class:

class AppServiceProvider extends ServiceProvider
{

    public $bindings = [
        \Spatie\MediaLibrary\MediaCollections\Filesystem::class => FileSystem::class,
    ];
...

I hope this will help.
If any one have time to implement in the original library, feel free to use. On my side I don't currently have time to do that.

KR,
Gerry N.

[themustafaomar]

[themustafaomar]

I wish I could vote someone's answer up, but actually the reply is not related to the issue at all, why should I mark a reply doesn't help anyone as answered????????

I didn't want to mark my reply as answered, but someone told me that you should do that, but of course I won't do that for a comment that doesn't answer my question.

[angeljqv]

First replacing code on vendor is a bad idea,
Second you did copy your same answer to another post as quote instead of mark the original discussion

[erikn69]

First replacing code on vendor is a bad idea

That is right

[themustafaomar]

I never update the vendor I know that of course, I've found a way.

Anyway, simply I shouldn't be do that, but I have no problem, happy now 😄

[themustafaomar]

Sorry but I didn't know if I voted you up, my reply will be there 🙏

And thanks for getting into this thread @angeljqv