GD toolkit & operations should catch \Throwable to fail gracefully in case of errors

I think that returning a catchable exception is preferable over a fatal error. So this issue is worth implementing.

Below first some remarks about the patch as is, after that some remarks about some design questions.

1. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitBase.php
   @@ -138,6 +138,10 @@ public function apply($operation, array $arguments = array()) {
   +    catch (\RuntimeException $e) {
   +      $this->logger->warning($e->getMessage(), []);
   

   This exception type should be docuemnted with a @throws in core\lib\Drupal\Core\ImageToolkit\ImageToolkitOperationInterface.php::apply().

2. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -181,6 +201,11 @@ protected function load() {
   +      throw new \RuntimeException("Not enough memory (required: " . format_size($this->getRequiredMemory()) . ") to load image '{$path}'");
   +    }
   

   $path is unknown, did you want to use $this->getSource()?

3. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -436,4 +461,41 @@ public function extensionToImageType($extension) {
   +   * @return bool
   +   *   TRUE if there is sufficient memory to allow the operation.
   +   *
   +   * @throws \RuntimeException
   +   *   When the available memory is not sufficient.
   +   *
   

   When there is insufficient memory, this method will return FALSE, not throw an exception

4. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -436,4 +461,41 @@ public function extensionToImageType($extension) {
   +    $this->memoryRequired = (int) ($width * $height * ($bits_per_pixel / 8) * $tweak_factor);
   +    $usage = memory_get_usage(TRUE);
   +    $memory_limit = ini_get('memory_limit');
   +    $size = Bytes::toInt($memory_limit);
   +    return Environment::checkMemoryLimit($usage + $this->memoryRequired, $memory_limit);
   +  }
   

   - memoryRequired is not a defined property, whereas requiredMeory is ...
   - $size is not used: remove
   - $memory_limit does the same as Environment::checkMemoryLimit() already does for you: remove and do not pass

5. +++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/CreateNew.php
   @@ -84,6 +84,11 @@ protected function validateArguments(array $arguments) {
   +      throw new \RuntimeException("Not enough memory (required: " . format_size($this->getToolkit()->getRequiredMemory()) . ") to perform the image '{$this->getPluginId()}' operation");
   +    }
   

   format_size() can do a translation while exception messages should not be translated. In this case doing unformatted output of the number might be better.

6. +++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Rotate.php
   @@ -98,6 +98,16 @@ protected function execute(array $arguments) {
   +    // can never exceed the lenght of the diagonal of the rectangle represented
   +    // by the image.
   

   typo: length

7. +++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Rotate.php
   @@ -98,6 +98,16 @@ protected function execute(array $arguments) {
   +    $diagonal = (int) sqrt(pow($this->getToolkit()->getWidth(), 2) + pow($this->getToolkit()->getHeight(), 2)) + 1;
   +    if (!$this->getToolkit()->checkAvailableMemory($diagonal, $diagonal)) {
   

   You could use floor() instead of adding 1 yourself.

8. +++ b/core/modules/system/src/Tests/Image/ToolkitGdTest.php
   @@ -433,6 +433,15 @@ public function testResourceDestruction() {
   +    $image->createNew(20000, 20000);
   +    $new_res = $image->getToolkit()->getResource();
   

   Shouldn't checking the return value (FALSE) be part of the test?

These remarks are more at the design/architectural level:

1. We have this utility class Environment that checks the memory limit. If I look at the usages, it should also contain a method checkAvailableMemory() (or isMemoryAvailable()?), because most usages (including ours) check that, not the limit as is.
2. GDToolkit now has a property requiredMemory. To me that doesn't feel good: it is not a property of a toolkit but of an operation. We should strive to refactor that property away, by e.g. returning the missing amount of memory instead of just TRUE/FALSE (or allowing to pass a parameter by reference that gets that value; or by having checkAvailableMemory() throw the exception (and thus passing in an operation name or something like that.

Thanks for processing the remarks from my st review. This version is almost there, mostly minor remarks remain.

1. +++ b/core/lib/Drupal/Component/Utility/Bytes.php
   @@ -44,4 +44,33 @@ public static function toInt($size) {
   +   *   A string size expressed as a number of bytes or multiples.
   +   */
   

   Document that this one is untranslated (and refer to format_size() for a translating version of this feature).

2. +++ b/core/lib/Drupal/Component/Utility/Bytes.php
   @@ -44,4 +44,33 @@ public static function toInt($size) {
   +    if ($size < Bytes::KILOBYTE) {
   +      return $size . ' B';
   +    }
   +    else {
   +      $size = $size / Bytes::KILOBYTE;
   +      $units = ['KB', 'MB', 'GB', 'TB', 'PB', 'EB', 'ZB', 'YB'];
   +      foreach ($units as $unit) {
   +        if (round($size, 2) >= Bytes::KILOBYTE) {
   +          $size = $size / Bytes::KILOBYTE;
   +        }
   +        else {
   +          $size = round($size, 2);
   +          break;
   +        }
   +      }
   +      return $size . ' ' . $unit;
   +    }
   +  }
   

   This can be simplified to something like:

       $units = ['B', 'KB', 'MB', 'GB', 'TB', 'PB', 'EB', 'ZB', 'YB'];
       foreach ($units as $unit) {
         if (round($size, 2) < Bytes::KILOBYTE) {
           break;
         }
         $size = $size / Bytes::KILOBYTE;
       }
       return $size . ' ' . $unit;
    

   Advantages:
   - less cases (= less clutter).
   - a value in the YB range will also be rounded to 2 digits.

   (However: if this is a mere copy of format_size(), you can leave it as is).

3. +++ b/core/lib/Drupal/Component/Utility/Bytes.php
   @@ -44,4 +44,33 @@ public static function toInt($size) {
   +      $size = $size / Bytes::KILOBYTE;
   +      $units = ['KB', 'MB', 'GB', 'TB', 'PB', 'EB', 'ZB', 'YB'];
   

   self::KILOBYE (or even static::KILOBYTE) like in the method above? (4 usages).

4. +++ b/core/lib/Drupal/Component/Utility/Environment.php
   @@ -42,4 +42,33 @@ public static function checkMemoryLimit($required, $memory_limit = NULL) {
   +   * @param string $required
   +   *   The memory required for the operation, expressed as a number of bytes with
   +   *   optional SI or IEC binary unit prefix (e.g. 2, 3K, 5MB, 10G, 6GiB, 8bytes,
   +   *   9mbytes).
   +   * @param string $memory_limit
   

   2 lines with 81 characters.

5. +++ b/core/lib/Drupal/Component/Utility/Environment.php
   @@ -42,4 +42,33 @@ public static function checkMemoryLimit($required, $memory_limit = NULL) {
   +  public static function checkAvailableMemory($required, $memory_limit = NULL, $used_memory = NULL) {
   +    if (!$used_memory) {
   

   To me this is a wrapper around checkMemoryLimit() for a special case: check if there is enough memory available within the current process space. So remove the optional parameters: if you need one, use checkMemoryLimit() directly.

6. +++ b/core/tests/Drupal/Tests/Component/Utility/EnvironmentTest.php
   @@ -64,4 +65,49 @@ public function providerTestCheckMemoryLimit() {
   +  public function testCheckAvailableMemory($custom_memory_limit, $used_memory, $required, $expected) {
   +    $actual = Environment::checkAvailableMemory($required, $custom_memory_limit, $used_memory);
   +    $this->assertEquals($expected, $actual);
   +  }
   +
   

   If the parameters are removed, you can test this by using ini_get('memory_limit') and memory_get_usage(TRUE) and test a value above and below the difference.

1. The failed tests was because I forgot to round the value that will be returned. This code does pass:

    $units = ['B', 'KB', 'MB', 'GB', 'TB', 'PB', 'EB', 'ZB', 'YB'];
    foreach ($units as $unit) {
      if (round($size, 2) < Bytes::KILOBYTE) {
        break;
      }
      $size = $size / Bytes::KILOBYTE;
    }
    return ['unit' => $unit, 'size' => round($size, 2)];

Note that theoretically this code is better as the current code does rounding on rounding on rounding which may lead to false results: add 4053371676 as a test, this is 3,774996545 GB but 3,78 is currently returned...

2. Now that we are touching common.inc: format_size() has the same unnecessary if - else, this can be simplified to:

function format_size($size, $langcode = NULL) {
  $options = ['langcode' => $langcode];
  $val = Bytes::toUnitAndSize($size);
  $args = ['@size' => $val['size']];
  switch ($val['unit']) {
    case 'B':
      return \Drupal::translation()->formatPlural($val['size'], '1 byte', '@count bytes', [], $options);
    case 'KB':
      return new TranslatableMarkup('@size KB', $args, $options);
    case 'MB':
      return new TranslatableMarkup('@size MB', $args, $options);
    case 'GB':
      return new TranslatableMarkup('@size GB', $args, $options);
    case 'TB':
      return new TranslatableMarkup('@size TB', $args, $options);
    case 'PB':
      return new TranslatableMarkup('@size PB', $args, $options);
    case 'EB':
      return new TranslatableMarkup('@size EB', $args, $options);
    case 'ZB':
      return new TranslatableMarkup('@size ZB', $args, $options);
    case 'YB':
      return new TranslatableMarkup('@size YB', $args, $options);
  }
}

3. Regarding 11.5/6 and your 12 5/6:
What you are testing in method providerTestCheckAvailableMemory() is not checkAvailableMemory() but checkMemoryLimit(). As checkAvailableMemory() is just a wrapper, the test should check that checkMemoryLimit() does get called with the correct values. If you can't mock this, do a test like:

$available = ini_get('memory_limit') - memory_get_usage(TRUE);
// Function call, parameter stacking, etc may snoop away some memory: subtract a few KB.
$this->assertTrue(Environment::checkAvailableMemory($available - 4000));
$this->assertFalse(Environment::checkAvailableMemory($available + 1));

BTW: Does http://stackoverflow.com/questions/2357001/phpunit-mock-objects-and-stat... give some useful insights on mocking statics?
BTW2: You could: override Environment for testing purposes; change self:: into static:: (:)); and override checkMemoryLimit() to log/return its parameters. This way you can test checkAvailableMemory() in isolation.

4. testToString() has the same error: it actually tests testToUnitAndSize(): an error in testToUnitAndSize() will lead to 2 test failures, 1 of them in testToString(), even while toString() may be correct. But we have the same problem here as well: mocking statics.

EDIT: So I followed the link myself and read a bit about mocking statics. The 3 comments starting at https://github.com/sebastianbergmann/phpunit-documentation/issues/77#iss... explain everything. That last comment is what I suggested above, but we will have to use static (late static binding) instead of self ...

Yes, we are almost there. 1 medium and the rest minor.

1. +++ b/core/includes/common.inc
   @@ -249,40 +249,28 @@ function check_url($uri) {
   +  $options = ['langcode' => $langcode];
   

   I did some digging into where this is used and found out that if $langcode is null, we end up doing this in core\lib\Drupal\Core\StringTranslation\Translator\StaticTranslation.php, getStringTranslation(), line 40:
   $this->translations[NULL] = $this->getLanguage(NULL);
   So only assign if not null, otherwise make it an empty array.

2. +++ b/core/lib/Drupal/Component/Utility/EnvironmentMemory.php
   @@ -0,0 +1,115 @@
   +    $this->total = (!$size || $size == -1) ? -1 : Bytes::toInt($size);
   +  }
   

   Minor: Not sure here if any coding standards apply who tell the same, but personally I prefer === (over ==) whenever possible. (4 times in this file.)

3. +++ b/core/lib/Drupal/Component/Utility/EnvironmentMemory.php
   @@ -0,0 +1,115 @@
   +    return ($memory_limit == -1 || $memory_limit >= Bytes::toInt($required));
   

   Minor: I don't like too many brackets, certainly not if they are surrounding the whole expression, as they don't add any clarity in that case. (Several times in this file.)

4. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -436,4 +457,46 @@ public function extensionToImageType($extension) {
   +   * @return null|array
   +   *   NULL if there is sufficient memory to allow the operation. Otherwise, an
   

   Minor: I think it would be more intuitive to return TRUE or an array.

5. +++ b/core/tests/Drupal/Tests/Component/Utility/EnvironmentMemoryTest.php
   @@ -0,0 +1,165 @@
   +      $free_memory = $limit - memory_get_usage(TRUE);
   +      $memory = new EnvironmentMemory();
   +      $this->assertTrue($memory->checkAvailable($free_memory - Bytes::toInt('4k')));
   

   Minor: revers these 2 lines, you are allocating memory after querying the amount of free memory, but before using it.

RE#18.1: Yes this is a "bug", but one that does not show as apparently PHP does not care too much when trying to set index NULL in an array. However it is the code in format_size() that sets the 'langcode" option to NULL. So, as we do touch that code, we can solve it now.
(AFAICS in Drupal the pattern is: $langcode as parameter may be null or absent, 'langcode' in $options parameter may be absent but not NULL.)

RE#18.2: I hadn't thought of -1 coming from ini, in which case a check for == would have been acceptable.

RE#18.4: I would have settled for returning array|true instead of array|null but this is even more strict.

I leave it up to you to change #17.1 here or in another issue. If the latter this one is RTBC.

+++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
@@ -488,15 +496,27 @@ public function checkAvailableMemory($width, $height, $bits_per_pixel = 32, $twe
+   * @return array
+   *   An associative array with information about required, free and total

As the property is typed as array[], we can type this @return as array[] as well.

+++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
@@ -436,4 +465,58 @@ public function extensionToImageType($extension) {
+  public function checkAvailableMemory($width, $height, $bits_per_pixel = 32, $tweak_factor = 1.7) {
+    $required = (int) ($width * $height * ($bits_per_pixel / 8) * $tweak_factor);
+    $memory = new EnvironmentMemory();

And while we are at it now anyway: in theory I guess we should floor ($bits_per_pixel / 8). GD uses 7 bits for transparency, giving 31 bits per pixels, but no way that GD does not use 4 bytes per pixel.

#17.1: I don't understand, a very simple local change.

RE last point#20/#21:
GD uses 31 bits per pixel (8 + 8 + 8 +7), but it will store these 31 bits in 4 bytes and not use the 32nd bit. So, if I would pass 31 as 3rd parameter, the calculation should still use 4 not 31/8.

Actually, you want to know the number of bytes used to store 1 pixel internally, not how many bits it uses for the channels.

Postponed on #2599302: Let translation system accept NULL and empty sting as values for langcode key in $options array parameter.

@mondrake, nice work, really!

I would say this must go in 8.0.x because of some good reasons but it seems that Rectangle() is only in 8.1.x, right? Anyway, some points. Some of them maybe need comments.

1. +++ b/core/includes/common.inc
   @@ -249,40 +249,28 @@ function check_url($uri) {
   +  $value = Bytes::toUnitAndSize($size);
   

   I would create Byte::formatSize() to deprecate format_size(). That can be wrapped in format_size(). Just an idea, opinions?

2. +++ b/core/includes/common.inc
   @@ -249,40 +249,28 @@ function check_url($uri) {
   +      return \Drupal::translation()->formatPlural($value['size'], '1 byte', '@count bytes', [], $options);
   

   Not so sure what is better to use here: \Drupal::translation()->formatPlural() or new PluralTranslatableMarkup(). I see that \Drupal::translation()->formatPlural() doesn't wrap a service and calls the 2nd. I would go directly with new PluralTranslatableMarkup(). But it's up on you.

3. +++ b/core/lib/Drupal/Component/Utility/Bytes.php
   @@ -44,4 +44,36 @@ public static function toInt($size) {
   +   * @return array
   +   *   An associative array with the following keys:
   +   *   - 'unit': unit of measurement (string)
   +   *   - 'size': size (float, rounded to 2 decimals).
   +   *   An empty array if the input value is negative.
   ...
   +      return [];
   ...
   +    return ['unit' => $unit, 'size' => round($size, 2)];
   

   I think in such cases we return stdClass. I don't have a strong opinion, but it seems to me more intuitive and DX.

4. +++ b/core/lib/Drupal/Component/Utility/Environment.php
   @@ -9,6 +9,9 @@
   + * @deprecated in Drupal 8.1.x, will be removed before Drupal 9.0.0.
   + *   Use \Drupal\Component\Utility\EnvironmentMemory.
   
   @@ -28,18 +31,21 @@ class Environment {
   +   * @deprecated in Drupal 8.1.x, will be removed before Drupal 9.0.0.
   +   *   Use \Drupal\Component\Utility\EnvironmentMemory:
   ...
      public static function checkMemoryLimit($required, $memory_limit = NULL) {
   

   I know that it's ugly but "Use" should go one line above.

5. +++ b/core/lib/Drupal/Component/Utility/EnvironmentMemory.php
   @@ -0,0 +1,115 @@
   +   * Will be set to -1 when no memory limit is set or it is set to unlimited.
   ...
   +   *   The total memory, in bytes, available to PHP. Returns -1 when no memory
   ...
   +    $this->total = (!$size || (int) $size === -1) ? -1 : Bytes::toInt($size);
   ...
   +   *   The amount of memory, in bytes, free for use by PHP. Returns -1 when no
   ...
   +    return $this->getTotal() === - 1 ? -1 : $this->getTotal() - $this->getUsed();
   ...
   +    return $memory_limit === -1 || $memory_limit >= Bytes::toInt($required);
   

   Let's create a constant for -1. The code will be more readable and that can be used also outside to to test.

6. +++ b/core/lib/Drupal/Component/Utility/EnvironmentMemory.php
   @@ -0,0 +1,115 @@
   +   * @param int|string $size
   

   It seems that we can pass also NULL? Then null should be added.

7. +++ b/core/lib/Drupal/Component/Utility/EnvironmentMemory.php
   @@ -0,0 +1,115 @@
   +   *   bytes with optional SI or IEC binary unit prefix (e.g. 2, 3K, 5MB, 10G,
   ...
   +   *   with optional SI or IEC binary unit prefix (e.g. 2, 3K, 5MB, 10G, 6GiB,
   ...
   +   *   with optional SI or IEC binary unit prefix (e.g. 2, 3K, 5MB, 10G, 6GiB,
   

   "SI or IEC" can se add some @see links to the standards?

8. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -65,6 +67,13 @@ class GDToolkit extends ImageToolkitBase {
   +  protected $memoryInfo;
   
   @@ -436,4 +465,63 @@ public function extensionToImageType($extension) {
   +   * @return array[]
   +   *   An associative array with information about required, free and total
   +   *   memory.
   ...
   +  public function getMemoryInfo() {
   

   Maybe it's good to explain in docblock what keys holds this array (required, free, total).

9. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -436,4 +465,63 @@ public function extensionToImageType($extension) {
   +  public function checkAvailableMemory($width, $height, $bits_per_pixel = 31, $tweak_factor = 1.7) {
   
   +++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/CreateNew.php
   @@ -84,6 +84,12 @@ protected function validateArguments(array $arguments) {
   +    if (!$this->getToolkit()->checkAvailableMemory($arguments['width'], $arguments['height'])) {
   +      $memory = $this->getToolkit()->getMemoryInfo();
   

   Why not merging checkAvailableMemory() and getMemoryInfo(). The we can drop also the protected method. We return an empty array if ok (and that cast to FALSE) and the memory info array (cast to TRUE) when error. Then we need to reverse the semantics/naming of checkAvailableMemory()

1. +++ b/core/lib/Drupal/Component/Utility/EnvironmentMemory.php
   @@ -0,0 +1,127 @@
   +    $this->total = (!$size || (int) $size === -1) ? static::UNLIMITED : Bytes::toInt($size);
   

   Forgot a -1 :)

2. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -181,6 +185,24 @@ protected function load() {
   +    if (!empty($memory)) {
   
   +++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/CreateNew.php
   @@ -84,6 +84,12 @@ protected function validateArguments(array $arguments) {
   +    if (!empty($memory)) {
   
   +++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Rotate.php
   @@ -98,6 +99,14 @@ protected function execute(array $arguments) {
   +    if (!empty($memory)) {
   

   if (!$memory) {...} ?

3. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -436,4 +458,54 @@ public function extensionToImageType($extension) {
   +  public function memoryCheck($width, $height, $bits_per_pixel = 31, $tweak_factor = 1.7) {
   

   Hm. We need something reversed semantically. Something that is TRUE when there's no enough memory. memoryExhausted(), memoryLimited()... I'm out of ideas.

Please undo #33.3. IMO, this reduces readability. Method names should preferably be named positive, even if the result will always be checked with a ! in front of it. If you want to keep it this way, then I think, though I'm not native english, that unavailable is "uncommon", (i.e. not common), so then prefer not(Enough)Available. If you reverse then you could rename the less specific "memoryCheck" to a more explicit "isMemoryAvailable", that is IMO clearer.

#27.3: Let's see how it looks with a dedicated class but that would need 2 setters (with chaining) and 2 getters, not public methods. It seems to me a little bit over-engeeniring.

#27.9: So, we agree with a single method? It's more simple and clear. The only thing left is the naming. This is tricky. When reveals a positive state (has memory) it returns nothing (empty array is casting to FALSE). When reveals a negative state (not enough memory) it returns something (a non-empty array that is casting to TRUE). What about memoryLimitReached()?

Alternatively we can return mixed. We can return TRUE when there is memory and array when not. Then

if (($check = $this->getToolkit()->checkMemory(...)) !== TRUE) {
  ...
}

I'm OK with #38.

We could also return a bool and place the additional info in a(n optional) parameter that is passed by reference, as in preg_match()?

[REMOVED: same message, double clicked on the save button]

Wait! Why not throwing the exception directly in isMemoryAvailable() and we're done? :)

I'm marking this to provide more review tomorrow, when I'm attending Drupal Global SprintWeekend Luxembourg.

isMemoryAvailable() is a helper function and should (IMO) therefore not log on its own, but leave that to the code that knows the context it's running in. E.g: I might be doing a batch conversion and only present/log overall results at the end, not 1 error message per failure. isMemoryAvailable() does not know this, but that calling code would.

+1, ran into this too.

I just got bit by this on a D7 site. The image wasn't uploaded through Drupal. The image url came from a web service, so there was no way the UI could have prevented the image. Can't catch the error because it's a memory issue, took a long time to figure out the root problem. Happy that I found this issue, but now I need to do something about it.

Added postponed by issues to the issue summary since it's not readily apparent from the related issues.

Curious if the method could be packaged to trait

Looks ready for commiters

I think we need a change record here to tell contrib modules that provide operations about the new functionality.

Needs work for #103 and @catch's and my code review.

good idea with limit, hope it now ready for 10.0.x as CR states

I'm really not sure about this change - or if we can make it in the way we're doing here. I've run some tests on memory usage for images on my local PHP 7.4...

vendor/bin/drush php
Psy Shell v0.10.12 (PHP 7.4.30 — cli) by Justin Hileman
Drush Site-Install (Drupal 9.5.0-dev)
>>> ini_set('memory_limit', '2G');
=> "4G"
>>> ini_get('memory_limit');
=> "2G"
>>> format_size(memory_get_usage());
=> Drupal\Core\StringTranslation\TranslatableMarkup {#4613
     markup: "29.24 MB",
   }
>>> format_size(\Drupal\system\Plugin\ImageToolkit\GDToolkit::getRequired(20000, 20000));
=> Drupal\Core\StringTranslation\TranslatableMarkup {#4619
     markup: "2.53 GB",
   }
>>>  $test = \Drupal::service('image.factory')->get(NULL, 'gd');
=> Drupal\Core\Image\Image {#4625}
>>> $a =  $test->createNew(20000, 20000);
=> true
>>> format_size(memory_get_usage());
=> Drupal\Core\StringTranslation\TranslatableMarkup {#4640
     markup: "29.68 MB",
   }
>>>

How sure are we that the function here is going to always prevent fails and do the right thing. For me this is better handled by max file upload sizes. PHP settings already handle that.

I think maybe we should consider how we fail in image operations. Catching throwables, logging the error and returning FALSE seems like a great idea but doing memory calculations ourselves seems unnecessarily complex and handling the problem at the wrong point.

Discussed this with @alexpott briefly in slack and after going around in circles a couple of times, I think we should do the throwables bit (i.e. what the issue title says), but not the memory calculation here. We could always move that to a follow-up if it needs more investigation/discussion.

For the issue summary update requested in #113

MR is up to date.
Tests are all green.
No build errors.
Tests were added for the bug.
Tests fail without the whole fix.

Looks good

Committed 23edd01 and pushed to 10.1.x. Thanks!

Only committed to 10.1.x as we're changing where errors are caught so this results in subtle code flow changes. Also I think we can delete the CR as I'm not sure one is necessary and the API talked about in the CR is not there.

Unfortunately this change has introduced a random fail - see https://www.drupal.org/pift-ci-job/2556244 and https://www.drupal.org/pift-ci-job/2556296 - it seems to happen on postgres which is odd as I have no idea why this change would be DB related.

Curious why memory is limited by 12MB as both failed runs reports

The testbot is not happy.

But then we need a followup to keep trying to find a solution for insufficient memory issues.

Does this still need a followup?

Do you have a recommended way to manually test? Tried lowering my memory and using the snippet in the IS but couldn't trigger.

Can the issue summary be updated please. Since this is not about memory.

Thank you!

Tested this out by editing Drupal\system\Plugin\ImageToolkit\Operation\gd\Convert changing line 59 to

if ($this->getToolkit()->apply('creatfasde_new', $data)) {

Without the patch when I create a media object I get 2 errors in the logs

Image convert failed using the gd toolkit on and

The selected image handling toolkit 'gd' can not process operation 'creatfasde_new'.

Applied the fix

Created another media object

No errors logged.

Committed 4e49b11 and pushed to 10.1.x. Thanks!