spatie / laravel-medialibrary

Associate files with Eloquent models
https://spatie.be/docs/laravel-medialibrary
MIT License
5.78k stars 1.08k forks source link

Wrong order of manipulations #3665

Closed Jackardios closed 4 months ago

Jackardios commented 4 months ago

The documentation says that when using withManipulations "The package will regenerate all files (conversions) using the saved manipulation as the first manipulation when creating each derived image." But in fact this is not the case, all manipulations that are added through withManipulations will be executed last.

For example:

class SomeModel extends Model implements HasMedia
{
    use InteractsWithMedia;

    public function registerMediaCollections(): void
    {
        $this->addMediaCollection('image')->singleFile();
    }

    public function registerMediaConversions(Media $media = null): void
    {
        $this->addMediaConversion('cropped')
            ->fit(Fit::Crop, 300, 300)
            ->performOnCollections('image');
    }
}

$someModel = SomeModel::create();
$addedMedia = $someModel->addMedia($mediaFile)
  ->storingConversionsOnDisk(config('media-library.disk_name'))
  ->withManipulations([
      'cropped' => [
          'manualCrop' => [500, 300, 200, 150],
      ],
  ])
  ->toMediaCollection('image');

$conversions = Spatie\MediaLibrary\Conversions\ConversionCollection::createForMedia($addedMedia)
  ->filter(fn (Spatie\MediaLibrary\Conversions\Conversion $conversion) => $conversion->shouldBePerformedOn($addedMedia->collection_name));

dd($conversions->first()->getManipulations()->toArray());
[
  "optimize" => [
    Spatie\ImageOptimizer\OptimizerChain {#7285},
  ],
  "format" => [
    "jpg",
  ],
  "fit" => [
    Spatie\Image\Enums\Fit {#7532
      +name: "Crop",
      +value: "crop",
    },
    300,
    300,
  ],
  "manualCrop" => [500, 300, 200, 150],
]

I'm guessing this is due to an incorrect array merge in addAsFirstManipulations method: https://github.com/spatie/laravel-medialibrary/blob/4ebb5642323aa7399cc6a5924b8245adc1862678/src/Conversions/Conversion.php#L140 Instead it should be:

$allManipulations = array_merge($newManipulations, Arr::except($currentManipulations, array_keys($newManipulations)));

If this is indeed a bug, I can create a PR to fix it

freekmurze commented 4 months ago

I'm be open for a PR that fixes this. Make sure to include tests so this won't break again in the future.