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

Added admin notice when Imagick is not installed #51

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

igmoweb
Copy link
Contributor

@igmoweb igmoweb commented Dec 17, 2019

Imagick is needed to use this plugin, however it is not always installed.

I've added an admin notice to display an error message when the module is not available. There are certain things that could be also improved and discussed regarding this:

  • Should JPEG\data_for_file() return an error when the module is not available?
  • WP CLI just displays an standard The site is experiencing technical difficulties error when Imagick is not installed. It took me a while to realize what was the problem. Should we also add an error message for WP CLI?
  • Should the notice be dismissable?

Copy link
Contributor

@roborourke roborourke left a comment

Choose a reason for hiding this comment

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

Should JPEG\data_for_file() return an error when the module is not available?

I think we should just avoid bootstrapping the plugin at all apart from the admin notice in this case.

WP CLI just displays an standard The site is experiencing technical difficulties error when Imagick is not installed. It took me a while to realize what was the problem. Should we also add an error message for WP CLI?

Yeah I reckon that'd be helpful.

Should the notice be dismissable?

I think that's not super important, you either have Imagick or you don't, if you don't then you should probably be removing or deactivating the plugin.

inc/admin/namespace.php Outdated Show resolved Hide resolved
Co-authored-by: Robert O'Rourke <[email protected]>
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.

2 participants