Requirements should be provided by image toolkit

Subscribing

subscribe

This is a bug in Drupal core and cannot be resolved from contrib.

image.install unconditionally checks GD requirements, regardless of whether GD is actually being used.

This also means that you cannot install Drupal core without a working GD library. Even if you have ImageMagick readily available.

Somewhat related, but different aspects of GD validation in Drupal core:
#758628: Improve GD library hook_requirements() for color and image modules
#102987: Validation for GD toolkit

I don't now if its related but I saw 'GD' so my brain matches this with

#1115000: GD Error after moving gif upload

Is there a separate issue filed for the fact that drupal cannot be installed in an imagemagick-only environment?

How do we think this can be fixed? The only way to do this is to remove the gd check and then ... what? The standard profile has some image field if i remember correctly (not that i have seen standard in the last year or two) which will happily fail without gd and no imagemagick which is 99% of the cases.

Perhaps make it a "soft" fail that complains on the admin pages but not a hard failure and make it go away if the current image toolkit reports OK?

webchick points to #758628: Improve GD library hook_requirements() for color and image modules . Demoting.

BTW if Drupal install now without GD then please close as Duplicate.

Regardless of error or warning, both levels are "severe" -- what we need is a hook_requirements_alter(), so that a module like ImageMagick is able to conditionally alter those GD warnings away (e.g., into "info" severity), in case ImageMagick requirements are met.

Shouldn't \Drupal\system\Plugin\ImageToolkitInterface define a new method requirements(). Then image_requirements() would get the default toolkit (\Drupal\system\Plugin\ImageToolkitManager::getDefaultToolkit()) and call the right toolkit requirements?

Of course, after a clean install the system will report a broken GD library (if it's broken) because Drupal is shipped with GD toolkit. But this is the expected behaviour. When a new toolkit, like ImageMagick, is configured to be used as default toolkit, the system will report ImageMagick's requirements.

What about that?

So file module does something similar here. It searches for file upload progress implementations, of which there are multiple.

claudiu.cristea's solution sounds much more generic though, where file module runs through a hard coded list of possible implementations. requirements() method on the Interface sounds like a sensible approach to me.

• Fixed title
• Added "change notification" tag
• Ownership taken

Here's a patch.

Fixed some docs in interface.

Tested locally (disabled gd, tried to fresh install, re-enabled gd and installed).

Patch looks awesome. Assuming test bot likes it, RTBC.

As is green now, it needs a review. Any takers? :)

Switched to "task" as it's a task.

Has been tested manually in #16, bot likes it, code looks good. RTBC.

@claudiu.cristea contrib may want to add additional requirements to a toolkit (e.g. Textimage checks for FreeType support). Maybe we can find a clever way to extend this when #2073759: Convert toolkit operations to plugins is in... (scanning for plugins whose operation is 'requirements.xxxx'). Followup anyway.

#15: toolkit-req-1069140-15.patch queued for re-testing.

I bet this needs reroll after #2048827: Move Image toolkit API from system.module to Drupal\Core :)

Needs reroll

Let other contributor reroll it so that you can still re-RTBC ;)

Rerolled but consolidated also the namespacing because of future #2106903: Image toolkit operations may provide requirements. Let's follow this rule for image requirements:

• image.{key}: General image requirement. We don't have right now but it may occur in the future.
• image.toolkit.{toolkit}.{key}: Toolkit specific requirements.
• image.toolkit.{toolkit}.{operation}.{key}: Toolkit operation requirements.

+++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitInterface.php
@@ -174,6 +174,21 @@ public function save(ImageInterface $image, $destination);
+   *   name and toolkit id (e.g. 'image_gd_' should not be added as prefix to

Nitpick - should then be "e.g. 'image.gd.' should not..."

Right.

Thanks for nitpicking on the nitpick ;)

RTBC again then.

+++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitInterface.php
@@ -174,6 +174,21 @@ public function save(ImageInterface $image, $destination);
+   *   array. The array must have arbitrary and unique keys. Keys shouldn't be
+   *   prefixed by name and toolkit, the system automatically adds the module
+   *   name and toolkit id (e.g. 'image.toolkit.gd.' should not be added as
+   *   prefix to the array key).
+   *

Unique within the array returned? No problem, always true:) Change to something like

