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

WS2-1575: Card Arrangement: PNG images not being compressed #76

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

juanmitriatti
Copy link
Contributor

Ticket URL : https://asudev.jira.com/browse/WS2-1575
I installed a new module in order to allow PNG quality compression.

From Drupal module documentation :
Requirements
ImageMagick (or GraphicsMagick) needs to be installed on your server and the convert (or gm) binary needs to be accessible and executable from PHP.
The PHP configuration must allow invocation of proc_open() (which is security-wise identical to exec()).
Consult your server administrator or hosting provider if you are unsure about these requirements.
Contrib Module URL : https://www.drupal.org/project/imagemagick

Test :
Im using ddev in my personal, it's already set in my server. However, it needs to be checked on the server side.

@juanmitriatti juanmitriatti requested a review from a team March 29, 2023 17:28
@tbutterf
Copy link
Contributor

tbutterf commented Mar 29, 2023

https://docs.pantheon.io/external-libraries#imagemagick

Imagemagick is part of Pantheon, and they have instructions for configuring the Drupal module at the link above.

@tbutterf
Copy link
Contributor

I'm building a multidev on Pantheon now to test.

@tbutterf
Copy link
Contributor

My build failed because we need to make sure that we add the dependencies to the composer.json file. I guess that might be a different ticket, but it will need to be coordinated with this one, or it will fail.

@tbutterf
Copy link
Contributor

@juanmitriatti I have gotten a pantheon site up and running to test this. From what I can tell, the config/install files didn't get added because the profile had already been installed. So, we might need to trigger those configs being imported with the webspark_update_9003 function.

Copy link
Contributor

@tbutterf tbutterf left a comment

Choose a reason for hiding this comment

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

See my previous comments. My test environment is here: https://dev-webdirtravis.ws.asu.edu/

Copy link
Collaborator

@mlsamuelson mlsamuelson left a comment

Choose a reason for hiding this comment

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

Reviewed. Looks good at the code level. PR to add the module in the release testing upstream is completed (ASUWebPlatforms/webspark-release-testing#78).

@tbutterf Since you have a test environment spun up, can you verify with a test? Or @bjdevil21

\Drupal::service('webspark.config_manager')->importConfigFile('imagemagick.settings.yml');
\Drupal::service('webspark.config_manager')->importConfigFile('sophron.settings.yml');
\Drupal::service('webspark.config_manager')->importConfigFile('system.image.yml');
\Drupal::state()->set('configuration_locked', TRUE);
Copy link
Contributor

Choose a reason for hiding this comment

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

@juanmitriatti The importConfigFile function requires that the file name not have the extension at the end. Also, there is a problem with that function. It should reference $filename, but it only references the undefined $name variable. So, it doesn't work. I have created a ticket that fixes that problem and issued a pull request. So, now this PR depends on that one (ASUWebPlatforms/webspark-module-webspark_utility#23).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @tbutterf
Pr has been updated, please check.
Thanks for your feedback.

@mlsamuelson mlsamuelson added bug Something isn't working and removed bug Something isn't working labels May 1, 2023
@mlsamuelson
Copy link
Collaborator

mlsamuelson commented May 1, 2023

All pre-requisite PRs are now merged. Tested locally. It installs. update.php runs successfully.

Image configuration, however is still defined for GD2 image processing instead of ImageMagick:
/admin/config/media/image-toolkit

I tried re-running the configuration update hook and still the same result. Runs without errors, but the system image toolkit setting (and maybe others) aren't updated.

Also, the install instructions from Pantheon aren't implemented yet. Perhaps that's related?

@mlsamuelson mlsamuelson marked this pull request as draft May 1, 2023 19:27
@mlsamuelson
Copy link
Collaborator

@juanmitriatti I converted this PR to a Draft PR, since it will be made against the new webspark-ci repo instead, when it's ready.

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