Skip to content

Conversation

@matteotrubini
Copy link
Contributor

No description provided.

Copy link
Member

@bennothommo bennothommo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matteotrubini we might need some additional error handling with this. I've noticed that the save() and openImage() methods both contain calls to methods that may not exist on PHP 8.0 (which is still supported by Winter 1.2.x). While one is surrounded in a condition, and the other has its error squelched, there's no handling of the lack of an image object that would occur if one were to use an AVIF format image on Winter on PHP 8.0.

Perhaps we should probably have some functionality after the switch blocks in both of these methods so that if there's no image (or no image can be saved), an exception is thrown - maybe saying that AVIF files are only supported on PHP 8.1 and above.

Thoughts?

@matteotrubini
Copy link
Contributor Author

The check for supported image types should be moved only to openImage(), as it is executed in the constructor. Therefore, the checks in save() would be redundant.

Additionally, the handling of extensions and MIME types should be consolidated, as it is currently scattered across various parts.

My opinion: minimize the effort required to maintain the class logic and plan a switch to a third-party image manipulation library, keeping Resizer as the access point.

@bennothommo
Copy link
Member

bennothommo commented Jun 11, 2024

My opinion: minimize the effort required to maintain the class logic and plan a switch to a third-party image manipulation library, keeping Resizer as the access point.

I know @LukeTowers is open to this, but I'm not so sure - at least for core feature purposes. I'm hesitant to introduce more third-party dependencies unless absolutely required, as it increases our maintenance burden when those libraries change their requirements (ie. Intervention Image now requires PHP 8.1 with their 3.x branch, and hasn't released an update to their 2.x branch since 2022). Plus, given what's happened recently with malicious actors (like the XZ debacle), it increases the possibility of bad things happening. I've seen Laravel is doing its best to minimise the third-party libraries it depends on (such as dropping Doctrine in the latest version), and I think that's a good aim to achieve.

The great thing though is - the Resizer library is totally customisable via events, and there's absolutely nothing stopping someone from using those events to use a library like Intervention Image as part of a plugin, if they so choose.

@LukeTowers LukeTowers added this to the 1.2.7 milestone Jun 11, 2024
@LukeTowers LukeTowers merged commit 374add9 into wintercms:develop Jun 11, 2024
@matteotrubini matteotrubini deleted the imp-avif-resizer branch June 11, 2024 15:12
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.

3 participants