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

EZP-27993: Support quality override by image alias #1324

Merged
merged 1 commit into from
Nov 22, 2017

Conversation

peterkeung
Copy link
Contributor

@peterkeung peterkeung commented Sep 28, 2017

https://jira.ez.no/browse/EZP-27993

In eZ Publish legacy, we can set a global image quality in image.ini, but cannot override this per alias.
This is the reverse problem as eZ Platform / new stack, where you can set the quality by alias but cannot set a global value (liip/LiipImagineBundle#909).

This way, you can set something like this in image.ini to override the global setting:

[medium]
Quality[]
Quality[]=image/jpeg;80

# Override quality by MIME type for this alias only
# This follows the pattern from the global [MIMETypeSettings] settings
# Quality[]
# Quality[]=image/jpeg;80
Copy link
Collaborator

Choose a reason for hiding this comment

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

In PR description you mention Quality[image/jpeg]=80 as format. This is the correct one as far as I can see?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I've fixed the PR description now! Good catch. I got distracted by the format of the internal map.

foreach ( $qualityOverride as $qualityOverrideValue )
{
$elements = explode( ';', $qualityOverrideValue );
$mimeType = $elements[0];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe do a basic check if $elements[0] and $elements[1] exist before using them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was copying readMIMETypeQualitySettingFromINI() on line 559, so it's consistent to expect the user to follow the right format. Could harden this in a follow-up PR though.

@emodric
Copy link
Collaborator

emodric commented Sep 29, 2017

Useful feature 👍

I'm wondering about why use the override values which are constantly reset and filled for every image conversion process? I guess there's no other way if we want to keep eZImageManager interface intact?

@peterkeung
Copy link
Contributor Author

Right, this was what I came up with without having to send the alias information to each individual conversion handler. I can certainly change it if someone thinks of a way to do this per alias while maintaining the split in concerns between eZImageManager and the conversion handlers.

@peterkeung
Copy link
Contributor Author

Ping @alexgorski

@alexgorski
Copy link

Very useful feature requested by amnh-digital.

@thiagocamposviana
Copy link
Contributor

Looks good to me
Basically before we could only set the quality by mimetype, now it is possible to define quality by alias, which is very important, also defining quality only by mimtype is so limited it is not very helpful.

@ernestob
Copy link

Besides the comment from @emodric about testing the values of the quality array, this looks good.

This is a must have feature. 👍

@benkmugo
Copy link
Contributor

Great addition for sites that have a lot of aliases where quality needs to be controlled better due to alias size differences e.g. huge banners with high-quality needs vs. thumbnails that can be lower quality.

We've got at least a handful of clients where this update would mean a big improvement.

+1 from me

@andrerom andrerom added this to the 2017.12 milestone Nov 17, 2017
@andrerom
Copy link
Contributor

@crevillo you are also good with this?

@crevillo
Copy link
Contributor

yes! I am, sorry, i didn't see this notification. @peterkeung just pinged me about.

@andrerom andrerom changed the title Fix EZP-27993: Support quality override by image alias EZP-27993: Support quality override by image alias Nov 22, 2017
@andrerom
Copy link
Contributor

andrerom commented Nov 22, 2017

Thanks @crevillo and @emodric for the reviews, and thanks for the work here @peterkeung!

@andrerom andrerom merged commit f7dff9e into ezsystems:master Nov 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
8 participants