Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check for enum instances when applying manipulations #3661

Merged

Conversation

dennisvandalen
Copy link
Contributor

@dennisvandalen dennisvandalen commented Jul 7, 2024

It seems there is a bug introduced when passing paramaters that are already enum to image manipulations. #3626

The existing code for image manipulations was using a match expression to transform parameters based on the manipulation name. However, the match expression does not perform assignments in the way expected, leading to an error when attempting to convert parameters to enum types. Specifically, the following error was encountered:

TypeError: Spatie\Image\Enums\Fit::from(): Argument #1 ($value) must be of type string|int, Spatie\Image\Enums\Fit given in /var/www/html/vendor/spatie/laravel-medialibrary/src/Conversions/Manipulations.php:69

The root cause was that some parameters were already instances of the enum classes, and attempting to reassign them caused errors.

Solution Overview

To address this issue, the match expression was replaced with a switch statement. This allows for more explicit checks and transformations of parameters, ensuring that parameters are only converted if they are not already instances of the corresponding enum classes. The solution involves:

  1. Using a switch statement to handle each manipulation case.
  2. Checking if the parameter is not already an instance of the enum class before converting it.

For readability reasons I've changed the match to a switch statement.

I've also removed the pickColor handling as it is not not to be a valid image manipulation operation.

@freekmurze freekmurze merged commit e8d9f9f into spatie:main Jul 15, 2024
9 checks passed
@freekmurze
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants