Improve the performance of get remote image size

Thanks for this contribution. I think it would be a great addition, but am having difficulties with adding a mit licensed file to this Drupal repository. What is currently the best way (in D8) to add an external php library? Add it to our composer.json as a requirement? Is that enough to have it installed when people are installing this module via the UI? Or would that need a dependency on e.g. the Composer Manager module or the Libraries API?

If we are going this way, could we make it a soft dependency, thus e.g. first checking if the class exists? This way we can maybe save a lot of hassle for maintainers of a small site where the performance gain of this run once tool is of less importance?

What do you think?

Note that the github project that is used in this patch has been registered on packagist under https://packagist.org/packages/fastimage/fastimage and thus is available via composer.

Thanks for updating the patch. I changed it a bit by making $is_fast_image a static variable within image_field_repair_get_image_size(), so that this optimization is not exposed. The attached patch also contains some documentation updates (typo,missing @param), that will go in at the same time.

One more thing before committing: how to document this speed optimization? I guess we want this explained on the project page and in the readme.md (both are the same for now) with something like this:

Installation
============
If besides installing this module, you also install the library [fastimage/fastimage](https://packagist.org/packages/fastimage/fastimage) you can considerably speed up the process as this library can get image dimensions much faster than the internal PHP function getimagesize().
- If you install this module using composer, composer will download the library [fastimage/fastimage](https://packagist.org/packages/fastimage/fastimage) which speeds up the process as it considerably faster gets the dimensions of an image as the internal PHP function getimagesize().
- If you install this module manually or via the Drupal UI the library will not automatically be installed as well. If you have lots of images to check, we suggest that you install this library yourself using the command "composer require fastimage/fastimage".

Can you have a look a t that text and suggest any improvements you may see?

BTW: the syntax for suggests in composer.json is as follows:

The format is like package links above, except that the values are free text and not version constraints.

Example:

{
"suggest": {
"monolog/monolog": "Allows more advanced logging of the application flow",
"ext-xml": "Needed to support XML format in class Foo"
}
}

Thus no version indication but a reason why you may want to install the suggested package. I'm also not sure if our suggested package gets installed automatically when installing/requiring our module.

I changed the help texts as suggest does not automatically install the suggested library and I added the text also to the form.

However upon testing I found a few errors:
- The class name is Fastimage not Fastimage and that fooled class_exists().
- Fastimage fails to recognise a jpeg file ?! The first 2 bytes are 255 and 216 but after the mb_convert_encoding($result, "8BIT"); at the end of the getChars() method, this changes into 255 and 24 and thus subsequently the getType() method fails to recognise jpeg.

The latter is an error in the library which I cannot correct, so I have to put this one on hold. We either have to use another library or get it fixed. I attached the current state of this patch that now also covers for a failing Fastimage::getSize().

OK, good research on alternatives.

I think that the best we can do now is sort of go back to your patch from #2: let's take the orignal code from Tom Moor, just the class, not the readme, examples or whatever and add it to our module. This solves our bug. However the gr0g code has 1 addition: adding http to uris starting with //. I am not sure we need that, that is: can managed files start with this? I guess yes, so better add http or https based on what we are currently running in.

Looking at the MIT license we can do so if we leave the copyright messages at the top of the file:

/**
 * FastImage - Because sometimes you just want the size!
 * Based on the Ruby Implementation by Steven Sykes (https://github.com/sdsykes/fastimage)
 *
 * Copyright (c) 2012 Tom Moor, http://tommoor.com
 *
 * This port changes/adds:
 * - Adhering to Drupal coding standards
 * - Adhering to protocol wrapper syntax: //... is changed into http://... or https://...
 */

class FastImage
{
...

According to the Drupal license faq point 2, we may incorporate MIT licensed work and redistribute it under GPL.

Thanks for all the work. I committed the last patch with some changes: using PHPStorm it is easy to reformat a file to Drupal coding standards. Furthermore, I am not a big fan of static classes, but I left ImageSizer as is

NOTE: I wrote this while you were posting the patch:
Interesting read. But if we have a file name, plain or in wrapper syntax, can we check if it is local (plain file name or a stream wrapper deriving from Drupal\Core\StreamWrapper\LocalStream) and subsequently decide what to use?

Because for sites with (mostly/only) local storage but enormous amounts of images, this may be a dramatic performance fallback after all...

EDIT (added this after seeing your patch):
I think your patch ignores stream wrappers around e.g. cloud storage, not sure they already exist for D8, though https://www.drupal.org/project/remote_stream_wrapper seems to do so.

• realpath() won't work for remote stream wrappers
• getimagesize() accepts stream wrappers, so no need to use realpath() anyway, we just want to know if it is a local stream or not.
• Why a config setting here, while the switch is on the form that starts the batch? Is it still needed with only a factor 1.2 versus 3?

See \Drupal\Core\StreamWrapper\StreamWrapperInterface::realpath and its real implementation on \Drupal\Core\StreamWrapper\LocalStream and its FALSE returning implementation on \Drupal\file_test\StreamWrapper\DummyRemoteStreamWrapper.

So I think that something like the provided interdiff would be better regarding handling wrappers. I don't know the performance cost of repeated \Drupal::service('stream_wrapper_manager') calls but I placed that manager in a local (static) variable.

OK, some further reading and testing about local and remote stream wrappers taught me:

• We can leave out the wrapper creation and just look at the type of a scheme wrapper (Drupal defined schemes)
• But it turns out there's also a built-in PHP function: stream_is_local($uri)
• stream_is_local() works fine with just passing in scheme:// but it might emit warnings like "Warning: stream_is_local(): Unable to find the wrapper 'private' - did you forget to enable it when you configured PHP?" for non registered schemes.
• stream_is_local('file://') returns true, while stream_is_local('file://test.php') returns false !!!

So, I created an array that keeps track of schemes and its locality and initializes it with the most common schemes (abusing the last remark ...)

What do you think of this patch (no UI, thus possibly slower on sites with many local large files). If you think we still need a UI, let's then do so in a separate patch.

Oops, I was a bit in a hurry to finish it, and after using the Drupal stream wrapper related classes, I found out abut stream_is_local(), so quite some refactorings. So, yes should store the scheme not the uri:

  private function isUriLocal($uri) {
    $scheme = $this->file_system->uriScheme($uri);

    if ($scheme) {
      if (!isset($this->scheme_is_local[$scheme])) {
        $this->scheme_is_local[$scheme] = @stream_is_local($uri);
      }
      $result =  $this->scheme_is_local[$scheme];
    }
    else {
      // Plain old (local) file path.
      $result = TRUE;
    }
    return $result;
  }

- FileSystem::uriScheme() returns scheme without the :// (and checks for the ://, not only :) and FALSE if there's no scheme part.
- explode(':"' ...) would store the uri if there's no scheme

Thanks for testing. committed

Thanks for warning me. Should be restored by a merge commit.