The array can have arbitrary keys and they do not have to be prefixed by e.g. the toolkit ID, as the system will make the keys globally unique.

Question: are the requirements checked during install when the GD toolkit is automatically set as the default toolkit?

Unique within the array returned? No problem, always true:) Change to something like

The array can have arbitrary keys and they do not have to be prefixed by e.g. the toolkit ID, as the system will make the keys globally unique.

Not true. They must be unique inside the same toolkit. See also https://api.drupal.org/api/drupal/core%21modules%21system%21system.api.p... from where I started.

Question: are the requirements checked during install when the GD toolkit is automatically set as the default toolkit?

No, just runtime. See my comment from #11.

1 toolkit returns 1 array, so yes, the keys are unique, you don't have to document that a PHP array cannot contain 2 equal keys. This sentence confused me and so will probably confuse others as well.

Your comment from #11 states the opposite: "Of course, after a clean install the system will report a broken GD library (if it's broken)". This is what I (and you) would expect. But if this is considered confusing, I can live with it. OTOH, the only people that will encounter this are those that do their own installs (shared hosting always comes with GD), they will not be terrified by this message but nor will the absence of it cost them much time and make them $%^#@$%.

So that part is OK for me.

If you change the comment, it is RTBC for me as well.

ok

Thanks.

#32: toolkit-req-1069140-32.patch queued for re-testing.

There's an interesting crossover here between the requirements method and isAvailable method at the very least we should be calling the isAvailable method form the requirements method.

I think we should test this - perhaps using the BrokenToolkit.php

They are indeed almost the same. But isAvailable is non-verbose in the sense that it returns a bool, without telling what may be the problem, so IMO useless for requirements, as we cannot provide a clear message to the user what he should do to solve the problem. Calling the other way round seems more logical then in which case isAvailable can even become part of the (yet to commit) base image toolkit.

We could make the code check the same though, but only for the REQUIREMENT_ERROR messages. We could also get rid of isAvailable, but only if it is an accepted policy that Drupal may fail with a fatal non-interceptable error when not all requirements are met.

@alexpott,

OK, it makes sense to reuse isAvailable() in requirements() for GDToolkit. But this doesn't guarantee that it will be used in the same way in other ImageToolkitInterface implementations. That's why I see no reason for a test. Or I didn't understood well the purpose of the test.

