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.

Sign up for GitHub

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

Jump to bottom

Icon resize #10721

Merged
merged 2 commits into from Jan 27, 2017
Merged

Icon resize #10721

merged 2 commits into from Jan 27, 2017

Conversation

jeabakker
Copy link
Member

fixed several problems with image resizing

ref #10686

@jeabakker
fix(icons): set correct filename for temp resizing file
aeed706 
The new resize lib expects a valid image extension in order to resize an
image. Two problems existed, 1 when an image is uploaded it's stored in
a tmp file without a valid image extension and 2 if called when logged
out the file destination could not be created.
hypeJunction
hypeJunction reviewed Jan 24, 2017
View reviewed changes
engine/classes/Elgg/ImageService.php
* @see https://github.com/Elgg/Elgg/issues/10686
* @return void|string
*/
protected function getFileFormat($filename, $params) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use Imagine to do this? We also have a similar method in one of the file related classes (see ElggFile::detectMimeType())

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we use Imagine to do this?

I looked but couldn't find any

We also have a similar method in one of the file related classes (see ElggFile::detectMimeType())

maybe but $filename doens't have to be in the Elgg file structure (eg uploaded file is in /tmp)

Copy link
Contributor

Choose a reason for hiding this comment

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

See \Elgg\Filesystem\MimeTypeDetector, it's agnostic to Elgg's file system and works with file paths.

Copy link
Member Author

Choose a reason for hiding this comment

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

See \Elgg\Filesystem\MimeTypeDetector, it's agnostic to Elgg's file system and works with file paths.

I changed it, thanks for the tip

@hypeJunction
Copy link
Contributor

Can we add a test to see what the expected outcome is? From the code it's unclear what issue this is fixing.

@jeabakker
Copy link
Member Author

Can we add a test to see what the expected outcome is? From the code it's unclear what issue this is fixing.

I added 3 tests for resizing

  1. resize from an image with an image extension
  2. resize from a temp file (extension is .tmp, but the contents is still an image)
  3. resize from an image without an extension

@jeabakker
fix(icons): detect image format for resizing
dd9af8a 
Detect the correct image format to use for resizing an image based on
supplied output format of mimetype of the source image

fixes Elgg#10686
@jdalsem jdalsem merged commit 6f71d66 into Elgg:2.3 Jan 27, 2017
@jeabakker jeabakker deleted the icon-resize branch May 15, 2017 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hypeJunction hypeJunction hypeJunction approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jeabakker @hypeJunction @jdalsem