Remove isAvailable() from ImageToolkitInterface

Few comments. This method is used only once, in ImageToolkitManager::getAvailableToolkits(), to filter out broken toolkits:

$ grep -nr --exclude-dir=core/vendor  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:299:  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() {

The interesting part here is, IMO, how the system is picking up the active toolkit. Right now Drupal is:

1. Checking the toolkit stored either by install process, either by site admin in system.image.toolkit and use it if it's valid.
2. If is not valid, it picks up the first, arbitrary valid toolkit.

This behavior was inherited from the legacy Drupal 7 functions: image_get_toolkit() and image_get_available_toolkits().

Here we have a problem. I don't think that the system should assign an arbitrary toolkit. This would hide the failure. I think the site admin should be warned that the configured toolkit is either uninstalled (if the provider module is no more enabled), either broken (something occurred on the platform — e.g. GD2 extension was uninstalled). Course this check should be performed also when the admin submits the toolkit form in admin/config/media/image-toolkit.

What I think now is that this issue must be more an issue about getDefaultToolkit() than isAvailable(). The functionality of isAvailable() should be triggered only once, against the config stored toolkit and not all toolkits. I really don't know if this should be cached or not. At least for GD toolkit it doesn't seems an expensive operation (checking a function existence) but, in terms of performance, we already gained by not checking each toolkit. Probably we'll need to check each toolkit availability in admin/config/media/image-toolkit but still display all toolkits together with a warning message attached to broken ones — so that site admin is able to see the "whole picture". So, the active (configured) toolkit validity check may occur:

1. On each instantiation with exception throw (plus form validation and critical requirements). The most expensive (but less expensive than the current implementation).
2. On each instantiation but with cached value of availability (plus form validation and critical requirements). Less expensive but needs a cache invalidation mechanism and opportunity (cron?).
3. Without any check at runtime, only form validation and critical requirements. The most cheap solution.

A last note: While per-toolkit requirements is returning a classical hook_requirements() array and not other availability information, how we'll remove isAvailable()? Regardless of the solution we follow, we still need to check the availability (e.g. toolkit form validation, when setting the "is available" cache, in image_requirements(), etc).

My opinion: let's wait till #2111263: Toolkit setup form displays settings for multiple toolkits gets committed, then go with a single patch to address both this issue and #1069140: Requirements should be provided by image toolkit, built on top of patch in #1069140-50: Requirements should be provided by image toolkit. We can also adjust the Imagemagick conversion in the sandbox at the same time so to have two toolkits to play with.

Based on all the discussion, it seems to me that the key player in terms of checking the requirements would be the toolkit setup form, and a specific toolkit being set as default only on its requirements pass. Then the status report would be more or less 'subsidiary' i.e. a reference to check if something suddenly stops working - and that would happen only if something changed in the underlying setup (GD extension removed, path to convert changed, etc.).

Then I would go with

Without any check at runtime, only form validation and critical requirements. The most cheap solution.

from #1: effects already fail with a watchdog entry if the toolkit call fails.

Why everybody want to mix #1069140: Requirements should be provided by image toolkit with other issues, like this one? #1069140: Requirements should be provided by image toolkit is a D7 issue but moved to D8 according to the rule that an issue must be fixed first in the development release and after that back-ported. That one has a specific scope: Stop hardcoding requirements from GD, let the active toolkit scream! If, hypothetically, this issue will never be committed, #1069140: Requirements should be provided by image toolkit is still a valid, standalone issue, just good to get in D8 and back-ported to D7. Because is doing only that simple thing: passes the requirements generation from image to toolkit. The only "sin" of #1069140: Requirements should be provided by image toolkit is that is using that damn getDefaultToolkit() to get the active toolkit. But that's how we are selecting now the toolkit. So that is ready to go as it is because D7 is waiting for it too.

Here, in this issue, is the place were we need to discuss and fix getDefaultToolkit(), along with others. And if #1069140: Requirements should be provided by image toolkit is committed first we just need to touch again image_requirements() and use maybe the config value instead that method. But that change is not related to how requirements are built but is related with this issue. That's why, even touching again image.install, that change belong to this issue.

added that we should agree on solution

Here's a proposal, built on top of #2111263: Toolkit setup form displays settings for multiple toolkits and of #1069140: Requirements should be provided by image toolkit.

- drop the isAvailable() method from the toolkit.
- adds to the requirements() method a $form_state parameter, being the form_state of the Image toolkit setup form, to allow for bypassing stored config values while the toolkit is being setup (e.g. to change the path in ImageMagick).
- drop the getAvailableToolkits() method from the toolkit manager. Just use getDefinitions() instead, which bears all the toolkit plugins found (regardless if valid or not).
- getDefaultToolkit() in the manager will only return the configured toolkit. Will fail with an error message if no toolkit configured or toolkit is no longer existing (e.g. uninstalled).
- most of the checking is done via the toolkit setup form, at form validation. If this passes, we assume that the toolkit is configured properly, and make no runtime checks.
- the status report will report any error occurred 'outside' of Drupal toolkit configuration (e.g. change in PHP GD library, change in ImageMagick path).

do-not-test is diff against #2111263: Toolkit setup form displays settings for multiple toolkits and of #1069140: Requirements should be provided by image toolkit, the other is against HEAD.

Placing all of "Image toolkit configuration page" within the anchor tags

done

Not necessary to check for empty...

done

replace the return FALSE by throwing an exception to fail more clearly

there are only two cases when getDefaultToolkit is used. One is in image.install, and if a FALSE value is returned, then it is managed. The other is more complex as it is connected to the instantiation of the image.factory service: here the default toolkit is looked for and instantiated as well, then passed on to the Image object. Not sure what will happen if we just throw an exception here. For the time being I just allowed the image.factory constructor to accept a FALSE value if no default toolkit is found. To me it seems we may need to sort this out with try/exception/catch in #2105863: [meta] Images, toolkits and operations in the interaction between Image and Toolkit - i.e. let an Image be created with no toolkit and catch the exceptions that Image will get into by calling a missing toolkit.

BTW - I adjusted the Imagemagick conversion in the sandbox, to work with this patch. You can play with that to test if you wish.

brackets not necessary

done

Does the toolkit have a name that we can display to the user?

done

Can an image exist without Toolkit?

I think not. For the time being I changed ImageFactory::get() to return NULL if no toolkit is selected. There are a few points here:

1. More general. I just recently realised that the ImageToolkit is instantiated at the ImageFactory level, then the same instance is passed on to any Image being created. I think this is wrong when we look at #2103621: Move GD logic from ImageInterface to toolkit, tie toolkit instance to Image instance, toolkits should no longer be instantiated separately: if we move the resource object (in GD) and the ops array (in ImageMagick) to the toolkit, then each image should isolate its own instance of the toolkit. Suppose we 'get' two images in parallel, we cannot mix the resource/ops of each. I'd say this one needs to be tackled more in #2103621: Move GD logic from ImageInterface to toolkit, tie toolkit instance to Image instance, toolkits should no longer be instantiated separately.
2. If we accept that getDefaultToolkit can return NULL, then we cannot typehint the __construct in ImageFactory, otherwise unit tests will fail (NULL is throwing a recoverable fatal). setToolkit() we can, as we assume that the toolkit object will be checked before invoking it.
3. If we accept that ImageFactory::get() can return NULL if no toolkit is set (which I believe it's sane), then currently in the codebase there are no checks that the returned Image object is valid. This leads to WSOD, e.g. in the image style preview. If we agree this is the way forward, then we need to add this checks (not done here).

I cannot review a "3 issues" combined patch. Can we have also a separate "do not test" patch only for this issue?

1. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitManager.php
   @@ -48,47 +48,18 @@ public function __construct(\Traversable $namespaces, CacheBackendInterface $cac
   +      drupal_set_message(t('The Image toolkit is missing or not configured properly. Visit the <a href="@url">Image toolkit configuration page</a> to correct this.', array('@url' => url('admin/config/media/image-toolkit'))), 'error');
   

   We don't want to popup anything here because we cannot anticipate where we'll use this manager method. Retuning NULL should be enough but I think even to throw here an exception instead.

2. +++ b/core/modules/system/lib/Drupal/system/Form/ImageToolkitForm.php
   @@ -65,34 +67,91 @@ public function getFormId() {
   +  /**
   +   * {@inheritdoc}
   +   *
   +   * Validates the toolkit selection, by checking toolkit requirements.
   +   */
   

   It seems that in code standards we cannot use both {@inheritdoc} and custom docblock content. See {@inheritdoc}.

3. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitInterface.php
   @@ -174,12 +174,24 @@ public function save(ImageInterface $image, $destination);
   +   * @param array $form_state
   +   *   (optional) The current form state if the method is invoked from
   +   *   within the Image toolkit setup form. Allows the method to bypass stored
   +   *   config values if the parameters in the form are relevant to check the
   +   *   requirements.
   ...
   +  public function requirements(array &$form_state = array());
   
   +++ b/core/modules/system/lib/Drupal/system/Form/ImageToolkitForm.php
   @@ -65,34 +67,91 @@ public function getFormId() {
   +  public function validateForm(array &$form, array &$form_state) {
   

   I don't like, at all, the idea to mix requirements with form validation. Especially, to send $form_state as argument into requirements(). The form is an UI element while requirements() is most something placed at the model level. I would let each toolkit simply provide its own form validation callback that should be called when validating — as the submit callback is called now. That one has the possibility to call its own requirements but ONLY if he decides that.

4. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
   @@ -275,14 +275,33 @@ public function createTmp(ImageInterface $image, $width, $height) {
   +    $check = get_extension_funcs('gd');
   +    if (!$check || !in_array('imagegd2', $check)) {
   +      $requirements['image_gd'] = array(
   +        'title' => t('GD Library'),
   +        'value' => t('Not installed'),
   +        'description' => t('The GD library for PHP is missing or outdated. Check the <a href="@url">PHP image documentation</a> for information on how to correct this.', array('@url' => 'http://www.php.net/manual/book.image.php')),
   +        'severity' => REQUIREMENT_ERROR,
   +      );
   +      return $requirements;
   

   Hm... See my final comment.

As generic comment: Even this issue goal seems to be "Drop isAvailable()!", I'm not convinced anymore that we need that removal. I see 3 level of errors here:

1. No toolkit configured (system.image.toolkit is empty). This one is already treated in #1069140: Requirements should be provided by image toolkit.
2. The configured toolkit cannot be used at all. That one is handled now by isAvailable() and I think we can live with it. Action to take:
   • Change the severity of the toolkit requirement (image.toolkit) and add a proper description — handled in image_requirements()
   • Validation error in form
3. The configured toolkit can be used but has limitations/warnings. Requirements already provided in #1069140: Requirements should be provided by image toolkit, plus we need to set specific form validations on each toolkit.

#13, here's the do not test patch.

I don't like, at all, the idea to mix requirements with form validation.

You're right. Changed approach here.

in code standards we cannot use both {@inheritdoc} [...]

Corrected.

Retuning NULL should be enough but I think even to throw here an exception instead.

Not sure how to do that. Can you advise?

To the overall comment. I find having both isAvailable() and requirements() confusing. So I'd prefer if we can drop isAvailable().

@fietserwin thanks. New patch tomorrow, with an ImageToolkitException extending PluginException.

Here, ImageToolkitManager::getDefaultToolkit() returns null if it the default plugin is not set or missing, and ImageFactory::get() throws a new ImageToolkitException if the toolkit is not set.

#20: OK, here we go. ImageFactory is now back to original. We have to try/catch getDefaultToolkit in image.install to manage invalid default.

Hm... #21 fails GD Toolkit unit test as there is no default toolkit set in config in that context, and the new exception is thrown on

  $image_factory = $this->container->get('image.factory')->setToolkit($toolkit);

at get('image.factory'), before the toolkit is reset in setToolkit($toolkit).

In other words, we can not instantiate the ImageFactory at all if there is not a configured toolkit, and we can set a different toolkit for the ImageFactory only if this is already instantiated. This is the effect of moving the exception throwing at the toolkit plugin level instead on ImageFactory::get().

Here I changed the test so to set explicity 'gd' as the default toolkit in config.

#25 no, here we do not change the toolkit to the Image, we change it to the ImageFactory, so that if we get() an image from that factory, it will 'inherit' the toolkit set at that point (OK, in #11.1 we say there's a problem with 'inheriting' that toolkit instead of getting a new instance, but will address that elsewhere). I believe it's valid to provide developers the possibility to manage images with toolkits alternative to the default one; even if core won't use that.

Reroll, after #1069140: Requirements should be provided by image toolkit went in, and with latest patch from #2111263: Toolkit setup form displays settings for multiple toolkits.

This still requires #2111263: Toolkit setup form displays settings for multiple toolkits before being reviewable, so I'll mark it postponed after the test run.

Ooops...

Needs to wait for #2111263: Toolkit setup form displays settings for multiple toolkits

#2111263: Toolkit setup form displays settings for multiple toolkits landed, so we can reopen this. Needs reroll and re-thinking about how to manage ImageToolkitForm validation.

Things that should be fixed here:

1. Fix getDefaultToolkit().
2. Errors should follow the next precedence:
3. • No toolkit configured => system.image.toolkit is empty.
   • Toolkit configured but invalid => isAvailable(): (1) needs validation form error — invalid toolkits cannot be selected as default and (2) proper Status Report message — it might become invalid after selection.
   • Toolkit configured, available but has limitations/warnings. Already fixed.

Didn't have a position now on form validation.

As a conclusion: I think that we should keep isAvailable() as a hard flag, that tells if a toolkit is valid. Basically it's below the bottom line of requirements.

@fietserwin,

#33.1: Agree but as a followup to have a clear distinction between issues. getActive() seems the more correct option.

#33.2: I don't understand exactly what would be the reason for accessing other toolkit than the active one? Can you show us a use case? Do you suggest that modules will be able to use Drupal with more than one toolkit in the same time? Or? Will the ImageToolkitManager::get($id) method be an alias of ImageToolkitManager::createInstance($id)? If yes, why not using that straight method?

#33.3: Agree with getAvailableToolkits() removal but I don't understand what you mean by "Drupal has a good requirements checking and reporting system, at install time, selection time and via the status report". So you want to replace isAvailable() method with per-toolkit arbitrary checks? Why?

#33.4: I'm not sure that I understood your point 2. "So ImageToolkits should not crash in their constructor...". Is crashing now in constructor? Or? Confused about this explanation. Can you illustrate this with a scenario or a piece of code?

What I feel is that you made strong scope from removing isAvailable(). I'm not against but also not in favor. I don't see why this unified edge check should be replaced with per-toolkit arbitrary checks. Give me a reason.

Also, let's synchronise our effort with the very slow progress of committing our work. Trying to radically change the image system (e.g. by letting multiple active toolkits) can ruin the whole plan.

Basically just a reroll of #28, plus with the form validation logic moved to the new ImageToolkitBase. Only the currently selected toolkit is validated, however all the toolkits' submits handler are invoked.

This is just to set a new starting point; I did not review #32 and #33 yet.

NW anyway to deal with #32, #33. Also unassigned myself as I am not sure about the time I can give to this in the short term.

This patch would introduce methods that overlap with those in #2096703: Image toolkits should use PluginFormInterface and ContainerFactoryPluginInterface. It'd be better have that one first. In any case the patch in #35 would need serious rework at this stage.

Thank you for creating this issue to improve Drupal.

We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

believe to still be valid as isAvailable() is still part of this interface