1. +++ b/core/modules/image/image.install
   @@ -29,30 +29,15 @@ function image_uninstall() {
   +  $toolkit = \Drupal::service('image.toolkit.manager')->getDefaultToolkit();
   

   getDefaultToolkit() calls isAvailable() and won't return the default toolkit if it is not available. So requirements checking will not be executed if it would issue a REQUIREMENT_ERROR.

2. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
   @@ -275,6 +275,36 @@ public function createTmp(ImageInterface $image, $width, $height) {
   +    if (static::isAvailable()) {
   

   Use self::

The isAvailable() calling is a nasty error. Getting around it, I think we should abandon the whole idea of isAVailable(). Now that we get requirements checking per toolkit, the need for checking the requirements again and again on each instantiation looks a bit overkill to me.

@fietserwin is right.

I did a

$ grep --exclude-dir=core/vendor -nr isAvailable core
core/lib/Drupal/Core/ImageToolkit/ImageToolkitInterface.php:196:  public static function isAvailable();
core/lib/Drupal/Core/ImageToolkit/ImageToolkitManager.php:87:      if (call_user_func($definition['class'] . '::isAvailable')) {
core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php:281:    if (static::isAvailable()) {
core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php:308:  public static function isAvailable() {
core/modules/system/tests/modules/image_test/lib/Drupal/image_test/Plugin/ImageToolkit/BrokenToolkit.php:23:  public static function isAvailable() {
core/modules/system/tests/modules/image_test/lib/Drupal/image_test/Plugin/ImageToolkit/TestToolkit.php:136:  public static function isAvailable() {

We can see here that isAvailable() is used only once (!) in ImageToolkitManager.php (there is also the usage added by #37 -- ignore it!). So why keeping that in interface for only one operation?

My new approach is:

• Drop isAvailable()
• Rename requirements() to availability() because now it will return the return of deceased isAvailable() while still returning also the requirements array. Check the patch to see how :-)

1. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitInterface.php
   @@ -174,12 +174,22 @@ public function save(ImageInterface $image, $destination);
   +  public static function availability(array &$requirements = array());
    
   

   availability is a noun. I prefer to name methods after an action (verb, e.g. checkRequirements) or state (adjective, e.g. isAvailable). And yes, I know the hook was/is named requirements but in Drupal hook context that was more common. Calling it requirements would also be fine with me.

2. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitManager.php
   @@ -84,7 +84,7 @@ public function getAvailableToolkits() {
   +      if (call_user_func($definition['class'] . '::availability')) {
            $output[$id] = $definition;
   

   The bug remains: if availability returns FALSE, getDefaultToolkit() will return FALSE and thus image_requirements will fail with a fatal "trying to access method on non-object" or something like that. Anyway, resulting in a WSOD.

3. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
   @@ -275,14 +275,36 @@ public function createTmp(ImageInterface $image, $width, $height) {
        if ($check = get_extension_funcs('gd')) {
   -      if (in_array('imagegd2', $check)) {
   -        // GD2 support is available.
   -        return TRUE;
   +      $available = in_array('imagegd2', $check);
   +    }
   +    else {
   +      $available = FALSE;
   +    }
   +
   

   $available = function_exists('imagegd2');
   More consistency (same check as below), less code, less memory consumption (array creation), better performance.

So I still favor dropping isAvailable() at all in favor of a 1 time requirements checking beforehand. That will mean that applying image effects may result in a WSOD (undefined function), but the current implementation also results in a WSOD, because getDefaultToolkit() does not throw an exception, but returns FALSE which will subsequently be accessed as an object.

NOTE: the "WSOD" will probably occur on image generation, so is not as bad as a real WSOD on page generation. Therefore I think that dropping the runtime check (isAvailable) in get DefaultToolkit, is a good and acceptable idea. Otherwise getDefaultToolkit must throw an exception, so we get Drupal error handling and logging instead of a WSOD.

#40.1: That's a matter of taste. It's not isAvailable() because along with FALSE/TRUE is returning also requirements. It's not requirements() because it returns not only requirements but mainly if the toolkit is available. So, please provide a name that need to satisfy both return values (is available & requirements). I admit that my English is not too creative but till we'll found a better term let's go with this.

#40.2: This needs more attention. I don't have strong point of view that's why I didn't touch that for now.

#40.3: No, we need to test if there's a imagegd2() provided by GD extension. Otherwise any module can declare such function.

re #41-#40.1 this will be a non-issue when we follow my proposal to drop ti at all.

re #41-#40.2: This is a serious bug, so it has nothing to do with opinions or points of view. We can't call getDefaultToolkit in image_requirements as long as it returns FALSE (or just some "random" other toolkit) for a not available toolkitThe requirements checking is there to prevent (or at least foresee and warn) that from happening so cannot use the code that will fail because the requirements are not met.

re #41-#40.3: Far sought, but I have to admit that it occurred to me as well, but my thoughts are that if someone disables gd and implements its own imagegd2 function, I would like to assume that this person is providing its own replacement for gd and accept it as such and thus not bother and just call his own functions. But in that case, we would better check the functions that are called in the various effects. But anyway, you can check with extension_loaded() if gd is available and then dive deeper into specific requirements for specific effects:

// Check for GD version 2.
$available = extension_loaded('gd') && function_exists('imagegd2');

BTW: I do not know the differences between GD1 and GD2, but it seems like a strange requirement to me to require version 2 because of an image format we don't even support. So we should check for the existence of used functions, but that is for #2106903: Image toolkit operations may provide requirements.

So, what is the error when no toolkit is available? Isn't it Hey! There's no (valid) toolkit over here? Why do we insistently want to get a "no toolkit" error message out from an invalid/unavailable toolkit? That REQUIREMENT_ERROR was basically the toolkit-agnostic part of the requirements. And this is what we are doing here: we are separating general requirements from toolkit specific requirements. "No toolkit" is a general image system requirement error and not a GD2 specific one.

Simple, isn't so?

+++ b/core/modules/image/image.install
@@ -29,30 +29,30 @@ function image_uninstall() {
+  $toolkit->availability($requirements);
+  foreach ($requirements as $key => $requirement) {
+    $namespaced_key = 'image.toolkit.' . $toolkit->getPluginId() . '.' . $key;
+    $requirements[$namespaced_key] = $requirement;
+    unset($requirements[$key]);
   }
 

1. Hm. This means that the requirements will only be checked for the currently selected toolkit. But I believe when it comes to requirements, we would like to check requirements for any of the toolkits installed.
As a side reasoning, this may support the need to have image toolkits as independent modules, as discussed in #2105863-24: [meta] Images, toolkits and operations and in #2110835: Move the GD toolkit on its own module.
In other words, one may want to totally disable the GD toolkit and enable Imagemagick instead. To do this we would need to have the GD toolkit as a module that can be unistalled, which is not the case right now (we cannot uninstall the system module...)

2. I wonder about the getAvailableToolkits() method in general, at this point. It is basically the result of getDefinitions() less those toolkits that do not pass the availability check. If I understand right, this check occurs on every ImageFactory class instantiation via getDefaultToolkit(). Wouldn't it make sense to move the availability check to findDefinitions instead, so that only the available toolkit plugins get cached in the definitions? Then we could drop getAvailalableToolkits() and just use getDefinitions() in its place in getDefaultToolkit(). OTOH, we need to be careful about this in the toolkit selection form UI as we may need to list also toolkit that do not pass requirements there (e.g. a requirement may be dependent on a config setting, like the path of an executable).

@mondrake,

Hm. This means that the requirements will only be checked for the currently selected toolkit. But I believe when it comes to requirements, we would like to check requirements for any of the toolkits installed.

Let's not get into that complexity. No, I'm against revealing all installed toolkits requirements. Keep it simple.

As a side reasoning, this may support the need to have image toolkits as independent modules, as discussed in #2105863-24: [meta] Images, toolkits and operations and in #2110835: GD toolkit location.
In other words, one may want to totally disable the GD toolkit and enable Imagemagick instead. To do this we would need to have the GD toolkit as a module that can be unistalled, which is not the case right now (we cannot uninstall the system module...)

Let's keep that discussion in #2110835: Move the GD toolkit on its own module. I made an idea on that.

2. I wonder about the getAvailableToolkits() method in general, at this point. It is basically the result of getDefinitions() less those toolkits that do not pass the availability check. If I understand right, this check occurs on every ImageFactory class instantiation via getDefaultToolkit().

IMO we need the cached list of all toolkits, regardless if they are broken or valid. I wouldn't remove getAvailalableToolkits() because it's cheap in terms of performance. If you feel so, just open another ticket for this issues, here it's an out-of-scope problem.

@mondrake, returning to the main issue, how it looks like? I mean is this ready for RTBC?

Yes, I think so. RTBC for. me. I'll think more on a followup for #44.2.

I think that requirements should be checked on:
- submitting the toolkit selection form. But beware, this is a 2 step process, unless we ajax load toolkit specific form fields on selecting a different toolkit. So accept the new toolkit selection but display the failing requirements on the top of the page.
- status report

and not on:
- each instantiaton (as the current isAvailable() does).

In other words, the whole currently existing isAvailable() method and the way it is used is bogus. The remark in #44.2 regarding the toolkit selection form, also suggests that removing toolkits based on a failing isAvailable(), is a bad idea.

Consequences for this patch:
- Let requirements reporting warn the user, but do not check availability at runtime over and over again. Remove the call to isAvailable()/availability() from the ToolkitManager::getAvailableToolkits(). Adapt/remove tests accordingly.
- Rename availability() to checkRequirements() and let it return the array as is normal with requirements checking in Drupal. To better comply with Drupal naming, just requirements() is also fine.

So not a +1 on the RTBC from my side. If others insist that this is RTBC, set it back to RTBC and I won't block it again.

I think that requirements should be checked on:
- submitting the toolkit selection form. But beware, this is a 2 step process, unless we ajax load toolkit specific form fields on selecting a different toolkit. So accept the new toolkit selection but display the failing requirements on the top of the page.
- status report

and not on:
- each instantiaton (as the current isAvailable() does).

I'm not against your point but it's simply out-of-scope here. That is not a reason to switch to "needs work", better open a follow-up with the form validate issue. Don't forget this one: #2111263: Toolkit setup form displays settings for multiple toolkits. Here we are just fixing a part of the problem (you mention it as "- status report"). You asked for small, reviewable patches, right? This one simply does this: replace GD hardcoded requirements with others provided by the active toolkit.

Let's move that discussion, about isAvailable(), etc., in a followup ticket. Can you open a new one please?

It is #35 that required us to handle both in this issue. I was and am fine with the patch from #32 and would be fine to have a renewed look at that isAvailable method in a separate (follow-up) issue. However, the way that patch #43 combines the 2 features/methods is not how I would have done it, but I won't block it either any longer.

@fietserwin, It's not about "blocking". The development process is extremely slow here. And I'm feeling that a lot of issues from our big list will be sooner D9 issues :( I don't want, in any way, to lose something in terms of consistency and architectural clearness so I'm not against getting the most clear and accepted approach. I'm not against your proposals but I want to see small steps/fixes getting in HEAD. Because I feel now that image is the poor member of the family. Just look in the commit flow and check how many image related commits you'll see.

However, the way that patch #43 combines the 2 features/methods is not how I would have done it, but I won't block it either any longer.

Yes, explain how you would have done it in a followup. I assume that the problem is exceeding the status report area (that is the original issue) and we need to take in discussion also isAvailable(), getAvailableToolkits() and getDefaultToolkit().

That's why now I'm reverting back and keep isAvailable() till we agree on a solution. I'm back now to #37 but moving the basic "Is the active toolkit available?" toolkit requirement outside of toolkit, in general requirements (this is taken from #43). This is satisfying also @alexpott's complain from #35. Now we are dealing here only with status report stuff and let's open a new issue for the other issue.

Yeah, this is the way to go. (aside: with blocking I meant "I won't set it to NW again to prevent the patch as was, from being committed".)

+++ b/core/modules/image/image.install
@@ -29,30 +29,29 @@ function image_uninstall() {
+  $toolkit = \Drupal::service('image.toolkit.manager')->getDefaultToolkit();

If the default toolkit is broken, this function will either return:
- another toolkit, if installed and available.
- false, if no other available toolkit is installed.

In the first case, we won't report any message about the selected toolkit being unavailable. So, IMO we should not use the function getDefaultToolkit(), as the return value is not enough for us to return a message that accurately describes the situation (and how to solve it). To solve, we should check the following situations:

1) No toolkit defined at all:

    $toolkit_id = \Drupal::config('system.image')->get('toolkit');
    if (empty($toolkit_id) { ... }

2) Active toolkit (according to config) is not satisfying requirements

      $definition = \Drupal::service('image.toolkit.manager')->getDefinition($toolkit_id)
      $requirements = call_user_func($definition['class'] . '::requirements', $phase);
      ...

Note: if requirements are not met, instantiating might fail. So requirements() should be static, as isAvailable() also is. This is perhaps far sought, but isAvailable() is static for probably the same reason. So we should do the same with requirements().

Created #2122605: Remove isAvailable() from ImageToolkitInterface as a follow-up issue.

@fietserwin, #51...

That approach anticipates again #2122605: Remove isAvailable() from ImageToolkitInterface or any other that is discussing how the current toolkit is obtained by getDefaultToolkit(). Right now, like it or not, the active toolkit is that returned by getDefaultToolkit() and not that configured in \Drupal::config('system.image')->get('toolkit'). Let's say that system.image.yml have toolkit: gd but GD2 extension is broken (not installed, etc.) and ImageMagick module is enabled and is satisfying all requirements. When cropping or scaling an image what toolkit is effective?

Good point, but users should be warned about that. Otherwise they might think they are working with GD, while actually, without any way of finding out, they are working with IM. (ok, they can find out in the status report: they can see, on a green background, that the toolkit used is IM, not the one they selected (if they remember or know which one was selected). But who looks at the green lines?)

Thus it is the duty of status report to warn users about:
- no toolkit selected at all (error in your config)
- the toolkit that was selected is not satisfying requirements and is thus not the one that is actually used. Within this situation, we could further distinguish between "no available toolkit at all", or "we have selected another toolkit for you, that is working". However, I'm not sure we should describe that behavior of the core provided toolkit manager in the status report.

Thus IMO the approach from #51 is more informative.

That is not in the scope right now. The active toolkit is that returned by getDefaultToolkit() and that should be tested for requirements and also if there's no default toolkit. Forget \Drupal::config('system.image')->get('toolkit') and UI selected for now. And, yes, that is a bug and may confuse users. But here is not the right place to tackle that!

This patch is, simply, performing next:

1. Check if there's an active toolkit. If not, is yelling with red color. Note: The check is performed in the actual buggy manner but this is other discussion.
2. If found an active toolkit (in 1.), displays possible warnings about possible misconfigurations.

And that's all. Accepting, temporary, how getDefaultToolkit() is selecting the active toolkit, what are your complains/observations/improvements on this issue?

We agree it is an error, and still you want to postpone solving the error to a follow-up issue. This while this issue can be considered a new feature, not only moving code around. E.g. your 2nd paragraph in #11 does not describe what patch #50 will do if IM is installed but not configured to be the default toolkit.

IMO, Drupal won't be better after committing #50, so no RTBC from me.

That's your point of view.

So, as explained in #2122605-4: Remove isAvailable() from ImageToolkitInterface, we need #50 in as a standalone patch for eventual backporting to D7, and will continue cleanup for D8 in #2122605: Remove isAvailable() from ImageToolkitInterface.

RTBC

50: toolkit-req-1069140-50.patch queued for re-testing.

50: toolkit-req-1069140-50.patch queued for re-testing.

1. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitInterface.php
   @@ -174,6 +174,20 @@ public function save(ImageInterface $image, $destination);
   +   * Gets toolkit requirements in a format suitable for hook_requirements().
   ...
   +  public function requirements();
   

   Can we name this getRequirements(), please?

   requirements() sounds as if an action would be performed.

2. +++ b/core/modules/image/image.install
   @@ -29,30 +29,29 @@ function image_uninstall() {
    function image_requirements($phase) {
   

   Follow-up issue:

   Image handling affects way more than just Image module - this requirements check should be moved into System module.

3. +++ b/core/modules/image/image.install
   @@ -29,30 +29,29 @@ function image_uninstall() {
   +      'value' => $toolkit ? $toolkit->getPluginId() : t('None'),
   

   Aside from the 'title' key, every other array element is different based on whether $toolkit exists.

   Whenever you face such a situation, it is better to use two separate if/else chunks; e.g., like this:

   $toolkit = ...;
   $requirements['image.toolkit']['title'] = t('Image toolkit');
   if ($toolkit) {
     $requirements['image.toolkit'] += array(
       ...
     );
   }
   else {
     $requirements['image.toolkit'] += array(
       ...
     );
   }
   

   Doing so also allows you to remove the early-return in the middle of the function.

4. +++ b/core/modules/image/image.install
   @@ -29,30 +29,29 @@ function image_uninstall() {
   +      'severity' => $toolkit ? REQUIREMENT_INFO : REQUIREMENT_ERROR,
   

   Severity should only be set in case of an abnormal condition (i.e., only upon error in this case).

5. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
   @@ -275,6 +275,27 @@ public function createTmp(ImageInterface $image, $width, $height) {
   +    $requirements['rotate_and_desaturate'] = array(
   +      'title' => t('GD library rotate and desaturate effects'),
   +      'value' => $info['GD Version'],
   +    );
   

   Previously, the version was output in the "main" image toolkit requirements row.

   Now the version is output next to the following label, which doesn't really make sense from a UI perspective:

   "GD library rotate and desaturate effects"

   Let's simply shorten that to "GD library"?

@sun,

Added most in the patch. See interdiff.txt.

#62.2:

Follow-up issue:
Image handling affects way more than just Image module - this requirements check should be moved into System module.

I have to disagree here. I believe in a minimal core able to run without any image functionality enabled. I fact the only point now that needs image functionality (but without image module enabled) is the editor inline images stuff. For details see #2117661: Disable inline images when image module is uninstalled and #2121155: Image upload validators should live in image module. There are Drupal applications, edge cases, where there's no need for any image feature.

#62.5:

Previously, the version was output in the "main" image toolkit requirements row.
Now the version is output next to the following label, which doesn't really make sense from a UI perspective:
"GD library rotate and desaturate effects"
Let's simply shorten that to "GD library"?

Right! I renamed the title and now it makes sense. Anyway, it seems that the requirements order is not OK (it would better if the general toolkit check come first) but this how requirements work now. Maybe a follow up for weighted or hierarchical requirements?

Looks good. Back to RTBC. Note that re. comment #35 from @alexpott, we have a follow up #2122605: Remove isAvailable() from ImageToolkitInterface, where we will also use the BrokenToolkit for testing.

63: toolkit-req-1069140-63.patch queued for re-testing.

63: toolkit-req-1069140-63.patch queued for re-testing.

Is there a base class for toolkits? If so we could put the return array(); implementation on there, would mean no changes at all for the test class.

Also not having this implemented by the base class it looks like there's no test coverage?

Currently there is no base toolkit, though one will be needed anyway for #2073759: Convert toolkit operations to plugins. What methods could have a realistic implementation in a base toolkit:
- getInfo()
- supportedTypes()
- settingsForm()
- settingsFormsubmit()

IMO getRequirements() does not have a realistic base implementation as all toolkits foreseen for D8 (GD and IM) will have to override it.

Thus, if we should add that base class in this issue is questionable to me.

Regarding the test coverage. There is no test covering the code, but the code is executed during test setup and, to get some manual test coverage, I tested it manually. Results:

- GD enabled: status report and image toolkit settings form are the same. Though a bit of overkill is seen due to all the sub-requirements that are displayed separately.
- GD disabled: basically the same error messages in the status report:

old:

new:

Both old and new look like overkill to me, but that is probably not the point of this issue.

We could anticipate here the implementation of ImageToolkitBase that we have foreseen in #2073759: Convert toolkit operations to plugins, however I agree with @fietserwin that implementing getRequirements() in the base class would not be so meaningful - GD and Imagemagick will have their own overrides of this method, as well as probably any other hypothetical toolkit (Imagick and Imagine would have to check for specific PHP extensions or external libraries). So a base class implementation would only be used by the test toolkit, as even the broken toolkit will need to have its own (failing) requirements to be able to test it as we are planning in #2122605: Remove isAvailable() from ImageToolkitInterface, where we will have the test coverage.

Trying to anticipate the tests would bring us in the weird situation where we should have isAvailable() returning TRUE (to have the toolkit instantiable), but getRequirements() returning REQUIREMENT_ERROR which is kinda contradicting.

So for me this is still RTBC, on the basis that as explained in #58 we still consider this issue to be potentially backported to D7, and that we have #2122605: Remove isAvailable() from ImageToolkitInterface to complete the cleanup and get the tests in.

Please set it to 'needs work' if still these explanations are not acceptable, indicating whether we should implement the base class and/or tests, and we'll continue from there.

An ImageToolkitBase is added in #2073759: Convert toolkit operations to plugins. However, I'm not against anticipating the abstract method here.

What else?

Forgot the status and Happy New Year :)

If we are adding a base class here, we should do it correctly, and for me that means that at least we should add these methods as well:

- getInfo() (the getimagesize() function does not depend on GD being available, so is a real (and the only one) base method)
- supportedTypes()

+++ b/core/modules/image/image.install
@@ -29,30 +29,38 @@ function image_uninstall() {
+        'description' => t("No image toolkit is configured on the site. Check PHP installed extensions or add other contributed toolkit that doesn't require a PHP extension and make sure that at least one valid image toolkit is enabled."),
         'severity' => REQUIREMENT_ERROR,

Not setting to NW to not interrupt the test (just to see if the use statements are changed correctly).
This message has been bothering me for a while. Can someone native English have a look at it? For me, something like "No valid image ... or add another ..." are the minimal changes needed to this sentence.

70: toolkit-req-1069140-70.patch queued for re-testing.

Needs reroll after #2161955: Remove get_extension_funcs() call to improve HHVM compatibility . Re. #72, IMHO a full implementation of a base toolkit class should be matter of a separate follow-up.

Happy New Year all!!

@fietserwin,

If we are adding a base class here, we should do it correctly, and for me that means that at least we should add these methods as well:

This shouldn't block this issue and it's out of scope. But I agree that needs our attention. Let's not add more "noise" to this simple issue but fix that in a followup: #2165399: Add basic methods to ImageTookitBase.

Rerolled. Other aspects (from #72):

"No image toolkit is configured..." vs. "No valid image toolkit is configured..."

The second can be read as there is an invalid toolkit configured. This is a lie because we are testing there the case if there's no toolkit at all.

"... or add other..." vs. "...or add another..."

I think none are correct. I went with "...or add a..." which describes the best our case. I also split the long phrase to be more readable.

Base class methods were moved to a followup in #2165399: Add basic methods to ImageTookitBase.

Please set back to RTBC if turns green.

#78: No, there is always a toolkit configured. In #68 I tested the case where the GD toolkit was configured but the GD extension was disabled and I got the message "No toolkit is configured ..." is thus wrong and misleading.

Ah, your're right because of buggy getDefaultTookit(). But this will change in #2122605: Remove isAvailable() from ImageToolkitInterface where we'll distinguish between: (1) not configured; (2) configured but not available and (3) configured, available but not with full features.

Let's not block again this because of that "valid" word addition because it's not even 0,001% relevant now. Thank you!

Yes, let's move on. Base class ImageToolkitBase is now implemented with a getRequirements() method, as requested in #67.

Back to RTBC.

+++ b/core/modules/image/image.install
@@ -29,30 +29,38 @@ function image_uninstall() {
+  foreach ($toolkit->getRequirements() as $key => $requirement) {
+    $namespaced_key = 'image.toolkit.' . $toolkit->getPluginId() . '.' . $key;
+    $requirements[$namespaced_key] = $requirement;
   }

Why don't we move the foreach up into the the if. I missed the return $requirements; in the else above. Multiple return points add unnecessary complexity especially when you already have the if ($toolkit) {. Sorry for such minor feedback but I spent 5 minutes thinking isn't this going to generate PHP errors before spotting the return in the else. Apart from that this looks good to go and I'd be happy with a straight rtbc after moving that code inside the if and removing the return $requirements; from the else.

@alexpott, I think that is more a matter of developer's coding style. I like multiple exit points because it reduces the number of nesting indents in IDE, making code more readable. I see that other developers are complaining for this way of coding. Matter of taste, I think :)

Anyway, here's the patch.

@claudiu.cristea yes but you already had the if so there no additional levels of nesting so in this case multiple return points is always going to add confusion.

Committed 52eb936 and pushed to 8.x. Thanks!

Change notice at https://drupal.org/node/2166575.

This needs to be ported in D7.

D7 patch.

Added a placeholder issue in ImageMagick #2166787: Toolkits are able declare their requirements.

Looks good, though there is a UI text change. But as this is pure admin, I wouldn't mind being it committed.

1. I applied this patch and am confused by the before and after screenshots.

   Before:
   

   After:
   

   Why is the toolkit machine name ("gd") being displayed in the user interface?

   And why is the line that previously said "GD library rotate and desaturate effects" now something much less informative ("GD library")?

2. +    $function = 'image_' . $toolkit . '_get_requirements';
   +    if (is_callable($function)) {
   

   I think this new toolkit callback needs to be documented; see the hook_image_toolkits() documentation where all the other similar callbacks are documented.

   Also wondering why is_callable() is used here rather than simply using function_exists(), but that's pretty minor.

3. -        $requirements['image_gd']['description'] = t('The GD Library for PHP is enabled, but was compiled without support for functions used by the rotate and desaturate effects. It was probably compiled using the official GD libraries from http://www.libgd.org instead of the GD library bundled with PHP. You should recompile PHP --with-gd using the bundled GD library. See <a href="http://www.php.net/manual/book.image.php">the PHP manual</a>.');
   ....
   +      $requirements['version']['description'] = t('The GD Library for PHP is enabled, but was compiled without support for functions used by the rotate and desaturate effects. It was probably compiled using the official GD libraries from http://www.libgd.org instead of the GD library bundled with PHP. You should recompile PHP --with-gd using the bundled GD library. See <a href="@url">the PHP manual</a>.', array('@url' => 'http://www.php.net/manual/book.image.php'));
   

   I think it's OK for this patch to break translatable strings where necessary (since they're all admin-facing anyway) but we should keep it to an absolute minimum, and the above string is not something that needs to change.