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-31377: Use imagick by default on Platform.sh to avoid memory issues #501

Merged
merged 7 commits into from
Feb 24, 2020

Conversation

andrerom
Copy link
Contributor

@andrerom andrerom commented Feb 11, 2020

Question Answer
JIRA issue EZP-31377
Bug/Improvement yes
New feature no
Target version 2.5
BC breaks no
Tests pass N/A
Doc needed no

GD uses PHP memory and hence it's better if we can use imagick if it's available to avoid risking running into php memory issues on larger images.

TODO:

  • Implement feature / fix a bug.
  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

app/config/env/platformsh.php Outdated Show resolved Hide resolved
Co-Authored-By: Konrad Oboza <34310128+konradoboza@users.noreply.github.com>
Copy link
Member

@vidarl vidarl left a comment

Choose a reason for hiding this comment

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

Sorry, I have second thoughts about my approval here.

We already have configuration for imagemagick in ezplatform.yml:

ezpublish:
    imagemagick:
        enabled: true
        path: /usr/bin/convert

I think we should use that ( currently not used/refered to at all in meta repo), instead of setting the liip_imagine.driver parameter directly

Copy link
Member

@vidarl vidarl left a comment

Choose a reason for hiding this comment

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

I was made aware of these settings when working on a migration. Was not aware that they were only used for interjecting into legacy. Your PR looks good then

@andrerom
Copy link
Contributor Author

Ready for testing again then @micszo

Copy link
Contributor

@damianz5 damianz5 left a comment

Choose a reason for hiding this comment

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

I'm still verifying it but looks like there is no parameter liip_imagine.driver in
php bin/console debug:container --parameters --env=dev | grep liip_imagine
similar parameter is liip_imagine.driver_service -> liip_imagine.gd, but its resolved from config, not parameters

ref https://github.com/liip/LiipImagineBundle/blob/88775aaa5f208885c3486d34d0360486262c50f3/DependencyInjection/LiipImagineExtension.php#L102

@andrerom
Copy link
Contributor Author

@damianz5 Ok, do you see a way we can set it dynamically then?

@damianz5
Copy link
Contributor

Updated - ready for retest

@andrerom
Copy link
Contributor Author

andrerom commented Feb 24, 2020

Thanks @damianz5 👍 👍

PS: I took the liberty of removing the dot in the param name now to make to more clear it's global param.

Status: Ready for QA now

Copy link
Member

@micszo micszo left a comment

Choose a reason for hiding this comment

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

Retested successfully.

@micszo micszo removed their assignment Feb 24, 2020
@andrerom andrerom merged commit 9d42686 into 2.5 Feb 24, 2020
@andrerom andrerom deleted the EZP-31377_imagick_by_default_on_psh branch February 24, 2020 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants