Subfolder creation possible bug

[alexglue]

This one: vendor/spatie/laravel-medialibrary/src/Filesystem.php:116

        $directory = $media->id;
        $this->filesystems->disk($media->disk)->makeDirectory($directory);

Creates another NEW directory named the same as media ID for EVERY uploaded file. Is it a feature or a bug?

The path SHOULD NOT contains media_id! what if i want to reinsert this records? All links WILL BE broken cause of inconsistent auto_increment sequence. You already have file_name, and you could append it with subdirectory if u want. No need to recreate file path by id+filename concatenation, you can just get $this->filen_name after that.

[alexglue]

also,it's dangerouse to generate directory name for storing and getUrl() separately:

//vendor/spatie/laravel-medialibrary/src/UrlGenerator/BaseUrlGenerator.php:64
    public function getPathRelativeToRoot()
    {
        $path = $this->media->id;

[alexglue]

argh, i'm sorry for bothering you, but

    public function getMediaDirectory(Media $media)
    {
        $this->filesystems->disk($media->disk)->put('.gitignore', Gitignore::getContents());

this is extremely bad idea.

What if i use SUBVERSION? What if i don't use LOCAL storage? Damn, this is not SOLID! Why some getter should CREATE .gitignore file???

[freekmurze]

Hi, thank you for your interest in our package.

Here are some answers to your questions:

• I've explained the reasoning for creating a folder per file in #89.
• I don't see why it would be dangerous to generate directory name for storing and getUrl() separately. Can you elaborate on that?
• I don't think it's bad to automatically put a .gitignore in the directory. The idea behind this package is that you shouldn't mind what the contents of that directory is. If you' re going to store media on a disk, surely there's room for one .gitignore file. It think for the majority of users this is ok and helpful. You're quite right however that for Subversion users are being left out in the cold. I'll add an instruction in the readme so that they are aware they should ignore that directory themselves.

We create the packages for our own projects and try to make them extendable and general as possible so other users can enjoy them too. You should be aware that this package, along with the others ones we make, is quite opinionated. If you feel that we are doing dangerous things, feel free to fork our code.

[alexglue]

@freekmurze You WILL have a perfomance troubles on highload and large count of files. You could read this stackoverflow, etc for more info: http://stackoverflow.com/questions/19212766/user-uploads-folder-structure

Just FYI: this is how I solved this:

//vendor/spatie/laravel-medialibrary/src/Filesystem.php:111
    public function getMediaDirectory(Media $media)
    {
        $directory = $this->getMediaDirectoryName($media);
        $this->filesystems->disk($media->disk)->makeDirectory($directory);

        return $directory;
    }

    public function getMediaDirectoryName(Media $media)
    {
        $filename = pathinfo($media->file_name, PATHINFO_FILENAME);

        return implode(DIRECTORY_SEPARATOR, array_slice(str_split($filename, 3), 0, 3)) . DIRECTORY_SEPARATOR . $media->id;
    }

//vendor/spatie/laravel-medialibrary/src/FileAdder/FileAdder.php:100
    public function setFile($file)
    {
        $this->file = $file;

        if (is_string($file)) {
            $this->pathToFile = $file;
            $this->fileName   = md5($file) . pathinfo($file, PATHINFO_EXTENSION);
            $this->mediaName  = basename($file);

            return $this;
        }

        if ($file instanceof UploadedFile) {
            $this->pathToFile = $file->getPath().'/'.$file->getFilename();
            $this->fileName   = md5($file->getClientOriginalName()) . '.' . $file->getClientOriginalExtension();
            $this->mediaName  = $file->getClientOriginalName();

            return $this;
        }

        throw new FileCannotBeImported('Only strings and UploadedFileObjects can be imported');
    }

//vendor/spatie/laravel-medialibrary/src/UrlGenerator/BaseUrlGenerator.php:63
    public function getPathRelativeToRoot()
    {
        $path = app(Filesystem::class)->getMediaDirectoryName($this->media);

        if (is_null($this->conversion)) {
            return $path.'/'.$this->media->file_name;
        }

        return $path.'/conversions/'.$this->conversion->getName().'.'.
            $this->conversion->getResultExtension($this->media->extension);
    }

So now I got one-point of directory name rendering, and no any troubles with collisions and perfomance failing.

[freekmurze]

Hi,

you're right the performance may be an issue when storing a large amount of files. We've touched on this in #45.

This could be solved by adding an extra directory layer with a folders that hold 10000 items.

I'm thinking of a structure like this:

-medialib ---0 ------1 ------2 ------3 ... ----9999 ---1 ------10001 ------10002 ------10003 ...

This isn't yet implemented because we haven't had to deal which such big medialibraries ourselves.

I'd accept a PR that adds this functionality and provides tests to prove that it works.

[kickthemooon]

this is a nice conversation hope it'll have positive effect on the media library

[cviebrock]

I too would like to see some way to extract the subdirectory name generation out into one (overwriteable) method. The default way of using just the ID of the Media object is okay, but (as was mentioned above) will get unwieldy when you have thousands and thousands of images.

Also, in our case, we'd like to obfuscate the path a bit so that those primary keys aren't visible, using some hashing algorithm or something.

Maybe a method on the Media object itself that handles path generation? That way it's easily extendable by just specifying your own Media object:

// Spatie/MediaLibrary/Media.php

class Media extends Model {
    ....
    public function getMediaPath() {
        return $this->getKey();
    }
}

// App/Media.php

class Media extends Spatie\MediaLibrary\Media {
    ....
    public function getMediaPath() {
        return some_hashing_function( $this->getKey() );
    }
}

And \Spatie\MediaLibrary\Filesystem::getMediaDirectory and \Spatie\MediaLibrary\UrlGenerator\BaseUrlGenerator::getPathRelativeToRoot would both reference that method instead of hard-coding the ID.

Does that make sense?

[cviebrock]

I think the PR I have in #97 above is a bit better than my original idea above for adding methods on the model. It uses a PathGenerator class to do the work of coming up with unique path names, and those are used both for storage of files and for URL generation.

[freekmurze]

PR #97 has been merged. Changes are available in v.3.9.0

I found it solved this issue in a very elegant way. Thank you very much for your contribution.

[kickthemooon]

Could somebody help to change the media paths strategy to something like: Public/media

• 2015 (so the year as a folder )
  • 01
  • 02
  • 03 ( month, so the month as a folder under the year folder )
  • someimage.jpg
  • someimage-300x300.jpg
  • someimage-1024x686.jpg ( so the original and conversions under the month folder )

any way to achieve this?

or if no, could you suggest a nice, reliable and scalable folder structure, and how to implement it?

@alexglue @freekmurze @cviebrock

[alexglue]

@kickthemooon This is what i'm using at the moment:

The structure is like /a4f/ad3/f0c/104/file.jpg /a4f/ad3/f0c/104/c/thumb.jpg

*104 is media_id

//config/filesystems.php:

        'media' => [
            'driver' => 'local',
            'root'   => public_path().'/shared/media',
        ],

//config/laravel-medialibrary.php:
//...
'custom_path_generator_class' => App\Services\MediaLibrary\MediaLibraryPathGenerator::class,

and custom path generator:

class MediaLibraryPathGenerator implements PathGenerator
{
    /**
     * Get the path for the given media, relative to the root storage path.
     *
     * @param \Spatie\MediaLibrary\Media $media
     *
     * @return string
     */
    public function getPath(Media $media)
    {
        $parts = [
            implode(DIRECTORY_SEPARATOR, array_slice(str_split(md5($media->getKey()), 3), 0, 3)),
            $media->getKey()
        ];

        return implode(DIRECTORY_SEPARATOR, $parts) . DIRECTORY_SEPARATOR;
    }

    /**
     * Get the path for conversions of the given media, relative to the root storage path.
     *
     * @param \Spatie\MediaLibrary\Media $media
     *
     * @return string
     */
    public function getPathForConversions(Media $media)
    {
        return implode(DIRECTORY_SEPARATOR, [$this->getPath($media), 'c' . DIRECTORY_SEPARATOR]);
    }
}

@freekmurze @cviebrock It will be nice if only i could store its like /file.thumb.jpg instead of /c/thumb.jpg. Also it will be nice if only i could store it WITHOUT media_id in path. But unsuccessfully i can't do it with current version (

So, it is possible to have a lot of troubles with media_id in future when i would like to move some files physically or id_sequence will broken.