FriendsOfFlarum / upload

The file upload extension with insane intelligence for your Flarum forum.
https://discuss.flarum.org/d/4154
MIT License
177 stars 96 forks source link

No limits applied to image height #218

Closed fran-f closed 3 years ago

fran-f commented 4 years ago

While the text in the settings reads "You can choose a maximum width and height, in pixels.", as far as I can see the code only applies a width limit:

protected function resize(Image $manager)
{
    $manager->resize(
        $this->settings->get('resizeMaxWidth', Settings::DEFAULT_MAX_IMAGE_WIDTH),
        null,  // ← no height set
        // ...
    );
}

I'd be happy to provide a patch, and I can see three ways to solve this:

  1. fix the text, leave the code unchanged 😛
  2. apply the same limit to both dimensions (and update the text)
  3. add a second setting for the height limit

My preference would be to implement (2).

clarkwinkelmann commented 4 years ago

Option 2 seems like a good fix to me as well. And using the fit($width, $height) method instead of resize() should work fine.

fran-f commented 4 years ago

fit() will crop the image to fit the given dimensions, while we only want to resize the image, keeping the same aspect ratio. With the constraints defined in the code, resize() will do that.

clarkwinkelmann commented 4 years ago

Oops my bad. fit does not work here indeed.

Can resize actually be used in a single call? I don't see that option described in Intervention's documentation.

Or does it require to first compare which of height or width is the largest, then use widen or heighten accordingly?

fran-f commented 3 years ago

Closing this — solved by #219 👍