Command media-library:clean does not use custom path generators

[FishYellow085]

Hello,

The media-library:clean command uses the default Spatie\MediaLibrary\Support\PathGenerator\DefaultPathGenerator instead custom path generators that can be configured with the media-library.custom_path_generators configuration key (it's hard-coded in the command file). Due to that, if we use a custom path generator for models and configure a custom path for conversions, the deprecated images inside this custom directory won't be cleaned by the command (since the command do not look in the correct directory because it uses the default path generator included in the package).

As a simple example, let's assume I added the media relationship on the default User model and defined the following custom path generator:

<?php

namespace App;

use Spatie\MediaLibrary\MediaCollections\Models\Media;
use Spatie\MediaLibrary\Support\PathGenerator\DefaultPathGenerator;

class UserPathGenerator extends DefaultPathGenerator
{
    /**
     * @inheritDoc
     */
    public function getPathForConversions(Media $media): string
    {
        return $this->getBasePath($media) . '/my-custom-path/';
    }
}

and configuration in the config/media-library.php file:

/*
 * Here you can specify which path generator should be used for the given class.
 */
'custom_path_generators' => [
    \App\Models\User::class => \App\UserPathGenerator::class,
],

Media conversions will be correctly stored in a /my-custom-path sub-directory, the media-library:regenerate command will correctly regenerate conversions in this directory, but the media-library:clean command won't see deprecated conversions in this sub-directory but will instead look in the /conversions sub-directory.

I don't know how the dynamic part of the media-library.custom_path_generators is handled inside the package but it should probably also be used in the media-library:clean command.

[petsoukos]

Ouch... Needed this command today to clean 50m items generated by a custom path config

From what I can tell the CleanCommand's handle() method injects the DefaultPathGenerator directly

[petsoukos]

For now and until there's a proper implementation I changed the variable $conversionPath in deleteConversionFilesForDeprecatedConversions method, to $conversionPath = (new (config('media-library.path_generator')))->getPathForConversions($media);

https://github.com/spatie/laravel-medialibrary/blob/main/src/MediaCollections/Commands/CleanCommand.php#L113

[FishYellow085]

I think using the Spatie\MediaLibrary\Support\PathGenerator\PathGeneratorFactory factory already included in the package would be better? Something like $conversionPath = PathGeneratorFactory::create($media)->getPathForConversions($media), which would let the factory automatically instantiate the correct path generator as it's supposed to.

I've also realised while testing this modification that another issue would appear in the next lines in case the conversions path is configured to be the same path as the base the base image (so no sub-directory): the base image would be considered a deprecated conversion by the code and be removed. The complete method, with both the path generator and delete code fixed, I end up with is:

protected function deleteConversionFilesForDeprecatedConversions(Media $media): void
{
        $conversionFilePaths = ConversionCollection::createForMedia($media)->getConversionsFiles($media->collection_name);

        $conversionPath = PathGeneratorFactory::create($media)->getPathForConversions($media);  // Fixed path generator
        $currentFilePaths = $this->fileSystem->disk($media->disk)->files($conversionPath);

        collect($currentFilePaths)
            ->reject(fn (string $currentFilePath) => $conversionFilePaths->contains(basename($currentFilePath)))
            ->reject(fn (string $currentFilePath) => $media->file_name === basename($currentFilePath)) // Exclude base image from deprecated list
            ->each(function (string $currentFilePath) use ($media) {
                if (! $this->isDryRun) {
                    $this->fileSystem->disk($media->disk)->delete($currentFilePath);

                    $this->markConversionAsRemoved($media, $currentFilePath);
                }

                $this->info("Deprecated conversion file `{$currentFilePath}` ".($this->isDryRun ? 'found' : 'has been removed'));
            });
}

I have no idea is this would be an acceptable solution but at least it does the trick for now.

[petsoukos]

@TheoNetemedia Yes, it works even better than my temporary fix, because it can handle the custom_path_generators config. I only considered my needs when I posted the comment 😄

[freekmurze]

@TheoNetemedia could you PR that?

[FishYellow085]

PR #2925 opened with the changes I proposed above, but with my personal account (this is my professional account) to make it easier for me to go back to it (if necessary) outside of work hours, I apologise for the confusion it might cause.

[freekmurze]

We'll continue this in #2925